Include BP Edit Group Slug into the core

[slaFFik]

I believe (and also have lots of requests about this) that ​BP Edit Group Slug should be in the core of BuddyPress.

[boonebgorges]

The plugin's functionality can't simply be included as-is. Changing a slug means breaking backward compatibility with existing links. So, we'd probably need to do one (probably both) of the following on slug change:

• Store a history of slug changes for each group, and build some legacy-slug-checking logic into our router. We could look at how WP handles changed post slugs for a model.
• Search and replace across the primary_link and action columns of the activity table (and maybe the content?)

[r-a-y]

Changing a slug means breaking backward compatibility with existing links.

I recently did something for username change redirection. This is what I did:

• I hooked on the bp_do_404 action and detected if we're trying to load an invalid group page. Note: This highly relied on the $bp_unfiltered_uri global, which would probably break when moving to rewrite rules.
• Next, I grabbed an option containing group slug changes. eg. bp_get_option( 'bp_group_slug_changes'). Result is an array that looks like this - [oldslug] => GROUP_ID.
• If the old slug matches a key in the array, do a str_replace() in the requested URL with the new group slug and redirect. This would handle the primary_link and action problem without having to alter anything in the DB.

To me, this is the most economical way of doing redirections because we do it when we absolutely need to (during a 404) and without having to tack on a rewrite rule in WP.

The only problem I see is using bp_get_option(). If there are a ton of group slug changes, then this isn't the best storage model. Could probably use groupmeta, but the SELECT meta_value lookup would suck. However, since redirections would probably be done infrequently, it might be okay to use groupmeta.

[boonebgorges]

Cool - thanks for sharing, r-a-y. This is really helpful.

However, since redirections would probably be done infrequently, it might be okay to use groupmeta.

Yes, this. Won't perform super well, but like you say, it's fairly uncommon.

I think we can do something a bit simpler for groups than what you've done for members. Member lookup is done during the parse_uri() routine. But group lookup is done well afterward, during BP_Groups_Component::setup_globals(). So we just do a check for group_exists(), and if it fails, try to look for an old group slug in groupmeta, then redirect. ​https://buddypress.trac.wordpress.org/browser/tags/2.1.1/src/bp-groups/bp-groups-loader.php#L177

Of course, it would be nice to move this stuff - which is, essentially, "canonical" logic - into a centralized place.

[slaFFik]

Boone, what do you mean by "move to a centralized place"? Are you talking to put this somewhere in bp-core component as a separate logic that will handle groups, members etc? Or are you referring to something like bp-groups-redirects.php?

[boonebgorges]

Boone, what do you mean by "move to a centralized place"?

I'm not totally sure :) Currently we do our URL parsing in a couple places: in bp_core_set_uri_globals(), in bp_redirect_canonical(), in setup_globals() for various components. It would be valuable to go through and make a map of how this currently works, and to think about where new "canonical" logic should go - and if the existing stuff should be centralized (say, in a single 'bp_canonical_url' function, which all components could filter?) Suggestions welcome.

[DJPaul]

I like the idea of building group slug editing into the wp-admin/groups dashboard, but less convinced that it should go in the user-side frontend of BuddyPress. If the edit box can be added into one of the existing groups admin screens somewhere, that's fine. If it involves adding a brand new tab to the subnav, I like the idea less.

Replying to boonebgorges:

But group lookup is done well afterward, during BP_Groups_Component::setup_globals(). So we just do a check for group_exists(), and if it fails, try to look for an old group slug in groupmeta, then redirect.

Exactly I believe this is very similar to how old Page slugs are handled for WordPress Pages. KISS. :) Discussions about the other stuff can be looked into on another ticket in the future.

[dcavins]

Hi all-

Here's a first patch on allowing editable group slugs. It addresses these points:

• Adds a form field to the group edit screen in wp-admin and logic to save the changes.
• Stores previous slugs in group meta.
• Adds action on bp_groups_setup_globals to see if the current action refers to an old group slug. If yes, reset the current action before the groups component is set up, so the setup will refer to the correct group.
• Adds activity item logic for group details being changed.

• Changes groups_edit_base_group_details() to accept an array of arguments.
• Tests for new behaviors.

We could make this available in the front end too, via the group manage > details tab. I've added the save logic and the activity item creation logic, but not the form, because I'm not sure we want to go there. If yes, would we want this to be restricted to site admins or group admins (or probably it would be filterable, but what would the default be)?

Thanks for your comments,

-David

[dcavins]

Add support for changing group slug via wp-admin.

[hnla]

It appears to be 404-ing for me on a bp/wp trunk single site.

Slug changed/saved ok but navigating to the group with new slug fails.

Haven't any useful info to add at this point, DB shows group slug changed but I don't see any group meta saved for that particular group id I am expecting to aren't I?

Ignore groupmeta concern that's saved ok routine deletes/updates correctly.

I've got past the 404 error group resolves using new slug, but I have had the usual trouble with delete / add routines where it's easy to confuse them with the $old == $new.

One thing I have just changed groups_add_groupmeta() to save $group->slug rather than $old_group->slug we are updating with the new group slug rather than $old_group aren't we? Typo?

