Apache mod_rewrite have proven to be unreliable, as it does some decoding and reencoding of the URL paths. Trying to match Apache's encoding/decoding rules with PHPs one as proven difficult, and we are still using the ugly drupal_urlencode() to double encode some characters of the path.

Let's not depend on Apache / PHP for this, and derive paths from the URL ourselves.

This patch:

- adds path-deriving logic to drupal_initialize_variables()
- removes drupal_urlencode()
- implements drupal_path_encode(), that is specialized in encoding the path part of the URL
- implements a throughout test case

CommentFileSizeAuthor
#7 484554-conf.patch602 bytesandypost
#1 484554-stop-relying-on-apache.patch13.3 KBdamien tournoud

Comments

damien tournoud’s picture

damien tournoud commented
Status: Active » Needs review
StatusFileSize
new 484554-stop-relying-on-apache.patch13.3 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

pwolanin commented

I think it might be nicer if you do something like:

+  RewriteRule ^(.*)$ index.php?applied_rewrite_rule=1 [L,QSA]

Also, I think you might want 

$_SERVER['PHP_SELF']

"The filename of the currently executing script, relative to the document root." , instead of

$_SERVER['SCRIPT_NAME']

"Contains the current script's path."

damien tournoud’s picture

damien tournoud commented

I think it might be nicer if you do something like:

+ RewriteRule ^(.*)$ index.php?applied_rewrite_rule=1 [L,QSA]

I don't really see the point. It is much simpler to simple derive $_GET['q'] from the requested URI if it is not set.

Also, I think you might want
$_SERVER['PHP_SELF']

"The filename of the currently executing script, relative to the document root." , instead of
$_SERVER['SCRIPT_NAME']

"Contains the current script's path."

That was my starting point, until I saw:

// $_SERVER['SCRIPT_NAME'] can't, in contrast to $_SERVER['PHP_SELF'],
// be modified by a visitor.

The code that I use for that is simply moved from something that was already there in conf_init().

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Status: Needs work » Needs review

#1: 484554-stop-relying-on-apache.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 484554-stop-relying-on-apache.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
StatusFileSize
new 484554-conf.patch602 bytes

Related #1055856-14: Optimize settings.php discovery

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

http://www.php.net/manual/en/reserved.variables.server.php states:

'SCRIPT_FILENAME'
The absolute pathname of the currently executing script.

Note:

If a script is executed with the CLI, as a relative path, such as file.php or ../file.php, $_SERVER['SCRIPT_FILENAME'] will contain the relative path specified by the user.

'SCRIPT_NAME'
Contains the current script's path. This is useful for pages which need to point to themselves. The __FILE__ constant contains the full path and filename of the current (i.e. included) file.

---
Is this issue still relevant with these other D8 issues?

#1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support
#1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel

valthebald’s picture

valthebald
Location Sofia
commented

I second to @sun's question, is this still relevant?

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Closed (duplicate)

Something very similar went into Drupal 7 in a separate issue. So no, this is not relevant by a long shot.