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

[HttpFoundation] Fix request uri when it starts with double slashes #29494

Merged
merged 1 commit into from Jan 5, 2019

Conversation

@alquerci
Copy link
Contributor

alquerci commented Dec 6, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29478
License MIT
Doc PR ~

When the REQUEST_URI starts with a slash no need to parse_url(). However to keep the same behaviour regarding the fragment we need to add a logic to remove it. While parse_url() handle all cases itself.

@alquerci alquerci changed the base branch from master to 3.4 Dec 6, 2018

@alquerci alquerci changed the title [HttpFoundation] Fix prepare request uri when the uri is leading with double slashes [HttpFoundation] Fix prepare request uri when it starts with double slashes Dec 6, 2018

@alquerci alquerci force-pushed the alquerci:issue-29478 branch from 8721899 to 113a577 Dec 6, 2018

@alquerci alquerci force-pushed the alquerci:issue-29478 branch from 5bc1b0b to 0ddb310 Dec 7, 2018

@alquerci alquerci changed the title [HttpFoundation] Fix prepare request uri when it starts with double slashes [WIP] [HttpFoundation] Fix prepare request uri when it starts with double slashes Dec 7, 2018

@alquerci alquerci changed the title [WIP] [HttpFoundation] Fix prepare request uri when it starts with double slashes [HttpFoundation] Fix prepare request uri when it starts with double slashes Dec 7, 2018

@alquerci alquerci force-pushed the alquerci:issue-29478 branch 2 times, most recently from bfe7d4c to c2b69ef Dec 8, 2018

@alquerci alquerci changed the title [HttpFoundation] Fix prepare request uri when it starts with double slashes [HttpFoundation] Fix request uri when it starts with double slashes Dec 8, 2018

@alquerci alquerci force-pushed the alquerci:issue-29478 branch 2 times, most recently from efe0e35 to 99151e4 Dec 8, 2018

@chalasr chalasr added this to the 3.4 milestone Dec 9, 2018

@alquerci alquerci force-pushed the alquerci:issue-29478 branch 2 times, most recently from 95e9fe0 to 8a7707e Dec 9, 2018

@alquerci alquerci force-pushed the alquerci:issue-29478 branch from 8a7707e to ee1d77f Dec 9, 2018

@alquerci

This comment has been minimized.

Copy link
Contributor

alquerci commented Dec 9, 2018

All code review notes answered by a code modification and the branch has been rebased and squashed.

@danijelk

This comment has been minimized.

Copy link

danijelk commented Dec 12, 2018

Maybe extract the if( empty $requestUri string ) to jump over the if/else, unless you actually want to send an empty string into parse_url()? Just micro optimizations. I tested it with our setup and it worked well.

@alquerci

This comment has been minimized.

Copy link
Contributor

alquerci commented Dec 12, 2018

@danijelk Thank you for your feedback.

On the majority of cases REQUEST_URI starts with a slash. The parse_url() usage is just to handle an edge case.

Your idea is good in order to skip null and empty strings. However, it is a bit overkill regarding the occurrence of this case while parse_url() return an empty path without an huge performance impact.

So to keep the code simple (as we cannot use here an early return) I think that this micro optimization adds useless complexity.

@nicolas-grekas Do you think that we can turn this PR as Reviewed status?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 12, 2018

Is there a way to keep existing tests the same?
When tests change too much it raises a red flag because it means a regression is possible.
Please reduce the diff to the strict minimum to ease the work of reviewers.

@alquerci

This comment has been minimized.

Copy link
Contributor

alquerci commented Dec 12, 2018

Is there a way to keep existing tests the same?

Yes for sure. I can revert them.

@nicolas-grekas FYI: Tests has been refactored on this commit 95e9fe0 then squashed.

Its tests has been added on #29256.

cc @thomasbisignani

@alquerci alquerci force-pushed the alquerci:issue-29478 branch from ee1d77f to 2b97683 Dec 12, 2018

@nicolas-grekas nicolas-grekas referenced this pull request Dec 17, 2018

Closed

Fixes URI parsing #29631

@ro0NL

ro0NL approved these changes Dec 17, 2018

@alquerci alquerci force-pushed the alquerci:issue-29478 branch from 2b97683 to cf850c1 Dec 17, 2018

@Dylan-DutchAndBold
Copy link

Dylan-DutchAndBold left a comment

Isn't this guy's solution where he is just using the parse_url function correctly, much more obvious and readable? a3cbb65

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 18, 2018

@Dylan-DutchAndBold it's not: see how many tests this "solution" breaks.

@Dylan-DutchAndBold

This comment has been minimized.

Copy link

Dylan-DutchAndBold commented Dec 18, 2018

@Dylan-DutchAndBold it's not: see how many tests this "solution" breaks.

You're right, missed that, didn't get what you meant by your original comment at first.

@fabpot

fabpot approved these changes Jan 5, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 5, 2019

Thank you @alquerci.

@fabpot fabpot merged commit cf850c1 into symfony:3.4 Jan 5, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jan 5, 2019

bug #29494 [HttpFoundation] Fix request uri when it starts with doubl…
…e slashes (alquerci)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpFoundation] Fix request uri when it starts with double slashes

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29478
| License       | MIT
| Doc PR        | ~

When the `REQUEST_URI` starts with a slash no need to `parse_url()`. However to keep the same behaviour regarding the fragment we need to add a logic to remove it. While `parse_url()` handle all cases itself.

Commits
-------

cf850c1 [HttpFoundation] Fix request uri when it starts with double slashes

@alquerci alquerci deleted the alquerci:issue-29478 branch Jan 5, 2019

This was referenced Jan 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment