Custom BP Component dropped from BP Pages Association Settings in some cases with 2.2 changes

[dtc7240]

I found a potential problem that could affect BuddyPress site admins in the future given recent changes in 2.2.0. I found it while tracking down a problem that it turns out I had caused between two of my own plugins. I thought I would note it here in case it becomes an actual problem for anyone else. It COULD affect site admins in a manner similar to #6197 and #6226, but is not related to a multisite installation.

Scope: This potentially affects any BuddyPress Component that has it's own directory (based on the Skeleton Component Plugin).

Potential Issue: The WordPress page associated with the component in the BuddyPress->Settings "Pages" Association screen can become disassociated in certain circumstances, forcing the site admin to manually re-associate it.

Circumstances: If one of the following events occur WHILE a plugin within this scope is temporarily deactivated, the issue will manifest when the plugin is reactivated:
1) delete_post action hook is fired
2) bp_core_add_page_mappings() is called
3) "Save Settings" button on the BuddyPress->Settings->"Pages" tab is clicked

Flow: Any of the three events listed above do the following:
1) They call bp_core_update_directory_page_ids() to return a list of pages from the "bp-pages" entry of the wp_options table.
2) They make whatever additions or subtractions to the list of pages they are supposed to.
3) They ultimately call bp_update_option( 'bp-pages', $page_ids ) to rewrite the values of the "bp-pages" entry in the table.

Problem: In BuddyPress 2.2, an addition was made to bp_core_get_directory_page_ids() causing it to remove any inactive components from the list of pages it found in the "bp-pages" entry before passing the list of pages to whatever called it. If any of the three events listed above occur while the plugin is inactive, the page associated with the plugin is removed from the "bp-pages" entry. Therefore, when the site admin reactivates the plugin, it no longer has it's appropriate WordPress page associated with it and it has to be manually reset.

I believe the changes to bp_core_get_directory_page_ids() were good, as this function is called from a lot of different places, many of which are better off not having to process through inactive pages. The changes, however, had the unintended side-effect noted in this ticket since bp_core_get_directory_page_ids() is also used by the three events noted above that ultimately rewrite the "bp-pages" entry in the wp_options table.

Possible Soution: Maybe add an optional parameter to bp_core_get_directory_page_ids() that the three processes noted above use to have it not remove inactive components before returning its list?

Thanks, Scott

[boonebgorges]

Thanks for the report, dtc7240. Is this the same issue as #6244?

[dtc7240]

Absolutely related @boonebgorges, but the fix for #6244 was too far upstream to help in the three events I lined out above.

@imath graciously took on fixing #6244 directly at the source for the specifically reported issue ( Clicking the "Save Settings" button on the BuddyPress->Settings->Components tab ) as follows:

I've chosen to edit the bp_core_admin_get_active_components_from_submitted_settings() as the bp_is_active() check in the bp_core_get_directory_page_ids() was an improvement introduced in 2.2 and i think bp_core_admin_get_active_components_from_submitted_settings() should only take care of the BuddyPress built in components.

However, I found that if any of the three events above are called WHILE THE CUSTOM COMPONENT IS TEMPORARILY DISABLED, they run into the same issue with the bp_is_active check in bp_core_get_directory_page_ids() causing the page association to be lost. The easiest route to reproduce this is:

1. Install custom component( try buddypress-skeleton-component)
2. activate the plugin
3. Visit Settings->BuddyPress->Pages and save the associated page for Example component
4. Deactivate the plugin
5. Now visit, Settings->BuddyPress->Pages (You will not see the example component listed because it is not active at the moment ) and click Save settings.
6. Reactivate the plugin
7. Go back to Settings->BuddyPress->Pages and you'll see that the page association for the Example component is lost.

Again, the three events (that I found) that will cause this problem (while the custom component is temporarily deactivated) are:

1. delete_post action hook is fired (causing the also new bp_core_on_directory_page_delete() function to run)
2. bp_core_add_page_mappings() is called (by something other than bp_core_admin_get_active_components_from_submitted_settings() now that imath has fixed that upstream)
3. "Save Settings" button on the BuddyPress->Settings->Pages tab is clicked (running bp_core_admin_slug_handler())

It's arguably not the most horrific problem in the world, but it could cause some frustration at some point (as it did me...technically it just exacerbated a mistake I'd already made between two of my own custom plugins...but I shan't tell on myself any more).

I would suggest that any event (such as these three) that have the effect of re-populating the "bp-pages" entry in the wp_options table should skip the bp_is_active() check in the bp_core_get_directory_page_ids() function. The easiest way I can see to accomplish that is to add an optional parameter to bp_core_get_directory_page_ids() that if passed in as true will skip the bp_is_active() check. Then add that parameter as true to at least the three events I found thus far that call bp_core_get_directory_page_ids() and that end up rewriting the "bp-pages" entry:

