Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(http): Use Symfony to parse PATH_INFO #10618

Merged
merged 1 commit into from
Apr 5, 2017
Merged

Conversation

mrclay
Copy link
Member

@mrclay mrclay commented Nov 22, 2016

Instead of having Apache rewrite the path into a query string argument, we use Symfony's battle-tested Request::getPathInfo.

Fixes #10608

}
}
$req = Request::createFromGlobals();
$_GET[self::GET_PATH_KEY] = $req->getPathInfo();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming here that Request::createFromGlobals handles CLI server, it looks like it should.

@mrclay mrclay force-pushed the 10608_path_info branch 5 times, most recently from 6225130 to 3209cf4 Compare November 22, 2016 21:54
@mrclay mrclay changed the title WIP fix(http): Use Symfony to parse PATH_INFO fix(http): Use Symfony to parse PATH_INFO Nov 22, 2016
@mrclay
Copy link
Member Author

mrclay commented Nov 22, 2016

#10609 should go in 2.3, I'll move this to master.

@mrclay mrclay changed the base branch from 2.3 to master November 22, 2016 23:31
@mrclay mrclay changed the title fix(http): Use Symfony to parse PATH_INFO WIP fix(http): Use Symfony to parse PATH_INFO Dec 24, 2016
@mrclay mrclay force-pushed the 10608_path_info branch 2 times, most recently from f2ed222 to ccb5101 Compare December 29, 2016 18:06
@hypeJunction
Copy link
Contributor

Anything holding this back?

@mrclay mrclay changed the title WIP fix(http): Use Symfony to parse PATH_INFO fix(http): Use Symfony to parse PATH_INFO Jan 13, 2017
@@ -76,6 +76,5 @@ server {
include /etc/nginx/fastcgi_params;
fastcgi_param SCRIPT_FILENAME $document_root/index.php;
fastcgi_param SCRIPT_NAME /index.php;
fastcgi_param QUERY_STRING __elgg_uri=$uri&$args;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not tested this removal

@mrclay
Copy link
Member Author

mrclay commented Jan 13, 2017

Anyone else wanna test it?

@hypeJunction
Copy link
Contributor

We should add Travis jobs for both apache and ngnix perhaps

@mrclay
Copy link
Member Author

mrclay commented Apr 3, 2017

@Elgg/core I'm merging this Apr 10 if no objections.

Copy link
Member

@jeabakker jeabakker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if travis passes, i approve

Instead of having Apache rewrite the path into a query string argument,
we use Symfony's battle-tested `Request::getPathInfo`.

Fixes Elgg#10608
@mrclay mrclay merged commit 3874132 into Elgg:master Apr 5, 2017
@mrclay mrclay deleted the 10608_path_info branch April 5, 2017 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants