Skip to:
Content

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#7077 closed enhancement (fixed)

Add a global scope option to the BP taxonomy wrapper functions

Reported by: rekmla Owned by: boonebgorges
Milestone: 2.6 Priority: normal
Severity: normal Version:
Component: Core Keywords: needs-patch
Cc:

Description

In a multi-network WP install, wp_user is shared by all networks. BuddyPress can be configured with global member profile tables. However, member type is implemented as a custom taxonomy and each site with BuddyPress active will have it's own copy of member type term object relationships.

This patch allows a site owner to override the default behavior and store term object relationships in the root blog of the primary network instead of the root blog of the current network. This allows member type to be consistent across all networks.

To turn on global scope for a BP custom taxonomy (bp_member_type) one would add the following function and filter to bp-custom.php

function my_custom_bp_taxonomy_scope( $scope ) {
    return 'global';
}
add_filter( 'bp_taxonomy_bp_member_type_scope', 'my_custom_bp_taxonomy_scope' );

Attachments (4)

7077.patch (5.7 KB) - added by rekmla 2 years ago.
7077.2.patch (1.5 KB) - added by boonebgorges 2 years ago.
7077.3.patch (2.9 KB) - added by rekmla 2 years ago.
7077.4.patch (621 bytes) - added by Offereins 2 years ago.
Fix registration of BP's taxonomies in bp_get_object_terms()

Download all attachments as: .zip

Change History (22)

@rekmla
2 years ago

#1 @boonebgorges
2 years ago

  • Milestone changed from Awaiting Review to Future Release

@rekmla Thanks for the ticket and the patch.

The question of where BuddyPress should store taxonomy data is a legitimate one. Multinetwork + members is the most obvious case, but there are almost certainly other situations where one might legitimately want terms from a given BP taxonomy to be stored on one site, and those from another taxonomy on a different site.

For this reason, the suggested patch seems a bit too specific to the multinetwork use case. It makes your specific use case very easy - a one-line filter - but certain other use cases are difficult.

A more general solution would be to filter the "term storage blog id". See 7077.2.patch. (I only applied the necessary changes to bp_set_object_terms(), but you get the idea.) Your implementation would then look like this:

function bp7077_bp_member_type_should_be_stored_on_primary_network( $site_id, $taxonomy ) {
    if ( 'bp_member_type' === $taxonomy ) {
        $main_network = wp_get_network( get_main_network_id() ); 
 	$site_id = get_site_by_path( $main_network->domain, $main_network->path );
    }

    return $site_id; 
}
add_filter( 'bp_get_taxonomy_term_site_id', 'bp7077_bp_member_type_should_be_stored_on_primary_network', 10, 2 );

I've chosen to pass only the $taxonomy to the filter, because I'd think that in the vast majority of cases, this'll be the determining factor as to what gets stored where. But if it'd be useful, $object_id could be passed as well.

@rekmla What do you think? Also pinging @johnjamesjacoby, who has done a lot of thinking about BP in a multi-network environment.

@boonebgorges
2 years ago

#2 @rekmla
2 years ago

I think with great power comes great responsibility. I was limiting the scope to a strict hierarchy of options, but I like the wide open approach.

We plan on adding wrappers for other taxonomy functions as described in #6028 like wp_insert_term, term_exists, get_terms, etc. We want to provide an admin UI for an academic interests user taxonomy which will be used in user profiles and needs to be stored in the root blog of the primary network since users are global.

#3 @rekmla
2 years ago

I'll add a patch with all 3 taxonomy wrapper functions. The function get_site_by_path returns an object, so I've changed the filter function accordingly.

function bp7077_filter_taxonomy_storage_site( $site_id, $taxonomy ) {
        if ( 'bp_member_type' === $taxonomy ) {
                $main_network = wp_get_network( get_main_network_id() );
                $site = get_site_by_path( $main_network->domain, $main_network->path );
        }

        return $site->site_id;
}
add_filter( 'bp_get_taxonomy_term_site_id', 'bp7077_filter_taxonomy_storage_site', 10, 2 );

@rekmla
2 years ago

#4 @boonebgorges
2 years ago

  • Milestone changed from Future Release to 2.6

Thanks @rekmla ! I think this new approach is more flexible and agnostic about your network setup, which IMO is a big win.

The function get_site_by_path returns an object, so I've changed the filter function accordingly.

Ah yes. Can you tell I wrote it off the top of my head? :)

