Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6244 closed defect (bug) (fixed)

Custom Component Directory pages mappings removed when BuddyPress Componets selection saved in admin

Reported by: sbrajesh Owned by:
Milestone: 2.2.2 Priority: high
Severity: normal Version: 2.2
Component: Administration Keywords: has-patch
Cc: sbrajesh

Description

Hi,
In BuddyPress 2.2/2.2.1 when you set the directory pages for a custom component, It works fine but when you try to save the Components from BuddyPress->Components(even if you do not make any change in selection), the mappings for the custom component directory pages are lost and you will get a message 'The following active BuddyPress Components do not have associated WordPress Pages: component name'.

The problem is caused by a change in BuddyPress 2.2. There was a change in the bp_core_get_directory_page_ids that checks for the active components before returning the directory pages.

In the bp_core_admin_components_settings_handler, the active component is being reset by the following code

$bp->active_components = bp_core_admin_get_active_components_from_submitted_settings( $submitted );

and then bp_core_add_page_mappings() is called for saving the pages. Since bp_core_add_page_mappings uses bp_core_get_directory_page_ids which in turn checks for the active components, All the custom component associations are lost.

I am not sure if I made myself clear, so here is the step to reproduce it:-

  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
  1. Now visit, Settings->BuddyPress->Components and click Save settings. The page association for the Example component will be lost.

A solution can be returning all the core components(selected) as well as all other active components from the 'bp_core_admin_get_active_components_from_submitted_settings' if the checks for bp_is_active('component' ) is to be honored.

Relevant ticket: #6197

Attachments (3)

6244.patch (1.5 KB) - added by imath 5 years ago.
6244.unittest.patch (2.1 KB) - added by imath 5 years ago.
6244.02.patch (3.1 KB) - added by johnjamesjacoby 5 years ago.
Combined patch

Download all attachments as: .zip

Change History (20)

#1 @imath
5 years ago

  • Keywords reporter-feedback added

Hi sbrajesh

Are you making your component active from your component class like this : https://github.com/boonebgorges/buddypress-skeleton-component/blob/1.7/includes/bp-example-loader.php#L64 ?

There was a problem with BuddyPress links in this area, see my reply on this forum topic https://buddypress.org/support/topic/buddypress-2-2-and-links-page/#post-233733

#2 @sbrajesh
5 years ago

hi imath,
Yes, I am adding the component to the active components list as in the example component. I also saw the thread on bp.org, That thread is about saving the pages(if component does not add itself to the list of active components, the pages associate is not saved in Settings->BuddyPress->pages).

The above problem is related to resetting the page association when you try to save the components list in the Settings->BuddyPress->Components

You can reproduce the error by using https://github.com/boonebgorges/buddypress-skeleton-component

I have tested it on WP 4.1(non multisite) and BP 2.2.1

#3 @imath
5 years ago

I see, if i understand well, it works the first time when setting the page association, but if once the association made i click again, setting is lost, is that correct ?
I'll run some tests asap, thanks for your feedback.

#4 @imath
5 years ago

  • Keywords needs-patch needs-unit-tests added; reporter-feedback removed
  • Milestone changed from Awaiting Review to 2.2.2

Ok i've managed to reproduce. It's a regression from 2.1. I think we should fix this for 2.2.2.
Working on it.

Last edited 5 years ago by imath (previous) (diff)

#5 @sbrajesh
5 years ago

That's awesome. it is being caused due to the bp_is_active check in getting the page ids. Instead of removing that, some change in 'bp_core_admin_get_active_components_from_submitted_settings' can fix the issue.

Looking forward to your patch :)

#6 @imath
5 years ago

  • Keywords has-patch added; needs-patch needs-unit-tests removed

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.

So 6244.patch is setting an array of custom components by difference between the packaged components and the active components. Then i let the switch part do his job : setting the packaged active components. Finally i return the pakaged components + the custom ones by merging the two arrays.

6244.unittest.patch is only containing the unit test.

@imath
5 years ago

@imath
5 years ago

#7 @boonebgorges
5 years ago

imath - thanks for looking at this. This seems like a good fix. sbrajesh - it'd be great if you could test.

#8 @johnjamesjacoby
5 years ago

This looks good to me.

LOL @ test_bp_core_admin_get_active_components_from_submitted_settings_should_keep_custom_component_directory_page

#9 @sbrajesh
5 years ago

Thank you for the patch imath.

boonebgorges - It is working perfectly for me.

#10 @imath
5 years ago

thanks for your feedback sbrajesh, johnjamesjacoby, & boonebgorges

@johnjamesjacoby i'm pretty proud about the name of this test :) but open to improvements, of course ;)

@johnjamesjacoby
5 years ago

Combined patch

#11 @johnjamesjacoby
5 years ago

In 9553:

Core: Allow custom components to continue to have directory pages.

Fixes regression introduced in 2.2. Props imath. See #6244. (2.2 branch)

#12 @johnjamesjacoby
5 years ago

In 9554:

Missed test in r9553. See #6244. (2.2 branch)

#13 @johnjamesjacoby
5 years ago

In 9555:

Core: Allow custom components to continue to have directory pages.

Fixes regression introduced in 2.2. Props imath. See #6244. (trunk)

#14 @johnjamesjacoby
5 years ago

  • Priority changed from normal to high
  • Resolution set to fixed
  • Status changed from new to closed

Fixed in 2.2 branch and trunk. Thanks everyone.

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


5 years ago

#16 @boonebgorges
5 years ago

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.

#17 @boonebgorges
5 years ago

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.

Note: See TracTickets for help on using tickets.