Project

General

Profile

Actions

Feature #19493

closed

Dealing with BuddyPress 12.x upgrade

Added by Boone Gorges 3 months ago. Updated about 2 months ago.

Status:
Resolved
Priority name:
Normal
Assignee:
Category name:
-
Target version:
Start date:
2024-01-05
Due date:
% Done:

0%

Estimated time:
Deployment actions:

git submodule update (for social-paper)
Network-activate bp-classic
Check main nav menus.


Description

At some point soon, we will need to upgrade to BuddyPress 12.x. In the near term, we'll need to install BP Classic alongside in order to ensure compatibility with existing customizations and plugins.

Ray, I'm tentatively planning this for the first minor release after our upcoming 2.3.0 release. But I'd like your thoughts on this, and in particular on whether this upgrade might need the kind of testing that would require more time, or perhaps should even wait for a summer release.


Related issues

Related to CUNY Academic Commons - Bug #19944: Dealing with BuddyPress 12.x upgrade, pt. 2NewBoone Gorges2024-03-11

Actions
Actions #1

Updated by Raymond Hoh 3 months ago

Activating the BP Classic plugin will help, but I think we would benefit from additional time and testing to ensure all our custom BuddyPress code will continue to work.

I'm probably overthinking this and perhaps this might not require a lot of time to get things working, but here are some quick thoughts:

Actions #2

Updated by Boone Gorges 3 months ago

  • Target version changed from 2.3.1 to 2.3.2

Thanks for laying out these thoughts, Ray. I don't think you are overthinking. I'm probably underthinking :-D

My takeaway is that we'll need meaningful testing before moving forward with the change. To do that testing, it's probably wise for us to have cdev at our disposal. So it's not going to happen for 2.3.1. Let's target 2.3.2, which will be the second Tuesday in February.

A rough overview:
1. Deprecated code analysis, as you suggest. I'm hopeful the numbers will be somewhat small, and that we'll be able to provide the handful of functions without loading all BP deprecated libraries.
2. Figure out how to handle references to bp-default
3. Manual checking of primary BP functionality. This will include: group creation; display and creation of group-related content (Library, etc); invites; directories; site creation.

Let's start this process after the 2.3.0 release so we can have a shared space for testing.

Actions #3

Updated by Boone Gorges 3 months ago

  • Deployment actions updated (diff)

I've taken a first pass at the BP 12.0 upgrade, drawing on the conversation above. Here's what I've done.

1. On the bp12 branch:

- Update BP https://github.com/cuny-academic-commons/cac/commit/c3770fa39248ed6f4193955d5524177c3330a1f2
- Install bp-classic https://github.com/cuny-academic-commons/cac/commit/41efd3f71b6603d1851a9b951eca97aa634960c3 I played with ways of requiring this plugin without needing to activate it (in part to avoid fatals during the upgrade process) but it didn't work. For one thing, bp-classic has an activation routine it needs to run. For another, there's some magic in the plugin that prevents it from loading if it detects that you're not running an "approved" combination (ie, if BP is network-activated, the plugin bootstrap bails). Too smart for us. Anyway, I'll just have to run a manual activation after deployment.
- bp-classic changed the names of its widget classes, so we have to make a change to a place where we extend one of those classes https://github.com/cuny-academic-commons/cac/commit/41b53b28c79d9f5963928daba0aecc26ec575d3e. In addition, I put a change in place here so that our own widget library is loaded only if bp-classic is available - this is mainly to avoid fatals when first deploying bp-classic in an environment.
- Fix the path for the bp-default stylesheet https://github.com/cuny-academic-commons/cac/commit/8946594b0d7eeeed9dec1337df72d4f756de8954
- I reviewed BP 12 deprecated functions and found that one was used in social-paper. I'm unsure whether it will ever come up, but I added a polyfill in social-paper https://github.com/cuny-academic-commons/social-paper/commit/2edcf6539975d57bdecef4f642ab64820090712b, https://github.com/cuny-academic-commons/cac/commit/798cd5a8731572273aa99dddb7c58501d877d692

On my initial tests, the major parts of BP are working (home page, directories, individual member pages, individual group pages).

Ray, perhaps you'd like to take a look and see if I've missed anything obvious?

Actions #4

Updated by Raymond Hoh 2 months ago

I tested bp12 branch and came across a few issues:
Actions #5

Updated by Boone Gorges 2 months ago

Ray, thanks for having a look.

In the Group Library, clicking on a doc throws a 404.

I couldn't reproduce this, either on cdev or in my local environment. Can you share more details, or maybe a URL on cdev where I can test?