#5 @boonebgorges
2 years ago

bp_get_object_terms() accepts multiple $taxonomies, and the $taxonomies may in certain cases be stored on different sites. I'll update the patch to accommodate this possibility.

#6 @boonebgorges
2 years ago

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

In 10818:

Taxonomy: Allow the site used for taxonomy term storage to be filtered.

On some setups, it may be desirable to store the terms from a given BP taxonomy
on a site other than the default. For example, on a multi-network setup, where
users are shared between all networks, it may suit the purposes of the
installation better to have bp_member_type data stored on the primary network
and shared between networks.

The new function bp_get_taxonomy_term_site_id() function allows the value to
be filtered, on a taxonomy-specific basis. BP's taxonomy wrapper functions have
been updated to use this new function.

Props rekmla.
Fixes #7077.

@Offereins
2 years ago

Fix registration of BP's taxonomies in bp_get_object_terms()

#7 @Offereins
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

As mentioned in Slack, the bp_register_taxonomies() function now only runs when switching blogs. Since it doesn't run outside of the scope, wp_get_object_terms() consequently returns an invalid_taxonomy error for the current blog: the taxonomy isn't registered.

This can be fixed by moving the registration function outside of the switch block. Patch attached.

Last edited 2 years ago by Offereins (previous) (diff)

#8 @boonebgorges
2 years ago

Thanks very much, @Offereins. The first thing I'm seeing is that [10818] contains a variable typo, specifically in bp_get_object_terms(). I'm going to fix this, and we'll see if that resolves your issue.

#9 @boonebgorges
2 years ago

In 10846:

Fix incorrect variable name after [10818].

See #7077.

#10 @boonebgorges
2 years ago

I'm thinking through this more, and I'm having a hard time imagining why BP would fail to register its taxonomies on the current site, and so would need taxonomy registration as suggested in 7077.4.patch. @Offereins If you can provide any more info about your setup, it'd be very helpful.

I want to be cautious about re-registering taxonomies, as there may be cases where a plugin is doing just-in-time modification of these taxonomies, in such a way that re-registering unnecessarily would cause these changes to be overwritten. It may be better to do something like

$taxonomies_exist = true;
foreach ( $taxonomies as $taxonomy ) {
    if ( ! taxonomy_exists( $taxonomy ) ) {
        $taxonomies_exist = false;
        break;        
    }
}

if ( ! $taxonomies_exist ) {
    bp_register_taxonomies();
}

though even this is not perfect.

My preference would be to fix whatever problem (assuming it's a problem!) that's preventing the taxonomies from being registered in the first place :)

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


2 years ago

#12 @boonebgorges
2 years ago

@Offereins points out privately that the $switched logic for restore_current_blog() is backward.

I will follow up here with a fix for that as well as some conclusions from my discussion with him regarding the taxonomy-registration issue.

#13 @boonebgorges
2 years ago

  • Keywords needs-patch added; has-patch removed

In addition, @Offereins points out that:

Also, I think the new bp_get_taxonomy_term_site_id() should be used in BP_User_Query::get_sql_clause_for_member_types() when switching blogs

This should be verified for group type queries as well.

#14 @boonebgorges
2 years ago

In 10871:

Fix 'switched' logic in taxonomy functions.

Props Offereins.
See #7077.

#15 @boonebgorges
2 years ago

In 10872:

Use bp_get_taxonomy_term_site_id() when switching to generate tax queries for member and group types.

Props Offereins.
See #7077.

#16 @boonebgorges
2 years ago

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

I've spoken with @Offereins about the underlying issue, and I think we agree that it's due to an oddity of his setup. For the moment, let's not change the way taxonomies are registered in these functions. if anything comes up in the future, we'll look into more dynamic stuff (taxonomy_exists()).

#17 @r-a-y
2 years ago

In 10912:

Taxonomy: Fix usage of restore_current_blog() when using bp_get_taxonomy_term_site_id().

See #7077 where we introduced bp_get_taxonomy_term_site_id().

Props remkla.

Fixes #7142, #7144 (2.6-branch).

#18 @r-a-y
2 years ago

In 10913:

Taxonomy: Fix usage of restore_current_blog() when using bp_get_taxonomy_term_site_id().

See #7077 where we introduced bp_get_taxonomy_term_site_id().

Props remkla.

Fixes #7142, #7144 (trunk).

Note: See TracTickets for help on using tickets.