Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 4 years ago

#6892 closed defect (bug) (fixed)

Email taxonomy registered on wrong site in multisite

Reported by: needle Owned by: djpaul
Milestone: 2.5 Priority: normal
Severity: normal Version:
Component: Core Keywords: dev-feedback
Cc:

Description

This appears to be an edge case in the upgrade process: when I updated BP via SVN, it just so happened that the first page load thereafter was on a sub-site. This caused the 'bp-email' CPT and "BuddyPress email types" taxonomy to be installed on that sub-site, apparently because bp_update_to_2_5() ran on that site rather than the main site. The CPT and taxonomy are now orphaned on that site and are not removed via the "Reinstall emails (delete and restore from defaults)" tool in Network Admin.

Attachments (6)

6892.patch (791 bytes) - added by imath 4 years ago.
6892.02.patch (884 bytes) - added by imath 4 years ago.
6892.diff (1.1 KB) - added by boonebgorges 4 years ago.
6892.03.patch (3.9 KB) - added by imath 4 years ago.
6892.04.patch (1005 bytes) - added by DJPaul 4 years ago.
6892.05.patch (836 bytes) - added by DJPaul 4 years ago.

Download all attachments as: .zip

Change History (26)

#1 @needle
4 years ago

To clarify, this when BP is network activated.

#2 @imath
4 years ago

  • Milestone changed from Awaiting Review to 2.5

Thanks for your feedback @needle I confirm and was able to reproduce by switching branches. All emails were created on the subsite

switching to blog seems to create the emails on the right site, but the email taxo is not created.. @DJPaul what about using bp_set_object_terms() instead of wp_set_post_terms() in bp_core_install_emails() ?

Attached patch needs to be improve to also fix the taxo trouble.

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

@imath
4 years ago

#3 @DJPaul
4 years ago

I would prefer to switch blogs in a higher-level function. We might conceivably have this problem in the future with other content in the future. Maybe we should consider doing a switch_to_blog for the entire installation process. ?

#4 follow-up: @imath
4 years ago

@DJPaul yes probably.

But i'm *very* concerned that on my multisite install (WP Trunk), when BuddyPress is network activated, no terms are created, no relationships are created between the emails and the terms...

On this multisite install, if i activate BuddyPress on the root blog only or when i activate BuddyPress on a regular site i have terms added and the relationship between the email and term is created...

#5 in reply to: ↑ 4 @DJPaul
4 years ago

Replying to imath:

@DJPaul yes probably.

But i'm *very* concerned that on my multisite install (WP Trunk), when BuddyPress is network activated, no terms are created, no relationships are created between the emails and the terms...

On this multisite install, if i activate BuddyPress on the root blog only or when i activate BuddyPress on a regular site i have terms added and the relationship between the email and term is created...

Yes, that's dealt with by #6891. This ticket is about where the posts are added.

@imath
4 years ago

#6 @imath
4 years ago

6892.02.patch is switching blog inside bp_version_updater().

The patch on #6891 is fixing the taxonomy issue when the upgrade happens when on the root blog or in the network admin.

But if we are in the edge case Christian shared here: being in a subsite when upgrading, the terms are not created. Maybe the blog hasn't switched yet when the hook bp_register_taxonomies is fired.

Should we also switch to blog inside bp_register_default_taxonomies() ?? or do something like the following snippet inside bp_core_install_emails() ?

if ( ! taxonomy_exists( bp_get_email_tax_type() ) ) {
   do_action( 'bp_register_taxonomies' );
}

#7 @DJPaul
4 years ago

  • Keywords dev-feedback added

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


4 years ago

#9 @boonebgorges
4 years ago

This is going to be a problem whenever we depend on post types, taxonomies, post status, etc that are registered only on the root blog. It's likely that a similar bug could be reproduced with Member Types, for example, under the right circumstances. See https://core.trac.wordpress.org/ticket/20541.

In addition to whatever we might do during the upgrade routine, it may be safest to introduce some additional logic into our low-level functions that ensures that taxonomies and post types are registered during switch_to_blog(). See 6892.diff.

