Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 8 years ago

#3428 closed defect (bug) (fixed)

Better bp-pages integration for third-party components with top-level directories

Reported by: boonebgorges Owned by:
Milestone: 1.5 Priority: critical
Severity: critical Version: 1.5
Component: Core Keywords: has-patch 2nd-opinion
Cc: enderandrew

Description

See also #3422, which points to a specific bug in bp-pages and bp_core_add_root_component() implementation.

Integration of third party components into bp-pages is currently minimal at best. When a third party component registers itself using the old bp_core_add_root_component() method, a new bp-pages page is created, if necessary. But after that happens, there is no way to change this page association.

The attached patch has a potential solution. It has a few parts:

  • First, components that need top-level directories now must register themselves in the following way:

$bp->component->has_directory = true;
I've modified the BP_Core_Component class, and the various loader classes, accordingly. Components that register themselves in the new way, by extending BP_Core_Component, should be able to imitate this in a straightforward way.

  • Second, I've added a bit of logic to bp_core_add_root_component() that will allow old-style components to (a) be registered in the loaded_components array, and (b) be set to has_directory.
  • Third, I've changed the way that Dashboard > Pages displays components. Instead of hardcoding them, I'm loading them dynamically out of the $bp global: components where has_directory is true get a row on this settings page. This allows you to change the bp-pages association for these components. (Try it with something like Achievements.)
  • Fourth, I've changed the logic in bp_core_activation_notice() just a little, so "orphaned" third-party components show up in the activation notice.

I think that this is a good way to promote backward-compatibility with the old method, and it's also a good way to ensure that third-party components are integrated better into our admin UI. I'd love to hear feedback.

Please note: There is a bug that causes pages to be created ad infinitum for components that have no corollary in bp-pages. So if you set a third-party component like Achievements to '-None-' on Dashboard > BP > Pages, a new page will be created the next time you refresh. This is related to #3422, and will have to be fixed at the same time (perhaps we can just keep a null value in the bp-pages array for the third-party component instead of removing it altogether, or something like that).

IMO this *must* be fixed for 1.5 release.

Attachments (3)

3428.01.patch (8.4 KB) - added by boonebgorges 8 years ago.
3428.02.patch (8.3 KB) - added by boonebgorges 8 years ago.
plugin-page-admin.png (83.1 KB) - added by modemlooper 8 years ago.
I think for UX reasons plugins need the same type of page creation as core.

Download all attachments as: .zip

Change History (25)

#1 @boonebgorges
8 years ago

Another note: 3428.01.patch also includes the fix from 3422.01.patch.

#2 @modemlooper
8 years ago

Ok!

I reinstalled 1.2 and activated my custom component plugin, Then I upgraded to 1.5 with your patches added. Page created ok. But there is the error notice :

Some of your WordPress pages are linked to BuddyPress Components that are disabled: Mobile. Repair

Click repair and it goes to the pages setup. Nothing to help a user fix error.

So thats one problem with 3rd party components. It's like we need an admin page setup hook for our pages just like the core components.

The page creation loop hell still happens when you delete the page and recreating the page doesn't help. It will not allow you in the admin to create the previous auto created slug.

Last edited 8 years ago by modemlooper (previous) (diff)

#3 @boonebgorges
8 years ago

modemlooper - I think your problem is that you are registering your component wrong. You register it with the buddiphobe slug, but then you setup your global as $bp->buddiphone. The way I've written the code, takes two values have to match. I don't see a way around it.

That means changing your code to
bp_core_add_component( 'buddiphone' );

Make that one change and give it another try.

@modemlooper
8 years ago

I think for UX reasons plugins need the same type of page creation as core.

#4 @modemlooper
8 years ago

Boone, it still does not work even with the suggested code. Once you delete the page it just goes through the loop hell. I've tried 4 different BP plugins that currently use the same method of creating slugs. This is going to be a mess for people upgrading if they delete pages. if you upgrade and do not delete the pages it works fine. Once a deletion occurs you have to delete BP database and start over. I went in and deleted pages created and bp_pages and it did not fix the problem.

#5 @enderandrew
8 years ago

  • Cc enderandrew added

+1 for modemlooper's mock-up.

