Make WordPress Core

Opened 9 years ago

Last modified 3 years ago

#31603 new enhancement

Don't change $_SERVER['REQUEST_URI'] just to filter the current URL query string

Reported by: morganestes's profile morganestes Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: General Keywords: needs-patch
Focuses: administration Cc:

Description

While working on #23367, I found 14 places in core that overwrite $_SERVER['REQUEST_URI'], which causes problems when trying to use it elsewhere and I don't know which version is going to make an appearance.

I propose either switching from overwriting $_SERVER['REQUEST_URI'] to using a local var when we need to, or creating a filter that's accessible everywhere if we need the modified value to use elsewhere.

Change History (8)

#1 follow-up: @DrewAPicture
9 years ago

  • Keywords reporter-feedback added

@morganestes: Care to add a patch for consideration?

#2 in reply to: ↑ 1 @morganestes
9 years ago

  • Keywords reporter-feedback removed

Replying to DrewAPicture:

@morganestes: Care to add a patch for consideration?

Why yes I would. :) (<- emoji keeps getting stripped here)
I'll start working on it this weekend.

Last edited 9 years ago by morganestes (previous) (diff)

#3 @morganestes
9 years ago

This is a test comment with emoji for meta.

This ticket was mentioned in Slack in #core by morganestes. View the logs.


9 years ago

#5 @johnbillion
9 years ago

@morganestes Can you provide some examples of the places in core which override this and the problem it causes? Just for reference.

#6 @morganestes
9 years ago

@johnbillion I first came across this when working on #23367 and trying to create a canonical admin URL. By the time the admin header fires, $_SERVER['REQUEST_URI'] has already been modified by core several times, most often by remove_query_arg().

For example: in themes.php, it's been modified to remove 'enabled', 'disabled', 'deleted', 'error' from the query string, so when we check for any of those in the query later when we're actually triggering something based on their existence, $_SERVER['REQUEST_URI'] no longer reflects the true value from initial page load. This becomes a problem with things like dismissible or non-repeating notices.

In upload.php it's overwritten multiple times, but not for specific use later in the file.

Nearly every instance of changing the value is due to modifying the query string for displaying or hiding admin messages. The exceptions are for setting values for IIS.

My main concern is that although it's possible for it to change, modifying a server global in multiple places makes it a challenge to know what its value is when used elsewhere and it loses its trustworthiness as a true server value, especially since add_query_arg() and remove_query_arg() default to $_SERVER['REQUEST_URI'] if no URL param is passed.. It should remain a read-only value and any modifications should be to a new variable (even if it's a filterable one).

#7 @johnbillion
9 years ago

  • Keywords needs-patch added
  • Version 4.2 deleted

That's pretty nasty.

#8 @sabernhardt
3 years ago

  • Milestone set to Future Release
Note: See TracTickets for help on using tickets.