Previous_slug states what is required so I'm wrong here in changing that meta to new slug, even though I get a resolving url hmmm.

[hnla]

@dcavins Ignorethe comment above it all seems fine now as in your original patch, not sure why I got the original 404 but putting it down to the patch not applying fully, causing me to start editing DB entries etc causing old vs new entry conflicts?

[boonebgorges]

Thanks for working on this, @dcavins! The basic strategy sounds good to me.

A check like groups_get_id(), which will take place on every pageload, should be done carefully. I know you're looking to cache it #7477, but I wonder why it's necessary at all. The check already takes place here ​https://buddypress.trac.wordpress.org/browser/tags/2.8.2/src/bp-groups/classes/class-bp-groups-component.php#L198. Can't we just check for 0?

What's the advantage of breaking this into a hooked function? So that it can be disabled? It feels like it'd be more natural in our "routing" logic, inside BP_Groups_Component::setup_globals(). It also feels icky to be modifying global variables outside of our component classes, since it can be very unpleasant to debug.

If we need orderby=meta_id in group queries, we should add it there rather than implementing by filter. (When you asked me about this earlier, I didn't know it was for a BP patch :) )

[dcavins]

Replying to boonebgorges:
Thanks for taking a look at this patch! I agree with everything you've said.

What's the advantage of breaking this into a hooked function? So that it can be disabled? It feels like it'd be more natural in our "routing" logic, inside BP_Groups_Component::setup_globals(). It also feels icky to be modifying global variables outside of our component classes, since it can be very unpleasant to debug.

Yes, I added it via hook primarily so that someone could unhook it. (I'm not sure why you wouldn't want that behavior though.) I first built it in into BP_Groups_Component::setup_globals() but thought I was being presumptuous. If it's OK with everyone else, I'd rather put it with the rest of the routing logic, too.

If we need orderby=meta_id in group queries, we should add it there rather than implementing by filter. (When you asked me about this earlier, I didn't know it was for a BP patch :) )

Ha ha. Yes, I have another patch where I add that orderby option to get(). Again, it seemed so specific to my problem that I was afraid it wouldn't generally be of interest.

Thanks for the encouragement to move these changes into the foundational places they belong. I'll update the patch. :)

[dcavins]

I'm attaching a fresh version of the patch that moves the routing logic into BP_Groups_Component::setup_globals(). The current behavior is that attempting to visit a URL based on an old slug (like ​http://mysite.org/groups/oldslug/members) causes a 301 redirect to the correct URL (like ​http://mysite.org/groups/currentslug/members). In the case of several groups having had the same previous slug, the group that most recently used that slug wins.

In addition to the points in ​https://buddypress.trac.wordpress.org/ticket/6014#comment:8, this patch also adds support for orderby = meta_id in BP_Groups_Group::get() and a number of new tests.

Please test against the refreshed trunk.

Thanks again for your feedback!

[dcavins]

Add support for changing group slug via wp-admin.

[dcavins]

In 11533:

Add 'meta_id' orderby option to BP_Groups_Group::get().

Add meta_id orderby option to groups_get_groups() and its
underlying function BP_Groups_Group::get(). When searching for groups
by meta value, the found groups may now be ordered by the order in
which the found meta rows appear in the bp_groups_groupmeta table.

See #6014.

[dcavins]

In 11555:

Change signature of groups_edit_base_group_details().

• Changes the signature of groups_edit_base_group_details() to accept

a single array instead of several arguments.

• Adds the capability to update the group slug, with the old slug being

stored as a group meta item.

See #6014.

[dcavins]

In 11556:

Update group update form logic to handle slug changes.

Add error cases for handling form input that may include group slug
updates.

See #6014.

[dcavins]

In 11557:

Update group update notifications to include slug updates.

Add cases for the group slug being updated in the code that creates the
various notifications and activity items alerting group admins that
some group information has been updated.

See #6014.

[dcavins]

In 11558:

Introduce groups_get_id_by_previous_slug().

Introduce groups_get_id_by_previous_slug() function to find groups
that once had the requested slug.

See #6014.

[dcavins]

In 11559:

Redirect group requests to new group slug.

If a user attempts to visit a group using an old slug, redirect her to
the current URI.

Fixes #6014.

[johnjamesjacoby]

I'm going to start the process of deprecating my old BP Group Slugs plugin.

Thanks everyone for working so hard on merging this in.

[dcavins]

@johnjamesjacoby Note that the BP core change doesn't include a way to change the slug from the front end like your plugin does, so that may still be a useful piece for some users, until we decide whether BP should do that or not.

[johnjamesjacoby]

Ahh... OK.

I think with stashing the previous slug in meta, it's probably now safe to expose to users (probably not as dedicated tab like my plugin implemented it.)

[dcavins]

I've proposed that an input be added to the main settings page (Manage > Details), but there were some cold feet about that. :)

Now that the underlying "update" functions support slug changes, it would be pretty straightforward to add.

[dcavins]

Here's a working patch to provide a group slug edit input on the front end details form.

[dcavins]

Adds a front-end input for changing group slugs, accessible only by site admins.

[dcavins]

In 11594:

Add sanitization to groups_check_slug.

The function groups_check_slug() claims to sanitize the slug, but it doesn't. It really should.

See #6014.