WP sets REQUEST_URI to an invalid value if it's not set

[kkmuffme]

​https://github.com/WordPress/wordpress-develop/blame/trunk/src/wp-includes/load.php#L39

This is invalid, since REQUEST_URI is always at least / and cannot be an empty string (you can test it, even for ​https://example.com it will be / - or read ​https://datatracker.ietf.org/doc/html/rfc3986, which shows it's possibly that it can be empty, but in that case PHP would not set the $_SERVER variable, therefore in context of PHP it's impossible it's an empty string for a valid request, unless someone incorrectly modifies it - like WP in this case)

This issue propagates e.g. ​https://github.com/WordPress/wordpress-develop/blame/trunk/src/wp-includes/load.php#L72 which then means we have an impossible "?foo" for example as request URI.

[siliconforks]

Replying to kkmuffme:

​https://github.com/WordPress/wordpress-develop/blame/trunk/src/wp-includes/load.php#L39

I don't really see the issue here. The current code is simply initializing the $_SERVER['REQUEST_URI'] variable to the empty string (if it is not already set). The empty string is not a valid value in this case, but programmers initialize variables to invalid values all the time. For example, a programmer may define a variable $foobar with the line $foobar = null; - by itself, that is not a bug, although it could possibly lead to a bug at some later point in the code.

Is there some scenario where having $_SERVER['REQUEST_URI'] set to the empty string leads to some bug, warning, etc., somewhere else in the code?

This is invalid, since REQUEST_URI is always at least / and cannot be an empty string (you can test it, even for ​https://example.com it will be / - or read ​https://datatracker.ietf.org/doc/html/rfc3986, which shows it's possibly that it can be empty, but in that case PHP would not set the $_SERVER variable, therefore in context of PHP it's impossible it's an empty string for a valid request, unless someone incorrectly modifies it - like WP in this case)

Yes, it's impossible for $_SERVER['REQUEST_URI'] to be an empty string for a valid request, but looking at the ​commit message for the commit above, the change was made in order to handle situations where WordPress is run from the command-line (and so there is no valid request).

Is there a problem with setting $_SERVER['REQUEST_URI'] to the empty string in such cases?

[kkmuffme]

Yes, it's not allowed and it creates issues that are not caught with static analysis, since we typed it as non-falsy-string|null e.g. in psalm.

Practical example is also for caching - the empty string can lead to unexpected behavior

Also 2 examples that will result in a fatal when you pass an empty string:

<?php
explode('', '/some/url', 2);
random_bytes( strlen( '' ) );

The empty string is not a valid value in this case, but programmers initialize variables to invalid values all the time

Can you provide me an example of that in recent code? That used to be the case 15 years ago, but since the advent of strict_types and static analysis, you'll probably find that only in some sweatshop code.

[siliconforks]

It's funny you mention 15 years, since this was initially done almost exactly 15 years ago: [13812]

According to the commit message, the intention was to eliminate the need for isset() checks.

It could be argued that it is better to simply use isset() checks, but it has been like this for so long now that changing it would likely cause issues for existing plugins.

[kkmuffme]

It's funny you mention 15 years, since this was initially done almost exactly 15 years ago

Can you explain me how that's relevant?

According to the commit message, the intention was to eliminate the need for isset() checks.
It could be argued that it is better to simply use isset() checks, but it has been like this for so long now that changing it would likely cause issues for existing plugins.

Did you read the original ticket or look at the PR? Bc it seems you didn't understand any of it.
It's not about removing the declaration, it's changing it to '/' from ''.
This has no implications or changes to if or when isset() is needed.

[siliconforks]

Yes, I saw the PR - that's what motivated me to comment on this issue in the first place: the PR doesn't make any sense. It sets the value of $_SERVER['REQUEST_URI'] to a URL which was never actually requested. I see at least 3 problems with this:

1. Setting $_SERVER['REQUEST_URI'] to / will break any code which checks $_SERVER['REQUEST_URI'] to determine whether WordPress is running from the command-line (e.g., in WP-CLI).

2. The URL / will be nonsensical for WordPress installations that are not in the server document root - e.g., a WordPress installation at http://example.com/blog/. This could confuse any code which is attempting to parse the URL.

3. If WordPress is installed in the server document root, that may be even worse - $_SERVER['REQUEST_URI'] is getting set to a valid URL that was not actually requested. This may result in false positives for any code attempting to identify which URL was requested (for example, consider an analytics plugin which counts requests to each URL - setting $_SERVER['REQUEST_URI'] to / may result in an inflated count for the home page).

[kkmuffme]

the PR doesn't make any sense.

Why/What specifically?

1. Setting $_SERVERREQUEST_URI? to / will break any code which checks $_SERVERREQUEST_URI? to determine whether WordPress is running from the command-line (e.g., in WP-CLI).

Please provide a code reference of WP-CLI please (github link)

There is no sane reason to use REQUEST_URI to check if code is run from the command-line or not, so I doubt that you find any use cases for your claim.

2. The URL / will be nonsensical for WordPress installations that are not in the server document root - e.g., a WordPress installation at ​http://example.com/blog/. This could confuse any code which is attempting to parse the URL.

Please explain how the current empty string is better in that case?

3. If WordPress is installed in the server document root, that may be even worse

Can you please give me an example what you mean specifically?

[siliconforks]

As an example, there are hundreds of plugins in the plugin directory that check whether empty( $_SERVER['REQUEST_URI'] ) is true.

​https://wpdirectory.net/search/01JGA9WXAH1KY0230NBT8N5HV2

I'm not sure exactly what all of these plugins are doing, but they could potentially behave differently when running under WP-CLI if the default value were changed to '/'.

[kkmuffme]

I'm not sure exactly what all of these plugins are doing

Exactly, you have no idea.

Could you please reply each point of your claims you made https://core.trac.wordpress.org/ticket/62355#comment:7 thanks