I also think the reason that people might delete the pages might be the same reason I deleted them, that they don't want them in the nav menu. When BP creates the Register page, it doesn't get added to the nav menu. Not every single page is designed to be viewed from the nav menu. The bp-privacy plugin creates a page that presumably is designed to viewed during maintenance. That doesn't need to be in the nav menu. If they were hidden by default, or if this was an obvious way to hide them, then people wouldn't delete them.

#6 @modemlooper
8 years ago

I agree, creating nav items and associated pages is going to cause people to delete and then they will be freaking out why plugins do not work. This needs to be rethought out. Pages created should not automatically add a nav item. This should be added on the pages admin screen after a plugin is activated. <-- another issue entirely and I think it should be added as a feature request.

#7 @boonebgorges
8 years ago

This is going to be a mess for people upgrading if they delete pages.

Please be assured that I take the issue seriously. That's why these tickets are marked with high priority. If the fix is not working for you, it's not because I think it's OK for pages to be created like this; it's because the fix does not work. It is working correctly on my install, but I will run through it a few more times to see if I can reproduce what you're describing.

Re: your mockup. I agree that the items should have their own items on the Pages page. That's what my patch does. (If you've activated Achievements and don't see an Achievements item on Dashboard > BP > Pages, then either you haven't applied my patch correctly, or my patch is broken.) I think I disagree that plugins should be separated into a Plugins section. Admins don't really care about the distinction between core components and plugin components. It makes the most sense, to me, to list them all in the same list (as my patch is intended to do).

When BP creates the Register page, it doesn't get added to the nav menu.

Right... I think that we have some special code to keep it from happening. It's going to be hard to make that happen retroactively, for existing plugins.

Once a deletion occurs you have to delete BP database and start over. I went in and deleted pages created and bp_pages and it did not fix the problem

In wp_options, delete bp-pages. Then empty your WP trash.

Pages created should not automatically add a nav item.

DJPaul, I'd like to hear your opinion on this. bp_dtheme_main_nav() uses a simple wp_page_menu(). Should we rethink this? modemlooper's suggestion about an additional admin for the nav screen seems interesting - I would like to see a mockup - but it sounds to me like it would probably duplicate, at least in part, WP's Menus admin UI. Maybe we could have a checkbox column on the Pages panel called "Include in nav?" which disappears when someone is using a custom menu.

creating nav items and associated pages is going to cause people to delete and then they will be freaking out why plugins do not work

We need things like tooltips and warnings in the Dashboard that explain to people (a) that pages are required for certain plugins to work, and (b) warns them before deleting those pages, and (c) makes it easy to recover from accidentally deleting pages

Setting aside the issue of infinite-page-creation - which is an acknowledged bug that needs to be fixed but is not part of the design and so is part of a separate conversation - I'm beginning to think that part of the answer here is to shut off auto-page-creation altogether. What that means is that certain plugins, like Achievements, will not work until admins have manually created a WP page. This is not ideal, but we can mitigate the downside by providing an admin nag like the "Some components do not have pages..." we already have for core components.

Last edited 8 years ago by boonebgorges (previous) (diff)

#8 @modemlooper
8 years ago

The main reason to use a separate admin for component pages is because even though BP is using pages for the slug in reality they are not WordPress pages a normal user is accustom to using. I completely agree about shutting off page creation. Plugins can have a notice at top on activation asking user to create the component page links to BuddyPress page admin, they then choose page or create new like core components.

#9 @enderandrew
8 years ago

With this patch, I now see "Gallery" under BP Pages from Brajesh's updated BP-Gallery. I tried installing bp-privacy again, but all it did was spam pages again. I had emptied the page trash before, but I had not removed the information from bp-pages in wp_options. So then I dropped the tables that bp-privacy created, removed the entries from bp-pages and then I decided to re-install bp-privacy again relatively "fresh" with this patch applied.

This time, it didn't spam a bunch of pages. But it says:

"Some of your WordPress pages are linked to BuddyPress Components that are disabled: Privacy, Privacy-policy, Maintenance, Welcome."

The repair button takes me to the BP Pages page, but there are no options for me to specify the Privacy, Privacy-policy, Maintenance and Welcome pages.

It isn't fully working for me, but that may be because I had a broken install initially.

#10 @enderandrew
8 years ago

I installed Achievements, which I had not installed before. It created a page, but it also prompted me with:

"Some of your WordPress pages are linked to BuddyPress Components that are disabled: Achievements."

I hit repair, which took me to BP Pages, and I hit Save to tell it that the newly created Achievements page should be mapped to Achievements. No spamming of pages, and it appears to just work as intended. So it would seem the patch works, assuming you don't already have a broken install of a given plugin from triggering the bug before.

I don't want to necessary spam this thread with cross-talk, but are there DB entries I can remove to properly clean the bp-privacy install and try again to get it working? Are there rows written to the DB when bp_core_add_component() is called? And if I didn't want the Welcome, Maintenance, etc. pages in my nav menu, do I have to fully replace my menu with a custom menu, or is there another option? I'm fairly new to WP and I'm not aware of any traditional means to hide a page from the nav menu.

#11 @modemlooper
8 years ago

@Boone, I want to add that by separating plugins into a separate group it makes better sense because of current wording it states 'directories' and not all plugins will have a directory but will need a component slug created.

Take for example my buddiphone plugin. I need a slug 'mobile' but it will not be a BuddyPress directory like members or group listings.

#12 @boonebgorges
8 years ago

modemlooper - If you don't need a directory, then you don't need to register a root component with bp_core_add_root_component(). That's definitely the case in BP 1.5; I'm pretty sure it's the case in 1.2.x as well.

We'll have to make that clearer in the documentation. As soon as BP 1.5 is out, we'll totally revamp the skeleton component with new best practices.

#13 @modemlooper
8 years ago

It's a BuddyPress component it just doesn't have a directory. It's a landing page but not a list of things per say.

#14 @boonebgorges
8 years ago

Do you need a page at http://example.com/mobile/?
Do you need to have URLs that look like http://example.com/mobile/blah/blah/blah?

If your answer to both of these questions is "no", then you don't need to use bp_core_add_root_component(). The key word here is "root". You can have components that are not root components - eg bp-friends, bp-messages, bp-xprofile. See the inline docs: http://buddypress.trac.wordpress.org/browser/branches/1.2/bp-core.php#L961

#15 @johnjamesjacoby
8 years ago

(In [4920]) Add bp-pages admin interface for all components. Rename bp-pages related functions to match existing nomenclature. Introduce 'has_directory' variable in BP_Component class to assist in pairing page slugs to component directories. Set 'path' variable in components that were missing them. Clean up white space where tabs were used instead of spaces. See #3428. Props boonebgorges.

#16 @enderandrew
8 years ago

Forgive me if this is a stupid question, but even with that latest checkin, would plugins still need to be altered to work properly, ie not use bp_core_add_root_component() unless they need a directory?

#17 @boonebgorges
8 years ago

Forgive me if this is a stupid question

It's not too stupid :)

If plugins were using bp_core_add_root_component() when they didn't want a directory, they were doing it wrong. So yes, if they were doing it wrong, they will need to fix it. However, auto-page-creation has been turned off, so there won't be any big problems if they don't fix it - there will just be an erroneous admin message on the Dashboard saying that "You need a page to be set up for this component..."

#18 @enderandrew
8 years ago

Thanks for the clarification. One more question. I'm trying to get a particular abandoned plugin working with 1.5. What would be the correct function/method in place of bp_core_add_root_component()?

#19 @modemlooper
8 years ago

@Boone it is a root component because I need /mobile/slug.

I'm making a pseudo api for mobile. So mobile/profile

Last edited 8 years ago by modemlooper (previous) (diff)

#20 @boonebgorges
8 years ago

modemlooper - OK, that makes sense. That's a pretty unorthodox use case (where you need the URL for an API but you don't want the page to *render*, much less to appear in the nav menu).

There will be an easier way to do this easily in 1.5. I'll be writing a blog post on bpdevel this weekend with some strategies for the new has_directory, root_components, and edge cases like buddiphone.

#21 @modemlooper
8 years ago

Confirmed fixed. User set up is better with the pages not getting auto created. Whats really cool is a user can pick the title of the component slug by choosing a page title.

#22 @boonebgorges
8 years ago

  • Resolution set to fixed
  • Status changed from new to closed

Thanks for the feedback and your help, modemlooper. Closing this ticket. Further requests for enhancements or fixes can go in more specific tickets.

Note: See TracTickets for help on using tickets.