Fixed absolute url generation for query strings and hash urls #23261
Conversation
|
would be great to check if the Link URL resolution in DomCrawler suffers from the same issue |
14399e5 to
4d6ad3f
Compare
|
@stoff Looks good there but seems we need to handle |
4d6ad3f to
b85fb9a
Compare
b85fb9a to
5d12f0c
Compare
|
will change the base to 2.7 think thats the oldest supported version? |
5d12f0c to
508d9a4
Compare
|
I'm not sure why php7 keeps failing on travis |
| $path = $this->requestContext->getPathInfo().($queryString ? '?'.$queryString : '').$path; | ||
| } | ||
|
|
||
| 10000 | if ('?' === $path[0]) { |
There was a problem hiding this comment.
We can use elseif here and also below?
| $path = $request->getRequestUri().$path; | ||
| } | ||
|
|
||
| if ('?' === $path[0]) { |
|
|
||
| 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.
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.
test added seems not failing as a 0 as string is true.
There was a problem hiding this comment.
php -r "var_dump((bool)'0');"==> bool(false)
There was a problem hiding this comment.
@dmaicher can you tell me whats wrong with the test as it should fail.
There was a problem hiding this comment.
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 the getGenerateAbsoluteUrlRequestContextData provider 😉
|
👍 |
|
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 | MIT Fixed absolute url generation for query strings Commits ------- 89ad27d Fixed absolute url generation for query strings and hash urls
Fixed absolute url generation for query strings