Skip to:
Content

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#6325 closed enhancement (fixed)

Route /me/*/ to /members/<me>/*/

Reported by: DJPaul Owned by: DJPaul
Milestone: 2.6 Priority: normal
Severity: normal Version:
Component: Route Parser Keywords: dev-feedback has-patch
Cc: espellcaste@…, geminorum.ir@…

Description

It's pretty hard to build nav menus with links to the current user's profile pages, because those links are dynamic based on the currently logged in user.

How about routing /me/*/ to /members/<me>/*/ ? This will allow people to create dynamic links to users' profiles (useful for all sorts of things -- nav menus, support links, etc).

Attachments (5)

profile-universal-redirect.php (1.0 KB) - added by dcavins 2 years ago.
Redirect from /me/ to a user's profile, sending the user through the login screen if needed.
6325.tests.01.patch (1.2 KB) - added by dcavins 2 years ago.
Very basic tests.
6325.02.patch (1.6 KB) - added by r-a-y 2 years ago.
6325.03.patch (3.5 KB) - added by r-a-y 2 years ago.
6325.01.patch (2.0 KB) - added by r-a-y 2 years ago.
Refreshed with 'template_redirect' fix from comment:23 and includes basic unit test.

Download all attachments as: .zip

Change History (37)

#1 @johnjamesjacoby
3 years ago

I am 100% in favor of doing this, and will put whatever effort is necessary towards building it.

#2 @DJPaul
3 years ago

  • Milestone changed from Under Consideration to 2.3

Woo hoo! Let's get it done.

#3 @DJPaul
3 years ago

  • Owner set to DJPaul
  • Status changed from new to assigned

#4 @DJPaul
3 years ago

  • Milestone changed from 2.3 to 2.4

I'll get this done for 2.4.

#5 @DJPaul
3 years ago

I started looking at this but then realised I really want to duplicate a lot of the path cleaning code from the top-half of bp_core_set_uri_globals to handle things like subdirectories and stuff, which is 😢. So I thought about maybe using that function to handle the redirect which is also 😢 because our associated root page code is all 💀 and it's not the right place to put the code. But! Then I remembered we already have special code in there to handle the bp_get_search_slug which itself is handled in bp_core_action_search_site (bp_init, 7).

What do you think about implementing this ticket using this same general pattern?

(For novelty value, here is my in-development effort before I thought of all of the above, and it's real crappy: https://gist.github.com/paulgibbs/b406efa9ca0ca4e6a3f6 )

#6 @DJPaul
3 years ago

  • Keywords dev-feedback added

#7 @DJPaul
3 years ago

  • Milestone changed from 2.4 to 2.5

Would love some feedback to my comment during 2.5 so we can sort something out. :)

#8 @dcavins
2 years ago

I did something similar recently, so I'll attach my working function as another idea. I hooked to bp_init, 9 and used all $_SERVER variables because I'm on an IIS server and they're pretty reliable (when other methods are not).

@dcavins
2 years ago

Redirect from /me/ to a user's profile, sending the user through the login screen if needed.

#9 @DJPaul
2 years ago

  • Milestone changed from 2.5 to 2.6

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


2 years ago

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


2 years ago

#12 @boonebgorges
2 years ago

I'd suggest adding a new do_action() or apply_filters() call someone in the middle of bp_core_set_uri_globals() - after we've parsed and cleaned $bp_uri, but before we've started the component/action calculation. $bp_uri should be passed to this filter/action. Then 'me' redirects would be handled in a callback.

#13 @espellcaste
2 years ago

  • Cc espellcaste@… added

#14 @r-a-y
2 years ago

  • Keywords has-patch added

Attached patch relies on attachment:6694.02.patch:ticket:6694 from #6694, which is about filtering the member slug in bp_core_set_uri_globals().

It's similar to what boonebgorges suggests in comment:12.

This redirects /members/me/* to /members/LOGGEDIN_USER_SLUG/*. If root profiles is enabled, then this becomes /me/* to /LOGGEDIN_USER_SLUG/*.

My only concern is do we do this redirection for root profiles as well (x.com/me/*)? If we do, there is the potential to be a conflict with a page with the slug /me/. It's easy enough to disable this if root profiles is enabled. Just need some dev feedback.

Last edited 2 years ago by r-a-y (previous) (diff)

#15 @dcavins
2 years ago

That's a nice tidy patch @r-a-y.

My only doubt is that this approach somewhat limits the usefulness of/me/* links because they'll only work when the user is logged in. I can imagine links of this style being useful in emails, for instance, like from the site admin: "Hey, update your profile at /me/profile/edit", which would be unreliable if the user has to be already logged-in. (Although a savvy site admin could use your trick of appending the desired link to wp-login.php/?redirect=...--it's just not as straightforward.)

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


2 years ago

#17 @r-a-y
2 years ago

Thanks for the feedback, @dcavins!

02.patch should redirect a user to login if a user is logged out. So, you should be able to user /members/me/* in both logged-in and logged-out scenarios.

#18 @dcavins
2 years ago

Thanks @r-a-y for the new patch. It works well for me. @DJPaul do you have any feedback? This patch is a little different than what you were asking for (members/me/* is the pattern, not /me/*), but I think it makes a lot of sense to do it @r-a-y's way.

I was thinking about adding me to the list of illegal usernames in bp_core_get_illegal_names(), but bp_core_validate_user_signup() requires the username to be four characters long or more, so I guess that rule will cover it.

I wrote up the most basic of tests (attached), and wonder if the logged-out user test is exposing a problem. In the testing environment, if I route a logged-out user to members/me/, I get a headers-already-sent error:

Cannot modify header information - headers already sent by (output started at /tests/phpunit/includes/bootstrap.php:56)

It works fine in my browser, but is the bp_core_set_uri_globals_member_slug hook too late to be starting redirects?

Thanks again for your work on this!

@dcavins
2 years ago

Very basic tests.

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


2 years ago

@r-a-y
2 years ago

#20 @r-a-y
2 years ago

It works fine in my browser, but is the bp_core_set_uri_globals_member_slug hook too late to be starting redirects?

It's slightly too early. That's why the unit tests are complaining ;) We should preferably be doing this redirect on the 'template_redirect' hook.

I've refreshed 02.patch to use an anonymous function to hook onto 'bp_template_redirect' to do the login redirect. This fixes up the unit test since the 'bp_template_redirect' hook isn't run during unit tests.

If we hate anonymous functions, we can introduce a new function for this. Something like:

/**
 * Function to perform a generic no access redirect.
 *
 * This should only be run on the 'template_redirect' hook.
 *
 * @since 2.6.0
 */
