bp_core_referrer() not returning leading slash

[mechter]

Background:

bp_core_set_uri_globals() distinguishes between ajax and non-ajax calls like so:

if ( defined( 'DOING_AJAX' ) && DOING_AJAX || strpos( $_SERVER['REQUEST_URI'], 'wp-load.php' ) )
	$path = bp_core_referrer();
else
	$path = esc_url( $_SERVER['REQUEST_URI'] );

An ajax call will leave $path without leading slash, whereas a non-ajax call results in a leading slash.

It is important that this be streamlined because the bp_uri filter is applied to path right after the distinction and plugins/themes may assume that a path always has a leading slash only to discover that some ajax activity is mysteriously broken. (I know because this is what happened to me)

Suggestion:

bp_core_referrer() should return a leading slash, indicating an absolute path.

[mechter]

The patch should not cause any trouble.

• bp_core_referrer() is called from only one place in trunk (bp_core_set_uri_globals()) and its return value is handled by exactly the same code that is handling the non-ajax leadling-slashed path $_SERVER['REQUEST_URI'].
• All plugins using the following bp_uri hook must also already handle the properly slashed path in non-ajax scenarios.
• The path is generated using explode() and implode() with '/' as delimiter, thus the resulting string never contains a leading slash and it is save to simply prepend it.

However, I have not yet checked whether it breaks unit tests.

[boonebgorges]

Thanks for the report, mechter. Good catch. I agree that this is a bug, and to be honest it renders the bp_core_referrer() function next to useless. That being said, the function has been around since the very earliest days of BuddyPress, so it's likely that someone out there is using it for some purpose, and expects *not* to have the leading slash. In the interest of not breaking their sites, I'm going to suggest that we add the leading slash right in bp_core_set_uri_globals():

if ( defined( 'DOING_AJAX' ) && DOING_AJAX || strpos( $_SERVER['REQUEST_URI'], 'wp-load.php' ) )
	$path = '/' . bp_core_referrer();
else
	$path = esc_url( $_SERVER['REQUEST_URI'] );

What do you think?

[mechter]

That would fix the issue but leave the code buggy. Perhaps we could add bp_core_referrer_path() which correctly returns the leading slash and deprecate bp_core_referrer()? The new name would reflect that a path is being returned rather than complete referrer information.

[boonebgorges]

Replying to mechter:

That would fix the issue but leave the code buggy. Perhaps we could add bp_core_referrer_path() which correctly returns the leading slash and deprecate bp_core_referrer()? The new name would reflect that a path is being returned rather than complete referrer information.

I like the idea of deprecating the function, as developers should probably be warned not to rely on it. I'm on the fence about having a function to replace it - I'm not crazy about having one-use functions like this, especially when the use case (wanting a schemeless version of the referer) is so specific to bp_core_set_uri_globals(). That being said, I would like to be able to write unit tests for it, which suggests that a separate function would be best. Thanks for weighing in.

[mechter]

We could extract the code from bp_core_referrer() into bp_core_set_uri_globals(), fix the leading slash there, deprecate bp_core_referrer() and unit test bp_core_set_uri_globals(). We would have some duplicate code for a while. How many major/minor versions are deprecated functions kept in core?

[boonebgorges]

In 9559:

Improve AJAX referer determination during URI parsing.

When parsing referer URLs during bp_core_set_uri_globals(), BP has
historically used bp_core_referrer() to generate a "current URL" relative to
the current web root. This path is then passed to the 'bp_uri' filter before
being parsed. However, bp_core_referrer() incorrectly returns a URL without
a leading slash, making it a relative path rather than a webroot-absolute path.
The parsing logic later in bp_core_set_uri_globals() makes it so that the
error does not matter from the point of BP core, but plugins filtering 'bp_uri'
will receive a potentially incorrect URL path.

This changeset deprecates the unreliable bp_core_referrer() in favor of
bp_get_referer_path(). The latter function correctly returns URL paths with a
leading slash. bp_get_referer_path() is then used instead of
bp_core_referrer() in bp_core_set_uri_globals().

Props mechter for an initial patch.
Fixes #6252.

[mechter]

You are about to introduce a typo, as bp_get_referer_path() is missing an 'r'.

[boonebgorges]

Replying to mechter:

We could extract the code from bp_core_referrer() into bp_core_set_uri_globals(), fix the leading slash there, deprecate bp_core_referrer() and unit test bp_core_set_uri_globals(). We would have some duplicate code for a while. How many major/minor versions are deprecated functions kept in core?

Whoops, committed over the top of you there. bp_core_set_uri_globals() is a behemoth and nearly impossible to test accurately, especially for something like this that doesn't manifest itself in core behavior. I've gone ahead and added bp_get_referer_path() (function name chosen to parallel wp_get_referer() better) with appropriate unit tests. Deprecated functions are kept around forever, and are loaded by default - they can be toggled with the BP_IGNORE_DEPRECATED constant.

Thanks for your help working through this one.

[boonebgorges]

Regarding the spelling: ​https://en.wikipedia.org/wiki/HTTP_referer#Origin_of_the_term_referer

[mechter]

No worries and you're welcome. Thanks for reacting and fixing this so quickly.

I have just searched the web for 'referer' myself and found that same Wikipedia page. That is truly an amusing piece of Internet trivia. :D

[mechter]

Regarding the new function, I have played out lots of fixes in my mind and each one is a compromise. One more unit test or one less function, in the end I don't think it matters that much in this particular case.

Thanks for the heads-up on deprecation. It's funny how bugs stay around long after they have been fixed. I'll probably have the workaround in my plugin for a long time as to not lose backwards compatibility with BP < 2.3.