Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[Routing] added support for hostname requirement to routes #3057

Closed
wants to merge 1 commit into from

5 participants

@gunnarlium

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes (All Routing tests pass, Locale tests fail on my localhost, but I have not touched those files)
Fixes the following tickets: -
Todo:

  • Add documentation if feature is approved
  • Maybe add/test wildcard support

This patch adds support for hostname requirements on routes:

hostbased_route:
  pattern:  /
  requirements:
    _host: symfony.com|symfony.org

This route would only match if RequestContext::getHost() matches symfony.com or symfony.org.

@stof stof commented on the diff
src/Symfony/Component/Routing/Generator/UrlGenerator.php
@@ -159,7 +184,7 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
$port = ':'.$this->context->getHttpsPort();
}
- $url = $scheme.'://'.$this->context->getHost().$port.$url;
+ $url = $scheme.'://'.$host.$port.$url;
@stof Collaborator
stof added a note

this is not enough. If the current host (the one in the context) does not match the requirement, the url should be generated as absolute even if the call to generate did not set the absolute parameter to true

Do you agree that it makes sense that routes with host requirement always are generated as absolute? If so, I believe adding $absolute = true at line 105 should do the trick.

@stof Collaborator
stof added a note

no, they should be generated as absolute only if the host needs to be changed. if the current host already match the requirement, the control should be left to the developer if he doesn't want an absolute url

@stof Collaborator
stof added a note

oups sorry, I missed the line 163 which already implements it

Right, missed it myself too ;)

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

The support should also be added in the Apache dumper

@Koc
Koc commented

looks like duplicate of #3002

@stof
Collaborator

@fabpot ping. Please review these PRs and choose the one you want to keep.

@gunnarlium Please rebase your branch. It conflicts with master because of the move of the tests

@gunnarlium

@stof Rebase done.

