8000 [Routing] Use the default host even if context is empty by sroze · Pull Request #25491 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing] Use the default host even if context is empty #25491

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 3, 2018
Merged

[Routing] Use the default host even if context is empty #25491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Use the default host even if context is empty and fallback to relativ… 
…e URL if empty host
8000
  • Loading branch information
@sroze
sroze committed Jan 3, 2018
commit 8f357df75b7a8b1408db4f912ed4bfbdbe9b59f8
81 changes: 40 additions & 41 deletions src/Symfony/Component/Routing/Generator/UrlGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,63 +181,62 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
}

$schemeAuthority = '';
if ($host = $this->context->getHost()) {
$scheme = $this->context->getScheme();
$host = $this->context->getHost();
$scheme = $this->context->getScheme();
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get what you mean @nicolas-grekas. That's the point of this PR, right? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


if ($requiredSchemes) {
if (!in_array($scheme, $requiredSchemes, true)) {
$referenceType = self::ABSOLUTE_URL;
$scheme = current($requiredSchemes);
}
} elseif (isset($requirements['_scheme']) && ($req = strtolower($requirements['_scheme'])) && $scheme !== $req) {
// We do this for BC; to be removed if _scheme is not supported anymore
if ($requiredSchemes) {
if (!in_array($scheme, $requiredSchemes, true)) {
$referenceType = self::ABSOLUTE_URL;
$scheme = $req;
$scheme = current($requiredSchemes);
}
} elseif (isset($requirements['_scheme']) && ($req = strtolower($requirements['_scheme'])) && $scheme !== $req) {
// We do this for BC; to be removed if _scheme is not supported anymore
$referenceType = self::ABSOLUTE_URL;
$scheme = $req;
}

if ($hostTokens) {
$routeHost = '';
foreach ($hostTokens as $token) {
if ('variable' === $token[0]) {
if (null !== $this->strictRequirements && !preg_match('#^'.$token[2].'$#i', $mergedParams[$token[3]])) {
$message = sprintf('Parameter "%s" for route "%s" must match "%s" ("%s" given) to generate a corresponding URL.', $token[3], $name, $token[2], $mergedParams[$token[3]]);

if ($this->strictRequirements) {
throw new InvalidParameterException($message);
}
if ($hostTokens) {
$routeHost = '';
foreach ($hostTokens as $token) {
if ('variable' === $token[0]) {
if (null !== $this->strictRequirements && !preg_match('#^'.$token[2].'$#i', $mergedParams[$token[3]])) {
$message = sprintf('Parameter "%s" for route "%s" must match "%s" ("%s" given) to generate a corresponding URL.', $token[3], $name, $token[2], $mergedParams[$token[3]]);

if ($this->logger) {
$this->logger->error($message);
}
if ($this->strictRequirements) {
throw new InvalidParameterException($message);
}

return;
if ($this->logger) {
$this->logger->error($message);
}

< 8000 span class='blob-code-inner blob-code-marker js-skip-tagsearch' data-code-marker="-"> $routeHost = $token[1].$mergedParams[$token[3]].$routeHost;
} else {
$routeHost = $token[1].$routeHost;
return;
}
}

if ($routeHost !== $host) {
$host = $routeHost;
if (self::ABSOLUTE_URL !== $referenceType) {
$referenceType = self::NETWORK_PATH;
}
$routeHost = $token[1].$mergedParams[$token[3]].$routeHost;
} else {
$routeHost = $token[1].$routeHost;
}
}

if (self::ABSOLUTE_URL === $referenceType || self::NETWORK_PATH === $referenceType) {
$port = '';
if ('http' === $scheme && 80 != $this->context->getHttpPort()) {
$port = ':'.$this->context->getHttpPort();
} elseif ('https' === $scheme && 443 != $this->context->getHttpsPort()) {
$port = ':'.$this->context->getHttpsPort();
if ($routeHost !== $host) {
$host = $routeHost;
if (self::ABSOLUTE_URL !== $referenceType) {
$referenceType = self::NETWORK_PATH;
}
}
}

$schemeAuthority = self::NETWORK_PATH === $referenceType ? '//' : "$scheme://";
$schemeAuthority .= $host.$port;
if ((self::ABSOLUTE_URL === $referenceType || self::NETWORK_PATH === $referenceType) && !empty($host)) {
$port = '';
if ('http' === $scheme && 80 != $this->context->getHttpPort()) {
$port = ':'.$this->context->getHttpPort();
} elseif ('https' === $scheme && 443 != $this->context->getHttpsPort()) {
$port = ':'.$this->context->getHttpsPort();
}

$schemeAuthority = self::NETWORK_PATH === $referenceType ? '//' : "$scheme://";
$schemeAuthority .= $host.$port;
}

if (self::RELATIVE_PATH === $referenceType) {
Copy link
Member

Choose a reason for hiding this comment

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

|| empty($host)?
or better: set $referenceType relative before L230?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be a different behaviour and the route would be resolved to route instead of /app.php/route

Expand Down
32 changes: 32 additions & 0 deletions src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,38 @@ public function testHostIsCaseInsensitive()
$this->assertSame('//EN.FooBar.com/app.php/', $generator->generate('test', array('locale' => 'EN'), UrlGeneratorInterface::NETWORK_PATH));
}

public function testDefaultHostIsUsedWhenContextHostIsEmpty()
{
$routes = $this->getRoutes('test', new Route('/route', array('domain' => 'my.fallback.host'), array('domain' => '.+'), array(), '{domain}', array('http')));

$generator = $this->getGenerator($routes);
$generator->getContext()->setHost('');

$this->assertSame('http://my.fallback.host/app.php/route', $generator->generate('test', array(), UrlGeneratorInterface::ABSOLUTE_URL));
}

public function testDefaultHostIsUsedWhenContextHostIsEmptyAndSchemeIsNot()
{
$routes = $this->getRoutes('test', new Route('/route', array('domain' => 'my.fallback.host'), array('domain' => '.+'), array(), '{domain}', array('http', 'https')));

$generator = $this->getGenerator($routes);
$generator->getContext()->setHost('');
$generator->getContext()->setScheme('https');

$this->assertSame('https://my.fallback.host/app.php/route', $generator->generate('test', array(), UrlGeneratorInterface::ABSOLUTE_URL));
}

public function testAbsoluteUrlFallbackToRelativeIfHostIsEmptyAndSchemeIsNot()
{
$routes = $this->getRoutes('test', new Route('/route', array(), array(), array(), '', array('http', 'https')));

$generator = $this->getGenerator($routes);
$generator->getContext()->setHost('');
$generator->getContext()->setScheme('https');

$this->assertSame('/app.php/route', $generator->generate('test', array(), UrlGeneratorInterface::ABSOLUTE_URL));
}

/**
* @group legacy
*/
Expand Down
0