[Routing] Use the default host even if context is empty #25491
Conversation
cd7eadd to
07d6ff3
Compare
| if ($host = $this->context->getHost()) { | ||
| $scheme = $this->context->getScheme(); | ||
| $host = $this->context->getHost(); | ||
| $scheme = $this->context->getScheme(); |
There was a problem hiding this comment.
is it possible to have a non empty scheme with an empty host? if yes, we should review the behavior we want in this situation
There was a problem hiding this comment.
I don't get what you mean @nicolas-grekas. That's the point of this PR, right? 🤔
There was a problem hiding this comment.
the point of this PR is to deal with empty hosts
but it doesn't yet care about non-empty scheme + empty-host.
that's my point.
eg can/should an empty-host url be considered absolute?
looking at the code, it looks like yes after this PR.
does that make sense? that's one of the things I'm wondering.
There was a problem hiding this comment.
I thought about it... and I don't see what else we can consider in this example you gave... We need to assume it is going to be absolute, else we need to throw an exception. I'd go for absolute (to keep the current behaviour) unless a big counter-indication.
07d6ff3 to
0493816
Compare
| $generator->getContext()->setHost(''); | ||
| $generator->getContext()->setScheme('https'); | ||
|
|
||
| $this->assertSame('https:///app.php/route', $generator->generate('test', array(), UrlGeneratorInterface::ABSOLUTE_URL)); |
There was a problem hiding this comment.
@nicolas-grekas this indeed looks weird... should we throw an exception instead?
There was a problem hiding this comment.
or ignore the scheme and make the url relative? we should settle for the most BC.
There was a problem hiding this comment.
Agree with ignore scheme and make URL relative. It's much more BC, you are correct.
0493816 to
0f3ab10
Compare
|
@nicolas-grekas updated to fallback on relative when host is empty. Also squashed and rebased. |
| } | ||
|
|
||
| $schemeAuthority = self::NETWORK_PATH === $referenceType ? '//' : "$scheme://"; | ||
| $schemeAuthority .= $host.$port; | ||
| } | ||
|
|
||
| if (self::RELATIVE_PATH === $referenceType) { |
There was a problem hiding this comment.
|| empty($host)?
or better: set $referenceType relative before L230?
There was a problem hiding this comment.
It would be a different behaviour and the route would be resolved to route instead of /app.php/route
|
Travis' failure is timeout related on one of the jobs... amending & pushing again. |
…e URL if empty host
0f3ab10 to
8f357df
Compare
|
Thank you @sroze. |
…roze) This PR was merged into the 2.7 branch. Discussion ---------- [Routing] Use the default host even if context is empty | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25464 | License | MIT | Doc PR | ø When the host in the context is empty... we should still use the default host. **Note:** it seems like a lot of changes but all I did was to remove the `if` and de-indent the code that was inside. Commits ------- 8f357df Use the default host even if context is empty and fallback to relative URL if empty host
…nown (Tobion) This PR was merged into the 3.4 branch. Discussion ---------- [Routing] fix absolute url generation when scheme is not known | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | #25491 | License | MIT | Doc PR | This fixes two edge cases in the url generator: 1. when the context scheme is not known (empty) generating an absolute url would return an invalid url starting with `://host/path`. #25491 handled the case when the host is unknown which makes sense. but the way it was done, created this new problem. 2. non-http(s) urls do not require a host. e.g. typical `file:///path` urls. url generator is fixed to be in line with rfc3986 Commits ------- 8e04222 [Routing] fix absolute url generation when scheme is not known
When the host in the context is empty... we should still use the default host.
Note: it seems like a lot of changes but all I did was to remove the
ifand de-indent the code that was inside.