@Tobion Tobion commented on the diff
...Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
@@ -212,6 +218,27 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
}
}
+ if ($hostnames) {
+ $gotoname = 'not_'.preg_replace('/[^A-Za-z0-9_]/', '', $name);
@Tobion Collaborator
Tobion added a note

this one is duplicated now. you could use this above the corresponding code generation

if ($methods || $hostnames) {
    $gotoname = 'not_'.preg_replace('/[^A-Za-z0-9_]/', '', $name);
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Tobion Tobion commented on the diff
...Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
((5 lines not shown))
\$pathinfo = urldecode(\$pathinfo);
$code
- throw 0 < count(\$allow) ? new MethodNotAllowedException(array_unique(\$allow)) : new ResourceNotFoundException();
+ throw 0 < count(\$allow) ? new MethodNotAllowedException(array_unique(\$allow)) :
+ 0 < count(\$hosts) ? new HostnameNotAllowedException(array_unique(\$hosts)) :
+ new ResourceNotFoundException();
@Tobion Collaborator
Tobion added a note

missing ( ) around the second condition.

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

Closing this PR as #3378 is a better attempt to solve the problem.

@fabpot fabpot closed this
@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot merged branch arnaud-lb/hostname-routes (PR #3378)
This PR was merged into the master branch.

Commits
-------

17f51a1 Merge pull request #6 from Tobion/hostname-routes
e120a7a fix API of RouteCollection
26e5684 some type fixes
514e27a [Routing] fix PhpMatcherDumper that returned numeric-indexed params that are returned besides named placeholders by preg_match
7ed3013 switch to array_replace instead of array_merge
94ec653 removed irrelevant string case in XmlFileLoader
9ffe3de synchronize the fixtures in different formats and fix default for numeric requirement
6cd3457 fixed CS
8366b8a [Routing] fixed validity check for hostname params in UrlGenerator
a8ce621 [Routing] added support for hostname in the apache matcher dumper
562174a [Routing] fixed indentation of dumped collections
1489021 fixed CS
a270458 [Routing] added some more unit tests
153fcf2 [Routing] added some unit tests for the PHP loader
68da6ad [Routing] added support for hostname in the XML loader
3dfca47 [Routing] added some unit tests for the YAML loader
92f9c15 [Routing] changed CompiledRoute signature to be more consistent
d91e5a2 [Routing] fixed Route annotation for hostname (should be hostname_pattern instead of hostnamePattern)
62de881 [Routing] clarified a variable content
11b4378 [Routing] added hostname support in UrlMatcher
fc015d5 [Routing] fixed route generation with a hostname pattern when the hostname is the same as the current one (no need to force the generated URL to be absolute)
462999d [Routing] display hostname pattern in router:debug output
805806a [Routing] added hostname matching support to UrlGenerator
7a15e00 [Routing] added hostname matching support to AnnotationClassLoader
cab450c [Routing] added hostname matching support to YamlFileLoader
85d11af [Routing] added hostname matching support to PhpMatcherDumper
402359b [Routing] added hostname matching support to RouteCompiler
add3658 [Routing] added hostname matching support to Route and RouteCollection
23feb37 [Routing] added hostname matching support to CompiledRoute

Discussion
----------

[2.2][Routing] hostname pattern for routes

Bug fix: no
Feature addition: yes
Fixes the following tickets: #1762, #3276
Backwards compatibility break: no
Symfony2 tests pass: yes

This adds a hostname_pattern property to routes. It works like the pattern property (hostname_pattern can have variables, requirements, etc). The hostname_pattern property can be set on both routes and route collections.

Yaml example:

``` yaml
# Setting the hostname_pattern for a whole collection of routes

AcmeBundle:
    resource: "@AcmeBundle/Controller/"
    type: annotation
    prefix: /
    hostname_pattern: {locale}.example.com
    requirements:
        locale: en|fr

# Setting the hostname_pattern for single route

some_route:
    pattern: /hello/{name}
    hostname_pattern: {locale}.example.com
    requirements:
        locale: en|fr
        name: \w+
    defaults:
        _controller: Foo:bar:baz
```

Annotations example:

``` php
<?php

/**
 * Inherits requirements and hostname pattern from the collection
 * @Route("/foo")
 */
public function fooAction();

/**
 * Set a specific hostnamePattern for this route only
 * @Route("/foo", hostnamePattern="{_locale}.example.com", requirements={"_locale="fr|en"})
 */
public function fooAction();

```

Performance:

Consecutive routes with the same hostname pattern are grouped, and a single test is made against the hostname for this group, so the overhead is very low:

```
@Route("/foo", hostnamePattern="a.example.com")
@Route("/bar", hostnamePattern="a.example.com")
@Route("/baz", hostnamePattern="b.example.com")
```

is compiled like this:

```
if (hostname matches a.example.com) {
    // test route "/foo"
    // test route "/bar"
}
if (hostname matches b.example.com) {
    // test route "/baz"
}
```

The PR also tries harder to optimize routes sharing the same prefix:

```
@Route("/cafe")
@Route("/cacao")
@Route("/coca")
```

is compiled like this:

```
if (url starts with /c) {
    if (url starts with /ca) {
        // test route "/cafe"
        // test route "/cacao"
    }
    // test route "/coca"
}
```

---------------------------------------------------------------------------

by Koc at 2012-02-16T14:14:19Z

Interesting. Have you looked at #3057, #3002?

Killer feature of #3057 : multiple hostnames per route.

---------------------------------------------------------------------------

by arnaud-lb at 2012-02-16T14:21:28Z

@Koc yes, the main difference is that this PR allows variables in the hostname pattern, with requirements, etc just like the path pattern. The other PRs use a `_host` requirement, which works like the `_method` requirement (takes a list of allowed hostnames separated by `|`).

> Killer feature of #3057 : multiple hostnames per route.

If you have multiple tlds you can easily do it like this:

``` yaml
hostbased_route:
  pattern:  /
  hostname_pattern: symfony.{tld}
  requirements:
     tld: org|com
```

Or with completely different domain names:

``` yaml
hostbased_route:
  pattern:  /
  hostname_pattern: {domain}
  requirements:
     domain: example\.com|symfony\.com
```

Requirements allow DIC %parameters%, so you can also put you domains in your config.yml.

---------------------------------------------------------------------------

by Koc at 2012-02-16T15:52:16Z

wow, nice! So looks like this PR closes my #3276 ticket?

---------------------------------------------------------------------------

by arnaud-lb at 2012-02-16T15:53:55Z

Yes, apparently :)

---------------------------------------------------------------------------

by Koc at 2012-02-16T15:56:53Z

I cann't find method `ParameterBag::resolveValue` calling in this PR, like here https://github.com/symfony/symfony/pull/3316/files

---------------------------------------------------------------------------

by arnaud-lb at 2012-02-16T16:03:48Z

I think it's in core already

---------------------------------------------------------------------------

by Koc at 2012-02-16T16:11:38Z

looks like yes
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Routing/Router.php#L81

---------------------------------------------------------------------------

by dlsniper at 2012-02-16T19:37:57Z

This PR looks great, it's something like this I've been waiting for.

I know @fabpot said he's working on something similar but I think if he agrees with this it could be a great addition to the core.

@fabpot , @stof any objections about this PR if gets fully done?

---------------------------------------------------------------------------

by stof at 2012-02-16T20:00:21Z

Well, we already have 2 other implementations for this stuff in the PRs. @fabpot please take time to look at them

---------------------------------------------------------------------------

by stof at 2012-02-16T20:03:17Z

This one is absolutely not tested and seems to break the existing tests according to the description. So it cannot be reviewed as is.

---------------------------------------------------------------------------

by dlsniper at 2012-02-16T22:00:24Z

@stof I understand it's a WIP but the other PRs where ignored as well and like you've said, there's a bunch of PRs already on this issue all doing a thing or another. So an early feedback on this, or any other, could lead it to the right path in order to finally solve this issue.

---------------------------------------------------------------------------

by arnaud-lb at 2012-02-17T23:57:28Z

Added tests; others are passing now

---------------------------------------------------------------------------

by arnaud-lb at 2012-02-22T21:10:20Z

I'm going to add support for the Apache dumper and the XML loader; does this PR have a chance to be merged ? cc @fabpot @stof

---------------------------------------------------------------------------

by stof at 2012-02-22T22:05:23Z

@arnaud-lb We need to wait @fabpot's mind about the way he prefers to implement it to know which one can be merged.

---------------------------------------------------------------------------

by IjinPL at 2012-02-27T02:01:57Z

Forked @arnaud-lb *hostname_pattern* to add XML parasing support.

---------------------------------------------------------------------------

by stof at 2012-04-03T23:59:12Z

@arnaud-lb Please rebase your branch. It conflicts with master because of the move of the tests

@fabpot @vicb ping

---------------------------------------------------------------------------

by dlsniper at 2012-04-13T19:52:23Z

Hi,

If @arnaud-lb won't be able to rebase this I could help with some work on this but there's still the problem of actually choosing the right PR(s) for this issue. @blogsh says in his last commit that this PR is a bit better in his opinion but @fabpot needs to decide in the end.

---------------------------------------------------------------------------

by arnaud-lb at 2012-04-14T17:26:55Z

@stof rebased

---------------------------------------------------------------------------

by nomack84 at 2012-04-20T13:01:00Z

@fabpot Any final word about this pull request? It would be nice to have this feature ready for 2.1.

---------------------------------------------------------------------------

by asm89 at 2012-04-24T21:27:50Z

Using the `{_locale}` placeholder in the host would set the locale for the request just like it does now?

Another thing I'm wondering is how/if it should be possible to set the hostname pattern for all your routes, or at least when importing routes? Otherwise you'll end up repeating the same host pattern over and over again. I think this is also important when importing routes from third party bundles.

---------------------------------------------------------------------------

by fabpot at 2012-04-25T01:17:51Z

I'm reviewing this PR and I'm going to make some modifications. I will send a PR to @arnaud-lb soon.

---------------------------------------------------------------------------

by fabpot at 2012-04-25T03:10:18Z

I've sent a PR to @arnaud-lb arnaud-lb/symfony#3 that fixes some minor bugs and add support in more classes.

---------------------------------------------------------------------------

by fabpot at 2012-04-25T03:12:52Z

@asm89:

Placeholders in the hostname are managed in the same way as the ones from the URL pattern.

You can set a hostname pattern for a collection (like the prefix for URL patterns).

---------------------------------------------------------------------------

by Tobion at 2012-04-25T09:31:19Z

I think we need to change the contents of $variables, $tokens, and $hostnameTokens in the CompiledRoute. They contain redundant information and the content structure of these variables ist not documentation in any way. If we remove duplicated content and put it in a (single) well defined variable, it would also reduce the information that need to be saved in the generated class by the UrlGeneratorDumper.

---------------------------------------------------------------------------

by arnaud-lb at 2012-04-26T08:54:21Z

@fabpot thanks :) I've merged it

---------------------------------------------------------------------------

by stof at 2012-04-26T12:08:40Z

A rebase is needed

---------------------------------------------------------------------------

by fabpot at 2012-04-26T13:28:08Z

no need to rebase, I will resolve the conflicts when merging. I've still have some minor changes to do before merging though. Anyone willing to have a look at implementing the Apache dumper part?

---------------------------------------------------------------------------

by Tobion at 2012-04-26T14:59:00Z

@fabpot you want to merge this for 2.1 although it introduces big changes that need extensive review and testing? But #3958 is not considered for 2.1? I thought we are in some sort of feature freeze for the components in order to not postpone the release.

---------------------------------------------------------------------------

by fabpot at 2012-04-26T17:21:09Z

@Tobion: I never said it will be in 2.1. The plan is to create a 2.1 branch soon so that we can continue working on 2.2.

---------------------------------------------------------------------------

by Koc at 2012-04-26T19:46:43Z

https://twitter.com/#!/fabpot/status/178502663690915840
c94bdf6
@mmucklo mmucklo referenced this pull request from a commit
@fabpot fabpot merged branch arnaud-lb/hostname-routes (PR #3378)
This PR was merged into the master branch.

Commits
-------

17f51a1 Merge pull request #6 from Tobion/hostname-routes
e120a7a fix API of RouteCollection
26e5684 some type fixes
514e27a [Routing] fix PhpMatcherDumper that returned numeric-indexed params that are returned besides named placeholders by preg_match
7ed3013 switch to array_replace instead of array_merge
94ec653 removed irrelevant string case in XmlFileLoader
9ffe3de synchronize the fixtures in different formats and fix default for numeric requirement
6cd3457 fixed CS
8366b8a [Routing] fixed validity check for hostname params in UrlGenerator
a8ce621 [Routing] added support for hostname in the apache matcher dumper
562174a [Routing] fixed indentation of dumped collections
1489021 fixed CS
a270458 [Routing] added some more unit tests
153fcf2 [Routing] added some unit tests for the PHP loader
68da6ad [Routing] added support for hostname in the XML loader
3dfca47 [Routing] added some unit tests for the YAML loader
92f9c15 [Routing] changed CompiledRoute signature to be more consistent
d91e5a2 [Routing] fixed Route annotation for hostname (should be hostname_pattern instead of hostnamePattern)
62de881 [Routing] clarified a variable content
11b4378 [Routing] added hostname support in UrlMatcher
fc015d5 [Routing] fixed route generation with a hostname pattern when the hostname is the same as the current one (no need to force the generated URL to be absolute)
462999d [Routing] display hostname pattern in router:debug output
805806a [Routing] added hostname matching support to UrlGenerator
7a15e00 [Routing] added hostname matching support to AnnotationClassLoader
cab450c [Routing] added hostname matching support to YamlFileLoader
85d11af [Routing] added hostname matching support to PhpMatcherDumper
402359b [Routing] added hostname matching support to RouteCompiler
add3658 [Routing] added hostname matching support to Route and RouteCollection
23feb37 [Routing] added hostname matching support to CompiledRoute

Discussion
----------

[2.2][Routing] hostname pattern for routes

Bug fix: no
Feature addition: yes
Fixes the following tickets: #1762, #3276
Backwards compatibility break: no
Symfony2 tests pass: yes

This adds a hostname_pattern property to routes. It works like the pattern property (hostname_pattern can have variables, requirements, etc). The hostname_pattern property can be set on both routes and route collections.

Yaml example:

``` yaml
# Setting the hostname_pattern for a whole collection of routes

AcmeBundle:
    resource: "@AcmeBundle/Controller/"
    type: annotation
    prefix: /
    hostname_pattern: {locale}.example.com
    requirements:
        locale: en|fr

# Setting the hostname_pattern for single route

some_route:
    pattern: /hello/{name}
    hostname_pattern: {locale}.example.com
    requirements:
        locale: en|fr
        name: \w+
    defaults:
        _controller: Foo:bar:baz
```

Annotations example:

``` php
<?php

/**
 * Inherits requirements and hostname pattern from the collection
 * @Route("/foo")
 */
public function fooAction();

/**
 * Set a specific hostnamePattern for this route only
 * @Route("/foo", hostnamePattern="{_locale}.example.com", requirements={"_locale="fr|en"})
 */
public function fooAction();

```

Performance:

Consecutive routes with the same hostname pattern are grouped, and a single test is made against the hostname for this group, so the overhead is very low:

```
@Route("/foo", hostnamePattern="a.example.com")
@Route("/bar", hostnamePattern="a.example.com")
@Route("/baz", hostnamePattern="b.example.com")
```

is compiled like this:

```
if (hostname matches a.example.com) {
    // test route "/foo"
    // test route "/bar"
}
if (hostname matches b.example.com) {
    // test route "/baz"
}
```

The PR also tries harder to optimize routes sharing the same prefix:

```
@Route("/cafe")
@Route("/cacao")
@Route("/coca")
```

is compiled like this:

```
if (url starts with /c) {
    if (url starts with /ca) {
        // test route "/cafe"
        // test route "/cacao"
    }
    // test route "/coca"
}
```

---------------------------------------------------------------------------

by Koc at 2012-02-16T14:14:19Z

Interesting. Have you looked at #3057, #3002?

Killer feature of #3057 : multiple hostnames per route.

---------------------------------------------------------------------------

by arnaud-lb at 2012-02-16T14:21:28Z

@Koc yes, the main difference is that this PR allows variables in the hostname pattern, with requirements, etc just like the path pattern. The other PRs use a `_host` requirement, which works like the `_method` requirement (takes a list of allowed hostnames separated by `|`).

> Killer feature of #3057 : multiple hostnames per route.

If you have multiple tlds you can easily do it like this:

``` yaml
hostbased_route:
  pattern:  /
  hostname_pattern: symfony.{tld}
  requirements:
     tld: org|com
```

Or with completely different domain names:

``` yaml
hostbased_route:
  pattern:  /
  hostname_pattern: {domain}
  requirements:
     domain: example\.com|symfony\.com
```

Requirements allow DIC %parameters%, so you can also put you domains in your config.yml.

---------------------------------------------------------------------------

by Koc at 2012-02-16T15:52:16Z

wow, nice! So looks like this PR closes my #3276 ticket?

---------------------------------------------------------------------------

by arnaud-lb at 2012-02-16T15:53:55Z

Yes, apparently :)

---------------------------------------------------------------------------

by Koc at 2012-02-16T15:56:53Z

I cann't find method `ParameterBag::resolveValue` calling in this PR, like here https://github.com/symfony/symfony/pull/3316/files

---------------------------------------------------------------------------

by arnaud-lb at 2012-02-16T16:03:48Z

I think it's in core already

---------------------------------------------------------------------------

by Koc at 2012-02-16T16:11:38Z

looks like yes
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Routing/Router.php#L81

---------------------------------------------------------------------------

by dlsniper at 2012-02-16T19:37:57Z

This PR looks great, it's something like this I've been waiting for.

I know @fabpot said he's working on something similar but I think if he agrees with this it could be a great addition to the core.

@fabpot , @stof any objections about this PR if gets fully done?

---------------------------------------------------------------------------

by stof at 2012-02-16T20:00:21Z

Well, we already have 2 other implementations for this stuff in the PRs. @fabpot please take time to look at them

---------------------------------------------------------------------------

by stof at 2012-02-16T20:03:17Z

This one is absolutely not tested and seems to break the existing tests according to the description. So it cannot be reviewed as is.

---------------------------------------------------------------------------

by dlsniper at 2012-02-16T22:00:24Z

@stof I understand it's a WIP but the other PRs where ignored as well and like you've said, there's a bunch of PRs already on this issue all doing a thing or another. So an early feedback on this, or any other, could lead it to the right path in order to finally solve this issue.

---------------------------------------------------------------------------

by arnaud-lb at 2012-02-17T23:57:28Z

Added tests; others are passing now

---------------------------------------------------------------------------

by arnaud-lb at 2012-02-22T21:10:20Z

I'm going to add support for the Apache dumper and the XML loader; does this PR have a chance to be merged ? cc @fabpot @stof

---------------------------------------------------------------------------

by stof at 2012-02-22T22:05:23Z

@arnaud-lb We need to wait @fabpot's mind about the way he prefers to implement it to know which one can be merged.

---------------------------------------------------------------------------

by IjinPL at 2012-02-27T02:01:57Z

Forked @arnaud-lb *hostname_pattern* to add XML parasing support.

---------------------------------------------------------------------------

by stof at 2012-04-03T23:59:12Z

@arnaud-lb Please rebase your branch. It conflicts with master because of the move of the tests

@fabpot @vicb ping

---------------------------------------------------------------------------

by dlsniper at 2012-04-13T19:52:23Z

Hi,

If @arnaud-lb won't be able to rebase this I could help with some work on this but there's still the problem of actually choosing the right PR(s) for this issue. @blogsh says in his last commit that this PR is a bit better in his opinion but @fabpot needs to decide in the end.

---------------------------------------------------------------------------

by arnaud-lb at 2012-04-14T17:26:55Z

@stof rebased

---------------------------------------------------------------------------

by nomack84 at 2012-04-20T13:01:00Z

@fabpot Any final word about this pull request? It would be nice to have this feature ready for 2.1.

---------------------------------------------------------------------------

by asm89 at 2012-04-24T21:27:50Z

Using the `{_locale}` placeholder in the host would set the locale for the request just like it does now?

Another thing I'm wondering is how/if it should be possible to set the hostname pattern for all your routes, or at least when importing routes? Otherwise you'll end up repeating the same host pattern over and over again. I think this is also important when importing routes from third party bundles.

---------------------------------------------------------------------------

by fabpot at 2012-04-25T01:17:51Z

I'm reviewing this PR and I'm going to make some modifications. I will send a PR to @arnaud-lb soon.

---------------------------------------------------------------------------

by fabpot at 2012-04-25T03:10:18Z

I've sent a PR to @arnaud-lb arnaud-lb/symfony#3 that fixes some minor bugs and add support in more classes.

---------------------------------------------------------------------------

by fabpot at 2012-04-25T03:12:52Z

@asm89:

Placeholders in the hostname are managed in the same way as the ones from the URL pattern.

You can set a hostname pattern for a collection (like the prefix for URL patterns).

---------------------------------------------------------------------------

by Tobion at 2012-04-25T09:31:19Z

I think we need to change the contents of $variables, $tokens, and $hostnameTokens in the CompiledRoute. They contain redundant information and the content structure of these variables ist not documentation in any way. If we remove duplicated content and put it in a (single) well defined variable, it would also reduce the information that need to be saved in the generated class by the UrlGeneratorDumper.

---------------------------------------------------------------------------

by arnaud-lb at 2012-04-26T08:54:21Z

@fabpot thanks :) I've merged it

