Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[HttpFoundation] Ensure path info has a leading slash#40750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| 'SCRIPT_NAME' => '/index.php', | ||
| 'PHP_SELF' => '/index.php', | ||
| ]); | ||
| $this->assertEquals('http://test.com/index.php/.php', $request->getUri()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
that case looks wrong to me though, as that would be a different Uri that the input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What should the uri be though? If you make a request to index.php/something or /something in this situation you'd want the path info to be /something. And it is. I think this is a special edge case because of the way htaccess rules work for Drupal (and probably a number of other projects). The relevant parts of our rule is:
RewriteCond %{REQUEST_FILENAME} !-f RewriteCond %{REQUEST_FILENAME} !-d RewriteCond %{REQUEST_URI} !=/favicon.ico RewriteRule ^ index.php [L]I think we should treat a request to index.phpSOMETHING as a request to SOMETHING - regardless of whether SOMETHING starts with a /... which means we need to duplicate the logic of when there is no base path...
This is just above the code I've added.
if ('' !== $requestUri && '/' !== $requestUri[0]) { $requestUri = '/'.$requestUri; } if (null === ($baseUrl = $this->getBaseUrl())) { return $requestUri; }There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Forindex.php/something, It totally agree. But this isnot what this test is covering.
How would I be able to detect the difference between the following URLs: ? |
This is unaffected as far as I can see. Here's the preceding code which is going to deal with this case... |
Would this fix#37995? |
@xabbuh I was wondering that too but I think the question you asked in#37995 (comment) is key there. I'm not sure how you end up with |
@stof what do I need to do to get this one fixed? |
On a Drupal 8/9 site if you make a request for
/index.php.phpthen this will cause PHP notices. The default .htaccess rule shipped with Drupal does not redirect from/index.phpto/if it did that the path would be/.phpand it would correctly 404. However changing the Drupal .htaccess rule to behave likehttps://github.com/symfony/recipes-contrib/blob/master/symfony/apache-pack/1.0/public/.htaccess suggests is fraught and would probably not fix existing sites.Already in \Symfony\Component\HttpFoundation\Request::preparePathInfo() we ensure that it starts with a leading slash if \Symfony\Component\HttpFoundation\Request::getBaseUrlReal() returns NULL and I think we should do the same if strip the base url from the request URI.
Seehttps://www.drupal.org/project/drupal/issues/3167426 for more information on the bugs this is causing in Drupal.