function bp_core_do_no_access_redirect() {
        // Bail if we're using this function before the 'template_redirect' hook.
        if ( false === did_action( 'template_redirect' ) ) {
                _doing_it_wrong( __FUNCTION__, __( "This function should only run on the 'template_redirect' hook.", 'buddypress' ), 'BP 2.6.0' );
                return;
        }

        bp_core_no_access();
        exit();
}

I also want to return to this potential issue:

My only concern is do we do this redirection for root profiles as well (x.com/me/*)? If we do, there is the potential to be a conflict with a page with the slug /me/. It's easy enough to disable this if root profiles is enabled. Just need some dev feedback.

I don't think this is really an issue. If there is a conflict, a plugin dev can always filter our special 'me' slug to something else.

FWIW, if root profiles is enabled and there is a username called 'test' and there is a page with the slug test, BP will take precedence.

#21 @dcavins
2 years ago

Thanks @r-a-y. Re anonymous functions, I'll leave that up to the project leads. :)

I wholly agree with you on the issue you returned to. My feeling is that the value of this handy change far outweighs the chance of a conflict with a page named "me," and a dev can always filter the slug.

Thanks!

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


2 years ago

#23 @boonebgorges
2 years ago

Thanks for working on this, @r-a-y! A few questions.

Why do we have to exit after bp_core_no_access()? That function always ends in bp_core_redirect(), which has a die in it. Can't we just do add_action( 'template_redirect', 'bp_core_no_access' )?

Is there precedence for making me a translatable slug? It seems like it would be better UI if French installations got moi out of the box. This would require some sanitization, and there may be problems with character sets, and there might be potential conflicts with usernames in some languages, so maybe a terrible idea, but I wanted to raise it.

#24 @r-a-y
2 years ago

Can't we just do add_action( 'template_redirect', 'bp_core_no_access' )?

Complete oversight on my part. You're absolutely correct!

Is there precedence for making me a translatable slug?

A valid concern. I didn't make it translatable initially for that very reason.

We could probably make it a translatable slug, but use rawurlencode() afterwards to ensure accents and the like will work well when used in the URL. Will do some testing and try and write some unit tests with accents as characters.

#25 @boonebgorges
2 years ago

We could probably make it a translatable slug, but use rawurlencode() afterwards to ensure accents and the like will work well when used in the URL. Will do some testing and try and write some unit tests with accents as characters.

Don't spend too much time on it. It could be introduced later if there was a need for it. If we don't have time to test with different character sets, we shouldn't let it hold up the feature.

#26 @geminorum
2 years ago

  • Cc geminorum.ir@… added

just in case, I'm using this for a long time: https://gist.github.com/geminorum/c22dc0a07c1db6031cdc39d7f76292eb

#27 @dcavins
2 years ago

Hi @geminorum-

Since I don't recognize your username, let me say welcome. Thanks for checking in on this ticket and sharing your gist.

I hope you'll do a lot of ticket reporting and patch writing in the future! :)

-David

@r-a-y
2 years ago

@r-a-y
2 years ago

Refreshed with 'template_redirect' fix from comment:23 and includes basic unit test.

#28 @r-a-y
2 years ago

Don't spend too much time on it. It could be introduced later if there was a need for it. If we don't have time to test with different character sets, we shouldn't let it hold up the feature.

03.patch is an attempt to test for various character encodings and adds some logic to convert the custom slug over to utf-8 (requires iconv(), which might not be available on all installs) before URL encoding to match the $member_slug passed by BuddyPress.

Although my tests pass, I do not feel confident enough to commit it. So let's just commit 01.patch for 2.6.

Last edited 2 years ago by r-a-y (previous) (diff)

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


2 years ago

#30 follow-up: @boonebgorges
2 years ago

So let's just commit 01.patch for 2.6.

Agreed. Let's move your internationalization patch to a separate enhancement ticket.

#31 @r-a-y
2 years ago

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

In 10791:

URI: Support members shortlink redirection.

This commit redirects /members/me/* to /members/LOGGEDIN_USER_SLUG/*.
If root profiles is enabled, then this redirects /me/* to
/LOGGEDIN_USER_SLUG/*. If a user is logged out, /me/ redirects to the
login page and when properly authenticated, will redirect back to the
logged-in user's page.

This will allow devs to create dynamic links to a user's profile, which is
useful for all sorts of things -- nav menus, support links, etc.

Props r-a-y, dcavins.

Fixes #6325.

#32 in reply to: ↑ 30 @r-a-y
2 years ago

Let's move your internationalization patch to a separate enhancement ticket.

Moved to #7075.

Thanks everyone for their feedback on this ticket.

Note: See TracTickets for help on using tickets.