Pagination is not working on a group's /forum/ page or when in a single forum thread. There's a patch on bbPress Trac that hasn't been committed yet: https://bbpress.trac.wordpress.org/ticket/3576 . I haven't tested the patch yet.

I couldn't reproduce this either. See https://commons.gc.cuny.edu/groups/cac-community-team-project-planning/forum/ on cdev: https://commons.gc.cuny.edu/groups/cac-community-team-project-planning/forum/page/2/ My reading of that specific ticket is that bp-classic makes it a non-issue, since it reverts to the old URI-global system for page routing.

Some of our custom BuddyPress features need to run on the 'bp_parse_query' hook instead of 'init' or 'bp_init' due to the switch to use WP permalinks in BP 12. I've fixed up one instance in https://github.com/cuny-academic-commons/cac/commit/bf399847d075a68ac784e53735861012509f36d4 . There may be a few more places to check, but will need to do further testing.

Yeah, thanks for remembering this. I reviewed the following locations for items hooked to 'init' or 'bp_init': wp-content/plugins/cac-*, wp-content/mu-plugins, wp-content/themes/bp-nelo. I didn't find any additional places where callbacks made reference to BP's routing globals. But this is only a partial search - it's not possible for us to look at every 'init' callback in the entire codebase. So I agree that we probably just need more testing of common actions.

Actions #6

Updated by Raymond Hoh 2 months ago

In the Group Library, clicking on a doc throws a 404.

I couldn't reproduce this, either on cdev or in my local environment. Can you share more details, or maybe a URL on cdev where I can test?

Oops! Forgot to network-activate BP Classic! Group doc 404 is now resolved.

However, forum and thread pagination on group pages are still broken. For example, see https://commons.gc.cuny.edu/groups/cac-community-team-project-planning/forum/page/2/. The page is still reflecting the contents from page 1. I think we will need to hold off on deploying BuddyPress 12 until we decide to patch bbPress ourselves or until bbPress releases a new version.

I reviewed the following locations for items hooked to 'init' or 'bp_init': wp-content/plugins/cac-*, wp-content/mu-plugins, wp-content/themes/bp-nelo

Found one more instance in our bp-nelo theme. Addressed in https://github.com/cuny-academic-commons/cac/commit/4c609c1430253e4f1ef91304978dd3eca059b6b7


One thing I noticed on cdev is our BP directory pages in the nav menu are using the unpretty WordPress permalink (eg. /?post_type=buddypress&p=X). Might be something to be aware of when we deploy this to production. My local install isn't affected by this though.

Actions #7

Updated by Boone Gorges 2 months ago

  • Target version changed from 2.3.2 to 2.3.3

However, forum and thread pagination on group pages are still broken. For example, see https://commons.gc.cuny.edu/groups/cac-community-team-project-planning/forum/page/2/. The page is still reflecting the contents from page 1. I think we will need to hold off on deploying BuddyPress 12 until we decide to patch bbPress ourselves or until bbPress releases a new version.

It's strange - I can't reproduce the pagination thing on my local checkout, but I see it happening on cdev. I thought maybe it was a rewrite-rule issue, but a flush didn't help.

In any case, we might be waiting for a long time if we wait for bbPress to be patched. So I think we're going to have to do it ourselves, and run a fork of bbPress until such time (if any) that bbPress is updated. Do you have the bandwidth to have a first attempt at this in the next week or two?

Actions #8

Updated by Boone Gorges 2 months ago

I managed to reproduce the bbPress pagination issue while working on another project, and it is in fact due to rewrite rules. BP's new system registers a rule that matches group forum /topic/[topic-name]/page/ queries:

 groups/([^/]+)/([^/]+)/(.+?)/?$  => index.php?bp_groups=1&bp_group=$matches[1]&bp_group_action=$matches[2]&bp_group_action_variables=$matches[3] 

This rule is registered even when running bp-classic. And it's registered with a higher priority than the WP rule that handles /page/ pagination:

(.?.+?)/page/?([0-9]{1,})/?$ => index.php?pagename=$matches[1]&paged=$matches[2] 

As a result, the 'paged' query var is never set, and WP_Query (and therefore bbPress) thinks that we're always on page 1.