---------------------------------------------------------------------------

by stof at 2012-04-26T12:08:40Z

A rebase is needed

---------------------------------------------------------------------------

by fabpot at 2012-04-26T13:28:08Z

no need to rebase, I will resolve the conflicts when merging. I've still have some minor changes to do before merging though. Anyone willing to have a look at implementing the Apache dumper part?

---------------------------------------------------------------------------

by Tobion at 2012-04-26T14:59:00Z

@fabpot you want to merge this for 2.1 although it introduces big changes that need extensive review and testing? But #3958 is not considered for 2.1? I thought we are in some sort of feature freeze for the components in order to not postpone the release.

---------------------------------------------------------------------------

by fabpot at 2012-04-26T17:21:09Z

@Tobion: I never said it will be in 2.1. The plan is to create a 2.1 branch soon so that we can continue working on 2.2.

---------------------------------------------------------------------------

by Koc at 2012-04-26T19:46:43Z

https://twitter.com/#!/fabpot/status/178502663690915840
82356b4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 4, 2012
  1. @gunnarlium
This page is out of date. Refresh to see the latest.
View
37 src/Symfony/Component/Routing/Exception/HostnameNotAllowedException.php
@@ -0,0 +1,37 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Exception;
+
+/**
+ * The resource was found but the hostname is not allowed.
+ *
+ * This exception should trigger an HTTP 404 response in your application code.
+ *
+ * @author Gunnar Lium <post@gunnarlium.com>
+ *
+ */
+class HostnameNotAllowedException extends \RuntimeException implements ExceptionInterface
+{
+ protected $allowedHostnames;
+
+ public function __construct(array $allowedHostnames, $message = null, $code = 0, \Exception $previous = null)
+ {
+ $this->allowedHostnames = array_map('strtolower', $allowedHostnames);
+
+ parent::__construct($message, $code, $previous);
+ }
+
+ public function getAllowedHostnames()
+ {
+ return $this->allowedHostnames;
+ }
+}
View
23 src/Symfony/Component/Routing/Exception/InvalidHostnameParameterException.php
@@ -0,0 +1,23 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Exception;
+
+/**
+ * Exception thrown when hostname parameter doesn't match hostname from requirements
+ *
+ * @author Gunnar Lium <post@gunnarlium.com>
+ *
+ * @api
+ */
+class InvalidHostnameParameterException extends \InvalidArgumentException implements ExceptionInterface
+{
+}
View
27 src/Symfony/Component/Routing/Generator/UrlGenerator.php
@@ -15,6 +15,7 @@
use Symfony\Component\Routing\RouteCollection;
use Symfony\Component\Routing\RequestContext;
use Symfony\Component\Routing\Exception\InvalidParameterException;
+use Symfony\Component\Routing\Exception\InvalidHostnameParameterException;
use Symfony\Component\Routing\Exception\RouteNotFoundException;
use Symfony\Component\Routing\Exception\MissingMandatoryParametersException;
@@ -99,6 +100,11 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
{
$variables = array_flip($variables);
+ $preferredHost = null;
+ if (isset($parameters['_host'])) {
+ $preferredHost = $parameters['_host'];
+ unset($parameters['_host']);
+ }
$originParameters = $parameters;
$parameters = array_replace($this->context->getParameters(), $parameters);
$tparams = array_replace($defaults, $parameters);
@@ -151,6 +157,25 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
$scheme = $req;
}
+ $host = $this->context->getHost();
+ if (isset($requirements['_host']) && ($req = strtolower($requirements['_host'])) && $host != $req) {
+ $hosts = explode('|', $req);
+ $absolute = true;
+ if (1 === count($hosts)) {
+ $host = $req;
+ } else {
+ if ($preferredHost) {
+ if (in_array($preferredHost, $hosts)) {
+ $host = $preferredHost;
+ } else {
+ throw new InvalidHostnameParameterException(sprintf('Preferred hostname for route "%s" must match "%s" ("%s" given).', $name, $req, $preferredHost));
+ }
+ } elseif (!in_array($host, $hosts)) {
+ $host = $hosts[0];
+ }
+ }
+ }
+
if ($absolute) {
$port = '';
if ('http' === $scheme && 80 != $this->context->getHttpPort()) {
@@ -159,7 +184,7 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
$port = ':'.$this->context->getHttpsPort();
}
- $url = $scheme.'://'.$this->context->getHost().$port.$url;
+ $url = $scheme.'://'.$host.$port.$url;
@stof Collaborator
stof added a note

this is not enough. If the current host (the one in the context) does not match the requirement, the url should be generated as absolute even if the call to generate did not set the absolute parameter to true

Do you agree that it makes sense that routes with host requirement always are generated as absolute? If so, I believe adding $absolute = true at line 105 should do the trick.

@stof Collaborator
stof added a note

no, they should be generated as absolute only if the host needs to be changed. if the current host already match the requirement, the control should be left to the developer if he doesn't want an absolute url

@stof Collaborator
stof added a note

oups sorry, I missed the line 163 which already implements it

Right, missed it myself too ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
}
View
34 src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
@@ -62,11 +62,14 @@ private function addMatcher($supportsRedirections)
public function match(\$pathinfo)
{
\$allow = array();
+ \$hosts = array();
\$pathinfo = urldecode(\$pathinfo);
$code
- throw 0 < count(\$allow) ? new MethodNotAllowedException(array_unique(\$allow)) : new ResourceNotFoundException();
+ throw 0 < count(\$allow) ? new MethodNotAllowedException(array_unique(\$allow)) :
+ 0 < count(\$hosts) ? new HostnameNotAllowedException(array_unique(\$hosts)) :
+ new ResourceNotFoundException();
@Tobion Collaborator
Tobion added a note

missing ( ) around the second condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
EOF;
@@ -148,7 +151,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
$hasTrailingSlash = false;
$matches = false;
$methods = array();
-
+ $hostnames = array();
if ($req = $route->getRequirement('_method')) {
$methods = explode('|', strtoupper($req));
// GET and HEAD are equivalent
@@ -156,6 +159,9 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
$methods[] = 'HEAD';
}
}
+ if ($req = $route->getRequirement('_host')) {
+ $hostnames = explode('|', strtolower($req));
+ }
$supportsTrailingSlash = $supportsRedirections && (!$methods || in_array('HEAD', $methods));
@@ -212,6 +218,27 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
}
}
+ if ($hostnames) {
+ $gotoname = 'not_'.preg_replace('/[^A-Za-z0-9_]/', '', $name);
@Tobion Collaborator
Tobion added a note

this one is duplicated now. you could use this above the corresponding code generation

if ($methods || $hostnames) {
    $gotoname = 'not_'.preg_replace('/[^A-Za-z0-9_]/', '', $name);
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ if (1 === count($hostnames)) {
+ $code .= <<<EOF
+ if (\$this->context->getHost() != '$hostnames[0]') {
+ \$hosts[] = '$hostnames[0]';
+ goto $gotoname;
+ }
+EOF;
+ } else {
+ $hostnames = implode('\', \'', $hostnames);
+ $code .= <<<EOF
+ if (!in_array(\$this->context->getHost(), array('$hostnames'))) {
+ \$hosts = array_merge(\$hosts, array('$hostnames'));
+ goto $gotoname;
+ }
+
+EOF;
+ }
+ }
+
if ($hasTrailingSlash) {
$code .= <<<EOF
if (substr(\$pathinfo, -1) !== '/') {
@@ -248,7 +275,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
}
$code .= " }\n";
- if ($methods) {
+ if ($methods || $hostnames) {
$code .= " $gotoname:\n";
}
@@ -261,6 +288,7 @@ private function startClass($class, $baseClass)
<?php
use Symfony\Component\Routing\Exception\MethodNotAllowedException;
+use Symfony\Component\Routing\Exception\HostnameNotAllowedException;
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
use Symfony\Component\Routing\RequestContext;
View
22 src/Symfony/Component/Routing/Matcher/UrlMatcher.php
@@ -12,6 +12,7 @@
namespace Symfony\Component\Routing\Matcher;
use Symfony\Component\Routing\Exception\MethodNotAllowedException;
+use Symfony\Component\Routing\Exception\HostnameNotAllowedException;
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
use Symfony\Component\Routing\RouteCollection;
use Symfony\Component\Routing\RequestContext;
@@ -79,14 +80,19 @@ public function getContext()
public function match($pathinfo)
{
$this->allow = array();
+ $this->hosts = array();
if ($ret = $this->matchCollection(urldecode($pathinfo), $this->routes)) {
return $ret;
}
- throw 0 < count($this->allow)
- ? new MethodNotAllowedException(array_unique(array_map('strtoupper', $this->allow)))
- : new ResourceNotFoundException();
+ if (0 < count($this->allow)) {
+ throw new MethodNotAllowedException(array_unique(array_map('strtoupper', $this->allow)));
+ }
+ if (0 < count($this->hosts)) {
+ throw new HostnameNotAllowedException(array_unique(array_map('strtolower', $this->hosts)));
+ }
+ throw new ResourceNotFoundException();
}
/**
@@ -150,6 +156,16 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
continue;
}
+ // check hostname requirement
+ if ($req = $route->getRequirement('_host')) {
+ $host = $this->context->getHost();
+ if (!in_array($host, $req = explode('|', $req))) {
+ $this->hosts = array_merge($this->hosts, $req);
+
+ continue;
+ }
+ }
+
return array_merge($this->mergeDefaults($matches, $route->getDefaults()), array('_route' => $name));
}
}
View
17 src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php
@@ -1,6 +1,7 @@
<?php
use Symfony\Component\Routing\Exception\MethodNotAllowedException;
+use Symfony\Component\Routing\Exception\HostnameNotAllowedException;
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
use Symfony\Component\Routing\RequestContext;
@@ -23,6 +24,7 @@ public function __construct(RequestContext $context)
public function match($pathinfo)
{
$allow = array();
+ $hosts = array();
$pathinfo = urldecode($pathinfo);
// foo
@@ -52,6 +54,17 @@ public function match($pathinfo)
}
not_barhead:
+ // barhost
+ if (0 === strpos($pathinfo, '/barhost') && preg_match('#^/barhost/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
+ if (!in_array($this->context->getHost(), array('symfony.com', 'symfony.org'))) {
+ $hosts = array_merge($hosts, array('symfony.com', 'symfony.org'));
+ goto not_barhost;
+ }
+ $matches['_route'] = 'barhost';
+ return $matches;
+ }
+ not_barhost:
+
// baz
if ($pathinfo === '/test/baz') {
return array('_route' => 'baz');
@@ -162,6 +175,8 @@ public function match($pathinfo)
return $matches;
}
- throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException();
+ throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) :
+ 0 < count($hosts) ? new HostnameNotAllowedException(array_unique($hosts)) :
+ new ResourceNotFoundException();
}
}
View
17 src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php
@@ -1,6 +1,7 @@
<?php
use Symfony\Component\Routing\Exception\MethodNotAllowedException;
+use Symfony\Component\Routing\Exception\HostnameNotAllowedException;
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
use Symfony\Component\Routing\RequestContext;
@@ -23,6 +24,7 @@ public function __construct(RequestContext $context)
public function match($pathinfo)
{
$allow = array();
+ $hosts = array();
$pathinfo = urldecode($pathinfo);
// foo
@@ -52,6 +54,17 @@ public function match($pathinfo)
}
not_barhead:
+ // barhost
+ if (0 === strpos($pathinfo, '/barhost') && preg_match('#^/barhost/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
+ if (!in_array($this->context->getHost(), array('symfony.com', 'symfony.org'))) {
+ $hosts = array_merge($hosts, array('symfony.com', 'symfony.org'));
+ goto not_barhost;
+ }
+ $matches['_route'] = 'barhost';
+ return $matches;
+ }
+ not_barhost:
+
// baz
if ($pathinfo === '/test/baz') {
return array('_route' => 'baz');
@@ -184,6 +197,8 @@ public function match($pathinfo)
return array('_route' => 'nonsecure');
}
- throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException();
+ throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) :
+ 0 < count($hosts) ? new HostnameNotAllowedException(array_unique($hosts)) :
+ new ResourceNotFoundException();
}
}
View
32 src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php
@@ -50,6 +50,38 @@ public function testAbsoluteSecureUrlWithNonStandardPort()
$this->assertEquals('https://localhost:8080/app.php/testing', $url);
}
+ public function testAbsoluteUrlWithOneHostnameAddsHostname()
+ {
+ $routes = $this->getRoutes('test', new Route('/hostname', array(), array('_host' => 'symfony.com')));
+ $url = $this->getGenerator($routes)->generate('test', array(), true);
+
+ $this->assertEquals('http://symfony.com/app.php/hostname', $url);
+ }
+
+ public function testAbsoluteUrlWithMultipleHostnamesPicksFirstHostnameIfHostnameNotInContext()
+ {
+ $routes = $this->getRoutes('test', new Route('/hostname', array(), array('_host' => 'symfony.com|symfony.org')));
+ $url = $this->getGenerator($routes, array('host' => 'symfony.net'))->generate('test', array(), true);
+
+ $this->assertEquals('http://symfony.com/app.php/hostname', $url);
+ }
+
+ public function testAbsoluteUrlWithMultipleHostnamesPicksHostnameFromContextIfAvailable()
+ {
+ $routes = $this->getRoutes('test', new Route('/hostname', array(), array('_host' => 'symfony.com|symfony.org')));
+ $url = $this->getGenerator($routes, array('host' => 'symfony.org'))->generate('test', array(), true);
+
+ $this->assertEquals('http://symfony.org/app.php/hostname', $url);
+ }
+
+ public function testAbsoluteUrlWithMultipleHostnamesAndSpecifiedHostUsesSpecified()
+ {
+ $routes = $this->getRoutes('test', new Route('/hostname', array(), array('_host' => 'symfony.com|symfony.org')));
+ $url = $this->getGenerator($routes)->generate('test', array('_host' => 'symfony.org'), true);
+
+ $this->assertEquals('http://symfony.org/app.php/hostname', $url);
+ }
+
public function testRelativeUrlWithoutParameters()
{
$routes = $this->getRoutes('test', new Route('/testing'));
View
6 src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php
@@ -84,6 +84,12 @@ protected function getRouteCollection()
array(),
array('_method' => 'GET')
));
+ // hostname requirement
+ $collection->add('barhost', new Route(
+ '/barhost/{foo}',
+ array(),
+ array('_host' => 'symfony.com|symfony.org')
+ ));
// simple
$collection->add('baz', new Route(
'/test/baz'
View
37 src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php
@@ -12,6 +12,7 @@
namespace Symfony\Component\Routing\Tests\Matcher;
use Symfony\Component\Routing\Exception\MethodNotAllowedException;
+use Symfony\Component\Routing\Exception\HostnameNotAllowedException;
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
use Symfony\Component\Routing\Matcher\UrlMatcher;
use Symfony\Component\Routing\Route;
@@ -44,6 +45,21 @@ public function testMethodNotAllowed()
}
}
+ public function testHostnameNotAllowed()
+ {
+ $coll = new RouteCollection();
+ $coll->add('foo', new Route('/foo', array(), array('_host' => 'symfony.com')));
+
+ $matcher = new UrlMatcher($coll, new RequestContext());
+
+ try {
+ $matcher->match('/foo');
+ $this->fail();
+ } catch (HostnameNotAllowedException $e) {
+ $this->assertEquals(array('symfony.com'), $e->getAllowedHostnames());
+ }
+ }
+
public function testHeadAllowedWhenRequirementContainsGet()
{
$coll = new RouteCollection();
@@ -104,6 +120,27 @@ public function testMatch()
$matcher = new UrlMatcher($collection, new RequestContext(), array());
$this->assertInternalType('array', $matcher->match('/foo'));
$matcher = new UrlMatcher($collection, new RequestContext('', 'head'), array());
+
+ // test that route only matches when valid hostname is provided
+ $collection = new RouteCollection();
+ $collection->add('foo', new Route('/foo', array(), array('_host' => 'symfony.com')));
+
+ // route does not match if no hostname is provided
+ $matcher = new UrlMatcher($collection, new RequestContext(), array());
+ try {
+ $matcher->match('/foo');
+ $this->fail();
+ } catch (HostnameNotAllowedException $e) {}
+
+ // route does not match if for wrong hostname
+ $matcher = new UrlMatcher($collection, new RequestContext('', 'head', 'symfony.org'), array());
+ try {
+ $matcher->match('/foo');
+ $this->fail();
+ } catch (HostnameNotAllowedException $e) {}
+
+ // route does match if correct hostname is provided
+ $matcher = new UrlMatcher($collection, new RequestContext('', 'head', 'symfony.com'), array());
$this->assertInternalType('array', $matcher->match('/foo'));
// route with an optional variable as the first segment
Something went wrong with that request. Please try again.