1. bp_core_on_directory_page_delete()
2. bp_core_add_page_mappings()
3. bp_core_admin_slug_handler()

I think (but could be quite wrong) that this would have also resolved #6244.

Scott

[boonebgorges]

Thanks so much for the additional info. On review, I think you're right. To put your point in another way. There are two different kinds of context in which bp_core_get_directory_page_ids() is called:

1. In a read-only fashion. This happens mainly (a) when we're doing URL parsing, but also (b) when we're building settings markup (since inactive components don't need to be shown in the page mapping settings)
2. When doing save validation. These are the cases that Scott lists in his most recent comment (bp_core_on_directory_page_delete(), etc).

In cases like (2), we should take care not to be destructive of data - for third-party components or for BP's packaged components. The bp_is_active() filters should only apply in cases like (1), so that they don't have to apply in cases like (2).

In 6280.diff​, I've added a $status argument to bp_core_get_directory_page_ids(), and then changed up our use of the function to pass status=all when necessary. Scott and others, this could use a review, both to see if it fixes the current problem, and to see if this supercedes the fix from #6244 [9553].

[dtc7240]

Perfectly stated Boone. Read-only vs save validation. I hope to be able to communicate so effectively in so few words when I grow up ;-)

I really appreciate you adding this to 2.2.2!

Scott

[imath]

@boonebgorges

Looks great. This is what i've tested :
1- activate BuddyPress skeleton component branch 1.7
=> i get the warning message to associate a WordPress page <= ok

2- Associate the example component to a page
=> the warning message disapears <= ok

3- trash the page
=> i get the warning message to associate a WordPress page <= ok

4- Untrash the page
=> the warning message disapears <= ok

5- Update the core components from the components settings (eg: activate the blogs component)
=> No warning, the association is kept for the example component <= ok see #6244

6- Deactivate BuddyPress skeleton component, update the core components (eg: deactivate the blogs component) and reactivate the BuddyPress skeleton component
=> No warning, the association is kept for the example component <= ok

7- Ran the unittests : no error relative to this part.

So i'd say it's all good :)

I'm only adding a no-prefix version of the patch so that it will be faster to test ;)

[boonebgorges]

In 9608:

bp_core_get_directory_page_ids() should skip bp_is_active() check in save context.

In [9177], a bp_is_active() check was added to bp_core_get_directory_page_ids()
such that the function would not return page mappings for inactive components.
This change caused problems for custom components, as the bp_is_active()
check didn't always pass for components that didn't manually set themselves in
the active_components array. [9553] and [9555] addressed this problem by only
filtering out packaged components. See #6244. But this fix was not truly
general, as it still resulted in the deletion of bp-pages entries related to
third-party deactivated components when saving the page mapping settings.

This changeset introduces a more general fix, by distinguishing between two
different use "contexts" of bp_core_get_directory_page_ids():

• Read-only - When pulling up page IDs for display or for URI parsing, deactivated components should be ignored.
• Save - When modifying the bp-pages array stored in the database, we should be working with raw page data, including deactivated components.

The new $status parameter for bp_core_get_directory_page_ids() addresses
this distinction (possible values 'all' and 'active'). We then pass 'all' as
required in BP - namely, when modifying the bp-pages setting. This change
supercedes the fixes from #6244.

Props dtc7240, boonebgorges.
Fixes #6280.

[boonebgorges]

In 9609:

bp_core_get_directory_page_ids() should skip bp_is_active() check in save context.

In [9177], a bp_is_active() check was added to bp_core_get_directory_page_ids()
such that the function would not return page mappings for inactive components.
This change caused problems for custom components, as the bp_is_active()
check didn't always pass for components that didn't manually set themselves in
the active_components array. [9553] and [9555] addressed this problem by only
filtering out packaged components. See #6244. But this fix was not truly
general, as it still resulted in the deletion of bp-pages entries related to
third-party deactivated components when saving the page mapping settings.

This changeset introduces a more general fix, by distinguishing between two
different use "contexts" of bp_core_get_directory_page_ids():

• Read-only - When pulling up page IDs for display or for URI parsing, deactivated components should be ignored.
• Save - When modifying the bp-pages array stored in the database, we should be working with raw page data, including deactivated components.

The new $status parameter for bp_core_get_directory_page_ids() addresses
this distinction (possible values 'all' and 'active'). We then pass 'all' as
required in BP - namely, when modifying the bp-pages setting. This change
supercedes the fixes from #6244.

Props dtc7240, boonebgorges.
Fixes #6280.