Here's a very hacky workaround that I put in place on the other project:

        // ...
    add_filter( 'bbp_get_global_object', array( $this, 'fix_rewrite_pagination' ), 10, 2 );
        // ...

    /**
     * Fixes rewrite pagination.
     *
     * Required for compatibility with BP 12.0, where the /groups/ rewrite rule will
     * be caught before bbPress's /page/ rule.
     */
    public function fix_rewrite_pagination( $object, $type ) {
        if ( 'wp_query' !== $type ) {
            return $object;
        }

        if ( ! bp_is_group() ) {
            return $object;
        }

        if ( ! bp_is_current_action( 'forum' ) ) {
            return $object;
        }

        $page_number = null;

        // Can't use bbp_is_single_topic() because it triggers a loop.
        $is_single_topic = bp_is_action_variable( 'topic', 0 );

        if ( $is_single_topic ) {
            if ( bp_is_action_variable( 'page', 2 ) ) {
                $page_number = bp_action_variable( 3 );
            }
        } else {
            if ( bp_is_action_variable( 'page', 0 ) ) {
                $page_number = bp_action_variable( 1 );
            }
        }

        if ( ! $page_number ) {
            return $object;
        }

        $object->set( 'paged', $page_number );

        return $object;
    }

I think a more correct fix is for bbPress's BP extension to register its own pagination rewrite rules for groups. Something like:

 groups/([^/]+)/forum/page/?([0-9]{1,})/?$  => index.php?bp_groups=1&bp_group=$matches[1]&bp_group_action=forum&paged=$matches[2]  // pagination for the single-forum topic listing
 groups/([^/]+)/forum/topic/([^/]+)/page/?([0-9]{1,})/?$  => index.php?bp_groups=1&bp_group=$matches[1]&bp_group_action=forum&paged=$matches[3]&bbp_topic=$matches[2]  // pagination for single-topic - I don't know how the bbp_topic part would work, this would need to be figured out in bbPress

Furthermore, when running bp-classic, I had to do some additional work to ensure that bbPress's BP compatibility layer is loaded in time, and is loaded only once. Like the approach above, it's pretty hacky (requiring a static variable to prevent double-loading) but it basically works. Here's the full patch: https://github.com/openlab-at-city-tech/openlab/commit/27f2a68354550295d85f0f3c5aaafc55004268f0

I don't think that imath's patch on https://bbpress.trac.wordpress.org/ticket/3576 handles either the bp-classic load-order issue or the pagination issue. I'll post a note on that ticket.

Actions #9

Updated by Boone Gorges about 2 months ago

  • Deployment actions updated (diff)

I checked against the latest release of bp-classic, and it appears that it fixes the forum pagination issues.

The plan is to go forward with the BP 12.x upgrade during tomorrow's release. I'll wait to do the final merge into the 2.3.x branch until tomorrow morning, in case Ray thinks of any reason between now and then why it shouldn't move forward :-D

Actions #10

Updated by Raymond Hoh about 2 months ago

  • Deployment actions updated (diff)

The plan is to go forward with the BP 12.x upgrade during tomorrow's release. I'll wait to do the final merge into the 2.3.x branch until tomorrow morning

One thing I added to the bp12 branch is I updated BuddyPress to the latest version -- v12.3.0 -- as that includes an important fix for AJAX that I posted to BP Trac here: https://buddypress.trac.wordpress.org/ticket/9089

I think we should be good to go with regards to the rollout in the morning.


For more pronounced changes, I created a new branch -- bp12-part2 -- that namely switches out all BuddyPress 12 deprecated function calls. We can roll that out in a later maintenance release.

I've been testing bp12-part2 with BP Classic deactivated and with BP_IGNORE_DEPRECATED set to true and these are some issues I encountered:

About the bbPress load order issue:

Furthermore, when running bp-classic, I had to do some additional work to ensure that bbPress's BP compatibility layer is loaded in time, and is loaded only once. Like the approach above, it's pretty hacky (requiring a static variable to prevent double-loading) but it basically works. Here's the full patch: https://github.com/openlab-at-city-tech/openlab/commit/27f2a68354550295d85f0f3c5aaafc55004268f0

I tackled this a little differently in my bbPress commit. See https://github.com/cuny-academic-commons/cac/commit/e2145f2462d3d2cbe2f16de914e7875c48b6e1cc#diff-0f345898d2c7666be77acc95ec88e90e98a407644a4ce5431b3fb96d06749039 and the constructor and includes() portion of https://github.com/cuny-academic-commons/cac/commit/e2145f2462d3d2cbe2f16de914e7875c48b6e1cc#diff-a8d1ad6e40805411d0e557ee641e9d47c6cc3d0f17ef107f22ac729255439a5a . I think the constructor changes might fix your double load order issue. Can you do some tests and see if this works for you, Boone?

Actions #11

Updated by Raymond Hoh about 2 months ago

  • Deployment actions updated (diff)
Actions #12

Updated by Boone Gorges about 2 months ago

  • Status changed from New to Resolved
Actions #13

Updated by Raymond Hoh about 1 month ago

  • Related to Bug #19944: Dealing with BuddyPress 12.x upgrade, pt. 2 added
Actions

Also available in: Atom PDF