Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fixed absolute url generation for query strings and hash urls#23261
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
stof commentedJun 22, 2017
would be great to check if the Link URL resolution in DomCrawler suffers from the same issue |
14399e5 to4d6ad3fComparealexander-schranz commentedJun 22, 2017
@stoff Looks good there but seems we need to handle |
4d6ad3f tob85fb9aCompareb85fb9a to5d12f0cComparealexander-schranz commentedJun 22, 2017
will change the base to 2.7 think thats the oldest supported version? |
5d12f0c to508d9a4Comparealexander-schranz commentedJun 22, 2017
I'm not sure why php7 keeps failing on travis |
| $path =$this->requestContext->getPathInfo().($queryString ?'?'.$queryString :'').$path; | ||
| } | ||
| if ('?' ===$path[0]) { |
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.
We can useelseif here and also below?
| $path =$request->getRequestUri().$path; | ||
| } | ||
| if ('?' ===$path[0]) { |
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.
alsoelseif?
| array('http://localhost/foo/bar#baz','#baz','/foo/bar'), | ||
| array('http://localhost/foo/bar?baz=1#baz','?baz=1#baz','/foo/bar?foo=1'), | ||
| array('http://localhost/foo/baz?baz=1#baz','baz?baz=1#baz','/foo/bar?foo=1'), | ||
| ); |
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.
can you add this test case?
array('http://localhost/foo/bar?0#foo','#foo','/foo/bar?0'),
I think your current solution will strip out the existing?0 query string. I know its an edge case but still 😄
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.
test added seems not failing as a0 as string is true.
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.
php -r"var_dump((bool)'0');"==>bool(false)
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.
@dmaicher can you tell me whats wrong with the test as it should fail.
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.
Ah this is only executed when no current request is given (CLI) and the request context is used instead 😉
So your changes from L 73-79 are currently not covered by tests. You need to add some more test cases into thegetGenerateAbsoluteUrlRequestContextData provider 😉
Tobion commentedJul 5, 2017
👍 |
fabpot commentedJul 6, 2017
Thank you@alexander-schranz. |
…rls (alexander-schranz)This PR was squashed before being merged into the 2.7 branch (closes#23261).Discussion----------Fixed absolute url generation for query strings and hash urls| Q | A| ------------- | ---| Branch? | 2.7, ...| Bug fix? | yes| New feature? |no| BC breaks? | yes? absolute_url will change its output but the old was incorrect| Deprecations? |no| Tests pass? | yes?| Fixed tickets |fixes#23260| License | MITFixed absolute url generation for query stringsCommits-------89ad27d Fixed absolute url generation for query strings and hash urls
Uh oh!
There was an error while loading.Please reload this page.
Fixed absolute url generation for query strings