@boonebgorges
4 years ago

#10 @boonebgorges
4 years ago

(And in fact, to make sure that the switched environment is always predictable, we might consider a bp_switch_to_root_blog() wrapper that handle the reregistration of content types.)

#11 @imath
4 years ago

I like @boonebgorges 's idea.

The only trouble is, in the case of emails, it would require some extra wrapper functions, as bp_core_install_emails() is not using one of the "BP Taxonomy functions" but wp_set_post_terms().

So ideally we'd probably need something like bp_insert_post_type( $post_type, $args ), bp_set_post_terms(), bp_get_post_terms() ... in which we would switch/reset blog.

That's, in 03.patch why i've added the switch/reset inside the bp_core_install_emails() as it's used on install or upgrade, and it's the first and only place we need it so far.

Oops, i forgot to mention 03.patch is fixing this bug :)

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

@imath
4 years ago

#12 @DJPaul
4 years ago

I am reluctant to introduce a helper function for switch_to_blog that registers post types and taxonomies this late into the cycle. The idea seems a bit crazy to me, and maybe over-engineering at this point. But maybe WordPress is just crazy.

#13 @DJPaul
4 years ago

Also, for the specific instance of installing the emails, bp_set_object_terms isn't adequate because I have to make a call to wp_update_term to update each term's descriptions. Trying to create wrappers for every single taxonomy function in WordPress seems like a massive amount of extra code.

@DJPaul
4 years ago

#14 @DJPaul
4 years ago

Boone's 6892.diff patch is a worthwhile improvement to commit regardless of anything else. However, I've iterated on it in 6892.04.patch to remove the calls to bp_register_post_types() because WP doesn't check if a post type is registered before you use it (at least it doesn't in wp_insert_post and in WP_Query).

#15 follow-up: @DJPaul
4 years ago

I like the combination of the following patches, which is my suggestion:

I think this the lightest way to resolve the current blocker issues, and if there is interest, we can move the discussion about a BP switch_to_blog wrapper and post/taxonomy wrapper functions to another ticket for a future release.

Last edited 4 years ago by DJPaul (previous) (diff)

#16 in reply to: ↑ 15 @boonebgorges
4 years ago

Replying to DJPaul:

I like the combination of the following patches, which is my suggestion:

I think this the lightest way to resolve the current blocker issues, and if there is interest, we can move the discussion about a BP switch_to_blog wrapper and post/taxonomy wrapper functions to another ticket for a future release.

+1 to all of this. I think we *do* need wrappers, crazy as it may be, but it doesn't need to be done for 2.5.

#17 @imath
4 years ago

I like results!

6892.03.patch + https://buddypress.trac.wordpress.org/attachment/ticket/6891/6891.01.patch is fixing this ticket edge case: running the upgrade routine while on a subblog.

if @DJPaul's combination is also fixing this ticket, i'm fine with it :)

#18 @djpaul
4 years ago

In 10580:

Core: in BP taxonomy functions, make sure taxonomies are registered.

These functions already use switch_to_blog to ensure we are operating on the root blog context, but as WordPress doesn't update its internal list of registered taxonomies when this happen, core functions that use taxonomy_exists will fail.

By registering our taxonomies if we have to switch blog context, we prevent failures that could otherwise occur in a handful of multisite configurations.

See #6892

Props boonebgorges

#19 @DJPaul
4 years ago

Upon further consideration, I think we need the extra taxonomy registration check inside a switch_to_blog() in bp_version_updater(), so I've done that in 6892.05.patch which I think is good but I need to test it properly after work before committing.

i.e. like 6892.03.patch but, for the time being, a much lighter change without the proposed shared function.

Last edited 4 years ago by DJPaul (previous) (diff)

@DJPaul
4 years ago

#20 @djpaul
4 years ago

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

In 10582:

Core: be on root blog when installing, and register taxonomies.

Similar to r10580, we needed to make sure the email type taxonomy is
registered when we switch_to_blog to install BuddyPress.

Fixes #6892

Props boonebgorges, DJPaul, imath

Note: See TracTickets for help on using tickets.