Skip to content
This repository

[2.2][Routing] hostname pattern for routes #3378

Merged
merged 1 commit into from over 1 year ago
Arnaud Le Blanc

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:

# 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

/**
 * 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"
}
Konstantin.Myakshin

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

Killer feature of #3057 : multiple hostnames per route.

Arnaud Le Blanc

@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:

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

Or with completely different domain names:

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.

Konstantin.Myakshin

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

Arnaud Le Blanc

Yes, apparently :)

Konstantin.Myakshin

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

Arnaud Le Blanc

I think it's in core already

Florin Patan

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?

Christophe Coevoet
Collaborator

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

Christophe Coevoet
Collaborator

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

Florin Patan

@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.

Arnaud Le Blanc

Added tests; others are passing now

Arnaud Le Blanc

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

Christophe Coevoet
Collaborator

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

Błażej Krysiak

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

Christophe Coevoet
Collaborator
stof commented April 03, 2012

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

@fabpot @vicb ping

Florin Patan

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.

Arnaud Le Blanc

@stof rebased

Mario A. Alvarez Garcia

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

Alexander
asm89 commented April 24, 2012

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.

Fabien Potencier
Owner

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

Fabien Potencier
Owner

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

Fabien Potencier
Owner

@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).

Tobias Schultze
Collaborator

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.

src/Symfony/Component/Routing/Matcher/Dumper/DumperPrefixCollection.php
((42 lines not shown))
  42
+
  43
+    /**
  44
+     * Adds a route in the tree
  45
+     *
  46
+     * @param  DumperRoute $route The route
  47
+     * @return DumperPrefixCollection The node the route was added to
  48
+     */
  49
+    public function addPrefixRoute(DumperRoute $route)
  50
+    {
  51
+        $prefix = $route->getRoute()->compile()->getStaticPrefix();
  52
+
  53
+        if ($this->getPrefix() === $prefix) {
  54
+            $this->addRoute($route);
  55
+            return $this;
  56
+
  57
+        } else if ('' === $this->getPrefix() || 0 === strpos($prefix, $this->getPrefix())) {
1
Tobias Schultze Collaborator
Tobion added a note April 25, 2012

no need for elseif, previous statement returns

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/Matcher/Dumper/DumperPrefixCollection.php
((55 lines not shown))
  55
+            return $this;
  56
+
  57
+        } else if ('' === $this->getPrefix() || 0 === strpos($prefix, $this->getPrefix())) {
  58
+            $prev = $this;
  59
+            for ($i = strlen($this->getPrefix()); $i < strlen($prefix); ++$i) {
  60
+                $collection = new DumperPrefixCollection();
  61
+                $collection->setPrefix(substr($prefix, 0, $i+1));
  62
+                $prev->addRoute($collection);
  63
+                $prev = $collection;
  64
+            }
  65
+
  66
+            $collection->addRoute($route);
  67
+
  68
+            return $collection;
  69
+
  70
+        } else {
1
Tobias Schultze Collaborator
Tobion added a note April 25, 2012

same

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/Matcher/Dumper/DumperCollection.php
((35 lines not shown))
  35
+    /**
  36
+     * Returns the parent collection
  37
+     *
  38
+     * @return DumperCollection The parent collection
  39
+     */
  40
+    public function getParent()
  41
+    {
  42
+        return $this->parent;
  43
+    }
  44
+
  45
+    /**
  46
+     * Sets the parent collection
  47
+     *
  48
+     * @param DumperCollection $parent The parent collection
  49
+     */
  50
+    public function setParent(DumperCollection $parent)
1
Tobias Schultze Collaborator
Tobion added a note April 25, 2012

no need to make it public, private is fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/RouteCollection.php
((5 lines not shown))
176 179
      *
177 180
      * @throws \InvalidArgumentException When the RouteCollection already exists in the tree
178 181
      *
179 182
      * @api
180 183
      */
181  
-    public function addCollection(RouteCollection $collection, $prefix = '', $defaults = array(), $requirements = array(), $options = array())
  184
+    public function addCollection(RouteCollection $collection, $prefix = '', $defaults = array(), $requirements = array(), $options = array(), $hostnamePattern = null)
3
Tobias Schultze Collaborator
Tobion added a note April 25, 2012

I think you can rename all occurences hostnamePattern in this class to simply hostname because it's only prefix either and not prefixPattern. And the prefix can already contain variables.

In RouteCollection and Route, or in configuration too ? (annotation, xml, yml). I think hostnamePattern in configuration makes it more obvious that you can put {parameters} in it.

Fabien Potencier Owner
fabpot added a note November 05, 2012

The prefix can also have parameters in it. So, I would rename hostnamePattern to hostnamePrefix to be more consistent (this only needs to be done here).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/Tests/Matcher/Dumper/DumperPrefixCollectionTest.php
... ...
@@ -0,0 +1,60 @@
  1
+<?php
  2
+
  3
+namespace Symfony\Component\Routing\Tests\Matcher\Dumper;
  4
+
  5
+use Symfony\Component\Routing\Route;
  6
+use Symfony\Component\Routing\RouteCollection;
  7
+use Symfony\Component\Routing\Matcher\Dumper\DumperPrefixCollection;
  8
+use Symfony\Component\Routing\Matcher\Dumper\DumperRoute;
  9
+
  10
+class DumperPrefixCollectionTest extends \PHPUnit_Framework_TestCase
  11
+{
  12
+    public function testAddPrefixRoute()
  13
+    {
  14
+        $coll = new DumperPrefixCollection;
1
Tobias Schultze Collaborator
Tobion added a note April 25, 2012

missing () (multiple times)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Arnaud Le Blanc arnaud-lb referenced this pull request from a commit April 26, 2012
Commit has since been removed from the repository and is no longer available.
Arnaud Le Blanc

@fabpot thanks :) I've merged it

Christophe Coevoet
Collaborator
stof commented April 26, 2012

A rebase is needed

Fabien Potencier
Owner

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?

src/Symfony/Component/Routing/RouteCollection.php
... ...
@@ -41,6 +42,7 @@ public function __construct()
41 42
         $this->routes = array();
42 43
         $this->resources = array();
43 44
         $this->prefix = '';
  45
+        $this->hostnamePattern = null;
1
Tobias Schultze Collaborator
Tobion added a note April 26, 2012

should probably default to '' (same as prefix)

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

@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.

Fabien Potencier
Owner

@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.

Fabien Potencier
Owner

@Koc: the tweet was written on Match 10th. Things have changed since then ;)

Konstantin.Myakshin
Koc commented April 26, 2012

It's sad. This feature blocked us, for example (our project cannot to be switched to Symfony2 routing so we are using our implementation, wich not nice)

Błażej Krysiak

I am using this implementations with some tweaks and it hadles quite well.

Fabien Potencier
Owner

@IjinPL Can you tell us which tweaks?

Alexander
asm89 commented April 26, 2012

@Koc Have you looked at the JMSI18nRoutingBundle? It supports different hosts for different locales and generates and matches routes accordingly. Not sure if it is what you need, but it may be useful. :)

François Pluchino

@fabpot Is it still possible to integrate this functionality for 2.1?

This is a cool feature, very popular, and at this state, this PR is functionnal and doesn't break any compatibility. So, what does prevent the merge? The missing XML Loader? The lack of testing?

Jeremy Marc

yes I agree with @francoispluchino and @IjinPL, it's a must-have feature and it would be great to have it for the 2.1 version

Yoann Chambonnet

I agree too with @francoispluchino @j075m and @IjinPL would be great to have it for the 2.1 version.

Fabien Potencier
Owner

I agree with all of you but we need to stop at some point and this PR is not yet finished (I have implemented the XML loader between).

François Pluchino

The 2.1 GA will be out in several months, about 10 to 12 months after the 2.0 GA. So if we have to wait the 2.2 for this feature, we'll have to wait until Q2 of 2013.

We always can say it's not perfect for adding a new feature, but anyway, we just add a behavior to an existing feature.
It can be merged but we avoid to promote it for the 2.1, even it's useable. This is very common.

@fabpot I understand your point of view, but as it's a very popular feature, can you make an exception for this PR?
Thanking you in advance.

Fabien Potencier
Owner

People have asked for other exceptions ;)

The plan after 2.1 is to have faster release cycles (a bit like what I do for Twig). So, hopefully, 2.2 will be released in July/August 2012.

Florin Patan

@fabpot Consider that if 2.2 will also bring a bunch of BC breaks and so on people won't be tempted to ever use a certain version of Symfony2 just because it breaks BC too often. So why not release this under 2.1 version and have potentially less BC breaks in 2.2? Also consider that the original issue is date one year ago almost and now we actually have a good fix for it.

Arnaud Le Blanc

@fabpot I've added support for the apache dumper

I believe this PR is finished now :) Is there any chance to get it merged in 2.1 ?

Fabien Potencier
Owner

we are still discussing the schedule of Symfony 2.1 and 2.2, so depending on what we decide, there is a chance to get this into 2.1.

Błażej Krysiak
Neil Ferreira

I am patiently waiting to hear that this will be merged into 2.1, definitely an excellent feature and since it has backwards compatibility it should be a no-brainer :)

Victor Berchet
vicb commented May 04, 2012

btw @arnaud-lb any feedback on my last comment ?

Arnaud Le Blanc

@vicb I've added a test :)

Escaping the route te\ st gives te\\ st.

Apache apparently only knows backslash-space escape sequence (not \\), so te\\ st is seen as a single directive argument, and unescaped as te\ st.

The route /{foo} with requirements "foo"="te\\ st" is compiled as ^/(\\ )$, escaped ^/(\\\ )$, and unescaped as the original regex ^/(\\ )$.

Tobias Schultze
Collaborator
Tobion commented May 06, 2012

@vicb I don't understand the problem. Requirements regex and paths MUST both NOT be escaped/encoded by the user.
So a path like /sp ace must be given if a user wants to have a space in the path and the routing component will make sure it's escaped at the right places. If the user preencodes it like /sp%20ce, the routing component would double-encode it (as the component cannot know he wants a space instead of the real chars %20).
Same applies to regex requirements. They must not be escaped by the user.

Arnaud Le Blanc

@Tobion, the current PR considers \ as the user wants to match a \ in his route, not as the user escaped some character following the \. The test I've just added verifies this.

Victor Berchet
vicb commented May 06, 2012

well actually what I meant was that [\w\ ] === [\w ] in term or requirements (i.e. it is valid to escape spaces in regex). I think that both those reqs should generate the same thing in the apache file, ie: RewriteCond %{REQUEST_URI} ^/test/([^\w\ ]+?)/$, that is the space must be escaped.

Arnaud Le Blanc

currently, [\w\ ] will be escaped as [\w\\ ], and seen as [\w\ ] by apache. And [\w ] will be escaped as [\w\ ] and seen as [\w ] by apache.

So in both cases, in the end apache is seeing the original regex.

Tobias Schultze
Collaborator
Tobion commented May 06, 2012

@arnaud-lb correct, that's what I'm saying.
@vicb if [\w\ ] === [\w ] then it must also be valid to excape other chars optionally like [\w\"] === [\w"] but for some it's not true: [\w\x] !== [\wx]. That's at least the case for phps preg_match. Is [\w\ ] === [\w ] the case when using the PHP matcher? Then it should also be the case for the apache matcher. Because otherwise when switching the matcher from php to apache it would match different things.

Victor Berchet
vicb commented May 06, 2012

@Tobion The issue is that RewriteCond %{REQUEST_URI} ^/foo/te st$ is not valid because of the space. So spaces have to be escaped (RewriteCond %{REQUEST_URI} ^/foo/te\ st$), there is no issue with other chars.

Tobias Schultze
Collaborator
Tobion commented May 06, 2012

@arnaud-lb my question is whether apache interprets [\w\ ] and [\w ] the same, i.e. match excactly the same? Because phps preg_match does so, and thus IMO also the PhpMatcher class.

Arnaud Le Blanc

@Tobion apache would interpret [\w ] as a [\w argument, followed by a ] argument. So we escape the space charaters (and only space characters).

So [\w\ ] and [\w ] are wrote [\w\\ ] and [\w\ ] in the RewriteRule, but seen as [\w\ ] and [\w ] by apache / mod_rewrite (i.e. once RewriteRule arguments are parsed, Apache sees the original regex).

Victor Berchet
vicb commented May 06, 2012

@arnaud-lb

    RewriteCond %{REQUEST_URI} ^/ba\\ r$
    RewriteRule ^(.*)$ index.php [QSA,L]

would not match the URL "/ba r" on my system, but the following would:

    RewriteCond %{REQUEST_URI} ^/ba\ r$
    RewriteRule ^(.*)$ index.php [QSA,L]

which explains how escaping was implemented before.

If I understand your comments correctly it does not work the same on your setup. It is weird that the behavior is not consistent across all setups but at least the former solution (escaping spaces only when they are not already escaped) works for both our setup so I think it should be re-introduced.

What do you think ?

Arnaud Le Blanc

^/ba\\ r$ would not match the URL "/ba r" on my system

This regex means that your route's pattern is /ba\ r, which means that you want your route to match exactly that: /ba followed by \ followed by a space and r.

So it's expected that it doesn't match /ba r :)

The user shouldn't be responsible of escaping the regex for the rewrite rule, so we never consider that he did it (i.e. if there is a backslash before a space, we consider that the user wanted to match a backslash and a space).

I think that the PhpMatcher works in a similar way (if you have a \ in your route pattern, it matches a \). So this makes both matchers consistent.

Victor Berchet
vicb commented May 06, 2012

Ok so I think I got confused because we don't have the same expectations on the requirements then.

  1. Requirements are Regexs
  2. In PCRE patterns both " " and "\ " match a space
  3. If my requirement is "fo\ o" you should consider that I want to match the path "fo o" (the requirements should be "fo\ o" to match "fo\ o")
Jordan Stout
j commented May 08, 2012

Haven't dug deep into this, but +1 on the idea. I was looking into creating a more optimized route matcher like the one you explained above (by looking at prefixes and digging deep into them). One of my projects has about 80 routes and that poor last one takes a little back power to get to in the current implementation :P

Victor Berchet
vicb commented May 15, 2012

@arnaud-lb should I submit a PR to this branch to revert the changes on regexp escaping ? do you handle it ?

Matthias Breddin

This should really be implented in 2.1

Konstantin.Myakshin
Koc commented May 30, 2012

there is one interesting case: we have 2 sites (A, B). We should generate links to site B from A, but this routes shouldn't be matched

Błażej Krysiak
IjinPL commented May 30, 2012
Christophe Coevoet
Collaborator
stof commented May 30, 2012

@Koc for this, I would create a separate instance of the UrlGenerator with its own set of routes (the routes of site B) and call it when generating a link to the site B (and you can add a Twig function calling this generator if you need it)

Cédric Dugat
Ph3nol commented June 03, 2012

Very impatient about this useful feature implementation!
I agree with @lunetics: it really should be a 2.1 routing improvement. :)

Matthias Breddin

any update on this yet?

Mario A. Alvarez Garcia

The first Beta will be released after this weekend when the SymfonyLive ends. If this isn't include until now, I guess it will be delayed until December. So sad....

Heiko Krebs

Would be so great to use this in 2.1!

François-Xavier de Guillebon
deguif commented June 14, 2012

@nomack84 : is the first Beta released ?

Mario A. Alvarez Garcia

@deguif Not yet, but it should be around the corner as @fabpot said here: http://symfony.com/blog/towards-symfony-2-1

Ka Yue Yeung
kayue commented June 25, 2012

Would really like to see this included in 2.1.

Fabien Potencier
Owner
fabpot commented June 25, 2012

This will be included in 2.2.

Florin Patan

@fabpot Since this is a very useful feature and has lots of demands maybe it would help if you'd release a roadmap/timeline for 2.2 so that people know what to expect and don't flood with requests for one PR or another to be included in 2.1?

Thanks

Christophe Coevoet
Collaborator
stof commented June 26, 2012

@dlsniper 2.1 is in beta so it is frozen now. No new features will be merged in it anymore.

Florin Patan

@stof I know, that's why I didn't pushed for it.
Instead a roadmap/timeline for 2.2 could shed some light on what features/PRs are going to be included then and people could better focus on fixing 2.1.
I know it's hard to have something like that but I don't think it's the proper place to start such a discussion. I'll try and post something on the mailing list when my time will allow.

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request fails (merged d1b63dc into e0351c9).

Ciro Vargas

I have two bundles, each have a same rout.and with a domain parameter i'll know which is the bundle will run the request?

example: /blog

Bundle A

user_blog: pattern: /blog defaults: { _controller: UserBlogBundle:Default:blog }

Bundle B

team_blog: pattern: /blog defaults: { _controller: TeamBlogBundle:Default:blog }

if(domainA)
BundleA
else
BundleB


i make this:

//src/project/TestBundle
use Symfony\Component\Routing\RouteCollection;

$collection = new RouteCollection();
$req = $this->getRequest();
if($req->server->get('SERVER_NAME') == 'www.domainA.com')
$collection->addCollection($loader->import("@BundleABundle/Resources/config/routing.php"));
else
$collection->addCollection($loader->import("@BundleBBundle/Resources/config/routing.php"));
return $collection;

this is usual?

Cédric Dugat
Ph3nol commented July 14, 2012

What about creating two applications with .htaccess routing in function of the domain used?

Here is an .htaccess file example for subdomains filtering:

<IfModule mod_rewrite.c>                                                                            
    RewriteEngine On

    RewriteCond %{HTTP_HOST} ^example.com$
    RewriteRule ^(.*) http://www.example.com/$1 [QSA,L,R=301]

    RewriteCond %{REQUEST_FILENAME} !-f
    RewriteCond %{HTTP_HOST} ^www.*
    RewriteRule ^(.*)$ app.php [QSA,L]

    RewriteCond %{REQUEST_FILENAME} !-f
    RewriteCond %{HTTP_HOST} ^secondary.*
    RewriteRule ^(.*)$ secondaryApp.php [QSA,L]
</IfModule>

In this example, www.example.com matches with app project application and secondary.example.com with secondaryApp one.

Is it clear for you?

Ka Yue Yeung
kayue commented July 14, 2012

Having two app is a better but temporary way to fix it at this moment. I don't think @cirovargas's method would work as Symfony generate a lot of cache file, and switch on and off a bundle won't help in some cases (access control etc)

Eventually we will need subdomain support in routing, as well as in all config.yml including firewall / access control etc.

Ciro Vargas

hmn, my architecture is this

//app/config/routing.yml
MultiBundle:
resource: "@MultiBundle/Resources/config/routing.php"
prefix: /

in bundle 'Multi'
//project/MultiBundle/resources/config/routing.php

$collection = new RouteCollection();
$req = $this->getRequest();
if($req->server->get('SERVER_NAME') == 'www.domainA.com')
$collection->addCollection($loader->import("@BundleABundle/Resources/config/routing.php"));
elseif($req->server->get('SERVER_NAME') == 'www.domainB.com')
$collection->addCollection($loader->import("@BundleBBundle/Resources/config/routing.php"));
return $collection;

in Bundle A and B

home:
pattern: /
defaults: { _controller: BundleA:Default:home }

I wanted to do as best as possible for better performance and code maintenance

Christophe Coevoet
Collaborator
stof commented July 14, 2012

@cirovargas Your implementation will only work if you have separate cache folders for each domain (because the router is cached). And I'm not sure what $this is in your routing.php file

Ciro Vargas

@stof , yeah is a error sorry, if i use controller forwarding are good? http://symfony.com/doc/current/book/controller.html#forwarding

Rafael Amorim

Will It be possible to add an array of domains for hostname_pattern? I would like to match some domains from database for one bundle and others for another bundle

Mario A. Alvarez Garcia

I think is time to retake the development of this pull request, to have it soon merged on master.

Damien Alexandre

+1 on @nomack84 - this feature is what I miss the most out of SF2 at the moment. @cirovargas solution's expose the need to load different routing configuration depending on a subdomain. The security layer also need to be subdomain aware, that's why @Ph3nol solution's is maybe the best atm.

But it's an heavy task (duplicating all the app folder, double all the deployment commands (cache:clear etc), double the web folder...) and not documented (unless I missed it?).

I'm really wondering how real world SF2 websites are using a single code base with multiple sub-domains today, and how it should work tomorrow. Symfony1 was great at this btw :heart_eyes:.

What is described in this PR description don't look like at what cirovargas and I need.

Arnaud Le Blanc

rebased on master

Sebastiaan Stok

You did not rebase but merge, no worries this can be fixed :)

Arnaud Le Blanc

@sstok I'm pretty sure I rebased, how do you see that ? :) (@fabpot's commits you see in the list exist because he did a PR on my branch)

Sebastiaan Stok

OK ;) my bad.

Justin

Anyone have any updates on this? I took this code, and it works nicely so far. I'm currently attempting to add in firewall support as well (for authentication mostly where a subdomain will be authenticated but the main domain won't or vice versa or however the end user wants to utilize it)

aricem aricem referenced this pull request in aripd/albatros September 24, 2012
Open

multi-tenanted support #1

François Pluchino

@fabpot Is the actual code of this PR ok for you? And could we use this PR in the master branch?

Tobias Schultze
Collaborator

My routing PRs like 4225 should be merged first. Otherwise this PR inherits the same bugs as the current compiler.

Attila Bukor

Was this branch integration tested against the master? Because if it's up to date and works great, I would use this branch until it gets merged to Sf master (I really need this feature right now and don't want to reinvent the wheel).

Christophe Coevoet
Collaborator

@r1pp3rj4ck it currently conflicts with master.
@arnaud-lb a rebase is needed

Mario A. Alvarez Garcia

@arnaud-lb @fabpot @stof Could you please tell us something about the current state of this feature? This is a feature that many of us are been waiting for long time now.
Greetings!

Patrik Patie Gmitter

+1 still wait for the most anticipated feature :)

Arnaud Le Blanc

There are a few small conflicts due to a recently merged PR, I'll fix this soon. The PR is currently based on 2.1.0

@fabpot what are your plans about this PR ?

Tobias Schultze
Collaborator

I would suggest that you split the hostname feature and the dumper optimization. So this PR is only about the hostname pattern (without the DumperCollection etc.) and a different PR about the optimization. That makes is much simpler to review and see what is necessary and what not.

Arnaud Le Blanc

@Tobion agreed it would be simpler to review and maintain the PR; however most of this were actually required for implementing the PR IIRC. I'll see if I can move some parts in an other PR.

Mario A. Alvarez Garcia

Great @arnaud-lb. I'm sure we all want to hear @fabpot idea about the state of this PR.

src/Symfony/Component/Routing/RouteCollection.php
... ...
@@ -208,6 +211,12 @@ public function addCollection(RouteCollection $collection, $prefix = '', $defaul
208 211
         // the sub-collection must have the prefix of the parent (current instance) prepended because it does not
209 212
         // necessarily already have it applied (depending on the order RouteCollections are added to each other)
210 213
         $collection->addPrefix($this->getPrefix() . $prefix, $defaults, $requirements, $options);
  214
+
  215
+        // Allow child collection to have a different pattern
  216
+        if (!$collection->getHostnamePattern()) {
  217
+            $collection->setHostnamePattern($hostnamePattern);
3
Tobias Schultze Collaborator
Tobion added a note October 16, 2012

I think its inconsistent that default, requirements and options all overwrite existing ones in child routes. But the hostname is only applied to some routes.

I'll let you decide, but while using this feature I found it handy to be able to change the hostname of a single route even though the route collection had a hostname already.

Tobias Schultze Collaborator
Tobion added a note November 05, 2012

You can still set the a different hostname for a single route by hand. But in this function it should be consistent with the rest of its workings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Fabien Potencier fabpot referenced this pull request from a commit October 19, 2012
Fabien Potencier merged branch arnaud-lb/routing-php-dumper-simplification (PR #5734)
This PR was merged into the master branch.

Commits
-------

e54d749 [Routing] Simplified php matcher dumper (and optimized generated matcher)

Discussion
----------

[Routing] Simplified php matcher dumper (and optimized generated matcher)

Bug fix: no
Feature addition: no
Related: #3378
Backwards compatibility break: no
Symfony2 tests pass: yes

This simplifies the php matcher dumper by allowing the dumper to re-organize routes in the dumper's own structure.

As a result, dumping is made a little simpler. This is also helps much for my hostname-based routes PR #3378.

Reorganizing routes also allows to find more optimization opportunities:

Currently the dumper wraps some collections of routes in a `if (0 === strpos($pathinfo, '/someprefix')` if the collection has user-defined prefix, and if it contains more than one direct child Route. This can miss many optimization opportunities.

The PR changes this by building a prefix tree of routes based on the static prefix extracted from routes' patterns. Then every leave having a prefix and more than one child (route or collection) will be wrapped in a `if` statement.

Example:

```
// No explicit prefix is specified
@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"
}
```

Some tests have many white space changes, much more easier to review [here](https://github.com/symfony/symfony/pull/5734/files?w=1)

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

by Tobion at 2012-10-13T02:27:54Z

I'm not sure if adding these specific classes just for dumping is the best implementation because they duplicate some logic and this optimization should also work out-of-the-box with the standard RouteCollection etc.
What I have in mind is a new method in RouteCollection like `RouteCollection::optimizeTree` that returns a new RouteCollection with the Routes that includes these optimization you do here. So there would probably be no need for the new classes.

It think it requires some changes in RouteCollection like the handling of prefix that must start with a slash currently, which is too restrictive. But it should be possible.

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

by arnaud-lb at 2012-10-13T12:52:32Z

@Tobion

> I'm not sure if adding these specific classes just for dumping is the best implementation because they duplicate some logic and this optimization should also work out-of-the-box with the standard RouteCollection etc.

I think RouteCollection and DumperCollection do not share the same concerns; and RouteCollection does things that don't allow to reorganize routes freely. For instance when adding a collection to a RouteCollection this changes all the child routes' prefix, requirements, options, etc. When setting a collection's prefix, this prepends the prefix to every child route's pattern, etc.

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

by arnaud-lb at 2012-10-15T08:50:23Z

squashed the CS commits

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

by arnaud-lb at 2012-10-15T13:50:16Z

@fabpot @Tobion this PR is ready to be merged if everyone agrees

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

by Tobion at 2012-10-16T18:10:36Z

When the above is fixed, I think it's good to be merged.

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

by arnaud-lb at 2012-10-17T08:40:20Z

Fixed; thanks @Tobion @stof for your reviews

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

by Tobion at 2012-10-19T03:30:10Z

@arnaud-lb could you please test whether your PR fixes #5780 as a side-effect?
I can image that it's fixed because you use `$route->compile()->getStaticPrefix();` for prefix optimization.
If it's fixed please add a test case. If not, that's fine, and we need to fix it in another PR.
388cbff
Fabien Potencier
Owner

@arnaud-lb Now that #5734, can you update this PR for master. I will then do a final review and merge. Thanks.

Arnaud Le Blanc

Thanks! I'll see if I can extract some changes I made on the apache matcher to separate PR too (rebase will be easier after that), and then I'll rebase this one.

Mario A. Alvarez Garcia
Ciro Vargas

nice, i really need to use subdomains and multi-domains

Patrik Patie Gmitter

@cirovargas me too :)

Thomas Leidinger

Awesome! I would need that feature too

Moritz Kraft

Yes. :) Yes, please.

Mario A. Alvarez Garcia

@arnaud-lb please..., could you fine a bit of time..., many people wanting this feature!
Thanks!

Evan Owens

+1

Arnaud Le Blanc

@Tobion @stof @fabpot rebased

remaining issues are:

  • should we rename hostname_pattern to hostname in configuration ? #3378 (comment)
  • shoud it be possible to override a single route's hostname pattern if the collection has a hostname pattern already ? #3378 (comment)
Florin Patan

@arnaud-lb :

  • hostname_pattern should be used if you make it a regex pattern so that people know what they are doing but it's more of a cosmetic change rather that an important one;
  • yes, it should totally be possible to override a single route's hostname. Think of a case where you want to host the same project for multiple people but you all want to login under the same administration panel, example: s1.XYZ.ABC s2.XYZ.ABC and so on and admin.XYZ.ABC (maybe it's not the best example but something along this lines...). I'd really hate if I wouldn't be allowed to change it because it's not viewed as usefull plus it would reduce the headackes in the future and a need for another PR to fix this :)

Thanks for your hard work!

Fabien Potencier
Owner

@arnaud-lb Did you submit a PR on the docs? If not, can you make it one to document this new feature?

Arnaud Le Blanc

@fabpot yes I will

src/Symfony/Component/Routing/Generator/UrlGenerator.php
((6 lines not shown))
195 195
             $scheme = $this->context->getScheme();
196 196
             if (isset($requirements['_scheme']) && ($req = strtolower($requirements['_scheme'])) && $scheme != $req) {
197 197
                 $absolute = true;
198 198
                 $scheme = $req;
199 199
             }
200 200
 
  201
+            if ($hostnameTokens) {
  202
+                $ghost = '';
1
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

please use a better name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/Generator/UrlGenerator.php
((6 lines not shown))
195 195
             $scheme = $this->context->getScheme();
196 196
             if (isset($requirements['_scheme']) && ($req = strtolower($requirements['_scheme'])) && $scheme != $req) {
197 197
                 $absolute = true;
198 198
                 $scheme = $req;
199 199
             }
200 200
 
  201
+            if ($hostnameTokens) {
  202
+                $ghost = '';
  203
+                foreach ($hostnameTokens as $token) {
  204
+                    if ('variable' === $token[0]) {
  205
+                        if (in_array($mergedParams[$token[3]], array(null, '', false), true)) {
1
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

this seems really wrong

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/Generator/UrlGenerator.php
((6 lines not shown))
195 195
             $scheme = $this->context->getScheme();
196 196
             if (isset($requirements['_scheme']) && ($req = strtolower($requirements['_scheme'])) && $scheme != $req) {
197 197
                 $absolute = true;
198 198
                 $scheme = $req;
199 199
             }
200 200
 
  201
+            if ($hostnameTokens) {
  202
+                $ghost = '';
  203
+                foreach ($hostnameTokens as $token) {
  204
+                    if ('variable' === $token[0]) {
  205
+                        if (in_array($mergedParams[$token[3]], array(null, '', false), true)) {
  206
+                            // check requirement
  207
+                            if ($mergedParams[$token[3]] && !preg_match('#^'.$token[2].'$#', $mergedParams[$token[3]])) {
1
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

should also consider $this->strictRequirements (see code above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/Matcher/Dumper/ApacheMatcherDumper.php
... ...
@@ -131,6 +106,115 @@ public function dump(array $options = array())
131 106
     }
132 107
 
133 108
     /**
  109
+     * Dumps a single route
  110
+     *
  111
+     * @param  string $name Route name
  112
+     * @param  Route  $route The route
  113
+     * @param  array  $options Options
  114
+     * @param  bool   $hostnameRegexUnique Unique identifier for the hostname regex
  115
+     * @return string The compiled route
1
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

missing empty line (same below method).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Arnaud Le Blanc arnaud-lb referenced this pull request in symfony/symfony-docs November 05, 2012
Merged

[Routing] added hostname pattern #1894

src/Symfony/Component/Routing/Route.php
... ...
@@ -103,6 +106,18 @@ public function setPattern($pattern)
103 106
         return $this;
104 107
     }
105 108
 
  109
+    public function getHostnamePattern()
1
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

missing php doc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/Tests/Fixtures/validpattern.yml
... ...
@@ -1,5 +1,7 @@
1 1
 blog_show:
2  
-    pattern:   /blog/{slug}
3  
-    defaults:  { _controller: MyBlogBundle:Blog:show }
  2
+    pattern:          /blog/{slug}
  3
+    defaults:         { _controller: MyBlogBundle:Blog:show }
  4
+    hostname_pattern: "{locale}.example.com"
  5
+    requirements:     { 'foo': '\d+' }
1
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

I dont see a variable foo defined in any pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/Route.php
... ...
@@ -104,6 +107,28 @@ public function setPattern($pattern)
104 107
     }
105 108
 
106 109
     /**
  110
+     * Returns the hostname pattern
  111
+     *
  112
+     * @return string the pattern
1
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

not true, in your current implementation it can also return null. i'd prefer to use an empty string by default instead of null. or you need to specify string|null in many places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/CompiledRoute.php
... ...
@@ -60,6 +72,16 @@ public function getRegex()
60 72
     }
61 73
 
62 74
     /**
  75
+     * Returns the hostname regex
  76
+     *
  77
+     * @return string The hostname regex
1
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

can also return null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/Generator/UrlGenerator.php
((6 lines not shown))
195 195
             $scheme = $this->context->getScheme();
196 196
             if (isset($requirements['_scheme']) && ($req = strtolower($requirements['_scheme'])) && $scheme != $req) {
197 197
                 $absolute = true;
198 198
                 $scheme = $req;
199 199
             }
200 200
 
  201
+            if ($hostnameTokens) {
  202
+                $routeHost = '';
  203
+                foreach ($hostnameTokens as $token) {
  204
+                    if ('variable' === $token[0]) {
  205
+                        if (!array_key_exists($token[3], $defaults) || (string) $mergedParams[$token[3]] !== (string) $defaults[$token[3]]) {
3
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

this is also wrong, you probably need some more tests. then such things get obvious

Would you mind telling what's wrong ? I've copied this from the parameters check a few lines above and removed the !$optional as all variables are required in the hostname.

Tobias Schultze Collaborator
Tobion added a note November 05, 2012

These conditions are meant to find params that are not optional. And as you said, all variables are required in the hostname, so the conditions are not useful here. Simply remove it.

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

It guess it's important to mention in the docs that hostname variables are always required, even when using defaults. This is in contrast to the variables at the end of the path.

So {locale}.example.org with locale defaults to en will currently not be matched by example.org. Don't know if it's worth implementing. But it's different from the /path.{format} thing.

Ciro Vargas

the good is all the domain in pattern {prefix}{domain}/{parameters} => user1.domain1.com/blog ==> user1000.domain2.com/blog rendering all different,

Tobias Schultze Tobion commented on the diff November 05, 2012
src/Symfony/Component/Routing/Route.php
... ...
@@ -39,15 +40,17 @@ class Route implements \Serializable
39 40
      * @param array  $defaults     An array of default parameter values
40 41
      * @param array  $requirements An array of requirements for parameters (regexes)
41 42
      * @param array  $options      An array of options
  43
+     * @param string $hostname     The hostname pattern to match
1
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

also null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/Route.php
... ...
@@ -104,6 +107,28 @@ public function setPattern($pattern)
104 107
     }
105 108
 
106 109
     /**
  110
+     * Returns the hostname pattern
  111
+     *
  112
+     * @return string|null the pattern or null if no pattern is set
  113
+     */
  114
+    public function getHostnamePattern()
  115
+    {
  116
+        return $this->hostnamePattern;
  117
+    }
  118
+
  119
+    /**
  120
+     * Sets the hostname pattern
  121
+     *
  122
+     * @param string $pattern the pattern
1
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

also null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Attila Bukor

@Tobion I guess it would make more sense to let the users use these {locale}.example.com type routes with example.com too. Why do you think it shouldn't be that way?

Tobias Schultze
Collaborator

I didn't say it shouldnt be that way. But it complicates things a little. So maybe it's enough for another PR.

src/Symfony/Component/Routing/Loader/AnnotationClassLoader.php
... ...
@@ -97,10 +97,11 @@ public function load($class, $type = null)
97 97
         }
98 98
 
99 99
         $globals = array(
100  
-            'pattern'      => '',
101  
-            'requirements' => array(),
102  
-            'options'      => array(),
103  
-            'defaults'     => array(),
  100
+            'pattern'          => '',
  101
+            'requirements'     => array(),
  102
+            'options'          => array(),
  103
+            'defaults'         => array(),
  104
+            'hostname_pattern' => null,
1
Christophe Coevoet Collaborator
stof added a note November 05, 2012

Why defaulting to null here whereas the XmlFileLoader always sets a string ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
((37 lines not shown))
116 142
     }
117 143
 
118 144
     /**
119 145
      * Generates PHP code recursively to match a tree of routes
120 146
      *
121  
-     * @param DumperPrefixCollection $routes A DumperPrefixCollection instance
  147
+     * @param DumperPrefixCollection $routes               A DumperPrefixCollection instance
122 148
      * @param Boolean                $supportsRedirections Whether redirections are supported by the base class
123 149
      * @parma string                 $prefix Prefix of the parent collection
1
Christophe Coevoet Collaborator
stof added a note November 05, 2012

could you fix this line too (@param and the alignment of the description) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/Matcher/UrlMatcher.php
... ...
@@ -142,7 +147,7 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
142 147
                 continue;
143 148
             }
144 149
 
145  
-            return array_merge($this->mergeDefaults($matches, $route->getDefaults()), array('_route' => $name));
  150
+            return array_merge($this->mergeDefaults($hostnameMatches + $matches, $route->getDefaults()), array('_route' => $name));
1
Christophe Coevoet Collaborator
stof added a note November 05, 2012

AFAIK, we don't use + anywhere in the Symfony codebase to merge arrays. Could you use array_merge or array_replace ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/RouteCollection.php
((15 lines not shown))
191 194
      *
192 195
      * @throws \InvalidArgumentException When the RouteCollection already exists in the tree
193 196
      *
194 197
      * @api
195 198
      */
196  
-    public function addCollection(RouteCollection $collection, $prefix = '', $defaults = array(), $requirements = array(), $options = array())
  199
+    public function addCollection(RouteCollection $collection, $prefix = '', $defaults = array(), $requirements = array(), $options = array(), $hostnamePrefix = null)
4
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

The name hostnamePrefix is now even worse because it's not the prefix of the hostname or something. Then lets rather stay with hostnamePattern. Anyway, as mentioned earlier, it's not good that the hostname is only set one some routes, whereas the defaults and requirements on all. This can lead to some unintentionel behavior, like when the requirement is overriden for a route, but the hostname not.
IMO it would be best to remove this param from this method and instead use the setHostnamePattern for it, which already does the same thing.
Upgrade it to setHostnamePattern($pattern, $defaults, $requirements, $override = false).
With $override = true it will override the pattern, defaults and requirementone for sub-routes that already have a hostname pattern set. When its false, it will only set them on routes that have no hostname pattern yet.

I've renamed back to hostnamePattern and made the call to setHostnamePattern unconditional in addCollection

Tobias Schultze Collaborator
Tobion added a note November 05, 2012

You didnt fix the real issue. With ur current implementation, the hostname pattern is still only set on some routes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/RouteCollection.php
... ...
@@ -259,6 +265,23 @@ public function getPrefix()
259 265
         return $this->prefix;
260 266
     }
261 267
 
  268
+    public function getHostnamePattern()
  269
+    {
  270
+        return $this->hostnamePattern;
  271
+    }
  272
+
  273
+    public function setHostnamePattern($pattern)
  274
+    {
  275
+        $this->hostnamePattern = $pattern;
1
Tobias Schultze Collaborator
Tobion added a note November 05, 2012

'' should probably be treated as null. Also missing phpdoc with explanation how this methods behaves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Routing/RouteCollection.php
((8 lines not shown))
  273
+    public function getHostnamePattern()
  274
+    {
  275
+        return $this->hostnamePattern;
  276
+    }
  277
+
  278
+    /**
  279
+     * Sets the hostname pattern on this collection and all children.
  280
+     *
  281
+     * @param string $pattern The pattern
  282
+     */
  283
+    public function setHostnamePattern($pattern)
  284
+    {
  285
+        $this->hostnamePattern = $pattern;
  286
+
  287
+        if ('' === $pattern) {
  288
+            return;
7
Tobias Schultze Collaborator
Tobion added a note November 11, 2012

You must still iterate, you would not be able to reset the hostname pattern for children with this method. Also $this->hostnamePattern != $route->hostnamePattern. So this condition must be removed IMO. And you should also cast to string above now that you made '' the default: $this->hostnamePattern = (string) $pattern; (Done)

Tobias Schultze Collaborator
Tobion added a note November 12, 2012

When this is resolved, I think we're good to merge.

@Tobion I'm not sure to understand what behavior you want exactly, given previous changes; could you do a PR for this ?

Tobias Schultze Collaborator
Tobion added a note November 12, 2012

I mean, setting a hostname on a collection should also set that hostname on all routes. I don't see why you included if ('' === $pattern) return;. If pattern is empty, $this->hostnamePattern === '' but the routes still have their previous hostname.

Tobias Schultze Collaborator
Tobion added a note November 12, 2012

Ah, I know why. Ill make a PR too improve this.

Tobias Schultze Collaborator
Tobion added a note November 12, 2012

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Tobias Schultze Tobion commented on the diff November 11, 2012
src/Symfony/Component/Routing/Matcher/Dumper/DumperCollection.php
... ...
@@ -98,4 +98,50 @@ protected function setParent(DumperCollection $parent)
98 98
     {
99 99
         $this->parent = $parent;
100 100
     }
  101
+
  102
+    /**
  103
+     * Returns true if the attribute is defined.
  104
+     *
  105
+     * @param string $name The attribute name
  106
+     *
  107
+     * @return Boolean true if the attribute is defined, false otherwise
  108
+     */
  109
+    public function hasAttribute($name)
  110
+    {
  111
+        return array_key_exists($name, $this->attributes);
4
Tobias Schultze Collaborator
Tobion added a note November 11, 2012

You didnt define $this->attributes

Tobias Schultze Collaborator
Tobion added a note November 12, 2012

Yes, that what I fixed in the PR ^^

Ho :) Thanks! I think the declaration disappeared during some rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Bundle/FrameworkBundle/Command/RouterDebugCommand.php
... ...
@@ -112,7 +112,9 @@ protected function outputRoutes(OutputInterface $output, $routes = null)
112 112
                     ? implode(', ', $requirements['_method']) : $requirements['_method']
113 113
                 )
114 114
                 : 'ANY';
115  
-            $output->writeln(sprintf($format, $name, $method, $route->getPattern()));
  115
+            $hostname = null !== $route->getHostnamePattern()
1
Tobias Schultze Collaborator
Tobion added a note November 11, 2012

it's '' now, not null

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

@arnaud-lb see arnaud-lb/symfony#5 It also fixes the problems I mentioned above (except comment #3378 (comment) because is make some test fail, but the current behavior seems wrong).

I added a fix for PhpMatcherDumper that is basically unrelated to this PR. But doing it separately would require one of the PRs, to be heavily rebased. The problem was that preg_match returns also numeric indexes for the placeholders. They were not filtered when using the dumped matcher and no defaults were specified. So UrlMatcher::mergeDefaults must be called still. Otherwise the matcher returns too many params.
A test for it should probably be added later on. The dumped matcher is not tested at all for functionality yet, only the expected source code is compared. But not the bahavior at all.

Fabien Potencier
Owner

@Tobion: Do you see any other issues before I merge?

Tobias Schultze
Collaborator

@fabpot just one issue is unresolved: #3378 (comment)

Mateusz Gajewski

Do you need some help? This is exact feature we're looking for!

Fabien Potencier
Owner

@Tobion: Your reference does not exist anymore

Fabien Potencier
Owner

@wendigo Thanks but we are almost done now. This should be merged really soon.

Fabien Potencier
Owner

@wendigo But there is one thing that would definitely be helpful: try to use this feature in your code and give us some feedback.

Tobias Schultze
Collaborator

@fabpot: It's the discussion on on outdated diff at 2012-11-11T06:56:27-08:00

Tobias Schultze
Collaborator

@fabpot now it's ready to be merged.

Moritz Kraft

Awesome. This is like Christmas if Santa brought code instead of presents.

Jordi Boggiano
Collaborator

Not sure if the sample at the top is up to date (didn't read the gazillion comments) but can I ask why hostname_pattern vs just hostname or even better _hostname in requirements? It just seems a bit weirdly inconsistent, but maybe there is a good reason I missed.

Arnaud Le Blanc

@Seldaek the hostname pattern allows {placeholders}, which work exactly like the pattern's placeholders, can have requirements too, etc. I though hostname_pattern would make it more obvious that it works like pattern, but for the hostname.

Jordi Boggiano
Collaborator

I see, then I agree it might not belong in requirements, but still not sure hostname vs hostname_pattern.. The _pattern surely didn't hint at anything for me, but maybe that's my bad :)

Tobias Schultze
Collaborator

I also raised this question. But since it's pattern for the path, I'm fine with the suffix in hostname_pattern for the host.
It's the best consistency we can get. Too bad, pattern is not named path or path_pattern. This would be much better, but I guess we cannot change it anymore. Or @fabpot, would you accept deprecating pattern in favor of path in 2.2? And removing it in 2.3 so we have a clean naming in LTS?

Fabien Potencier
Owner

@Tobion: I've created an issue for the renaming: #5989

Mario A. Alvarez Garcia

@fabpot Are you going to wait for the discussion on that issue before merging this?

Fabien Potencier fabpot referenced this pull request from a commit November 12, 2012
Fabien Potencier 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
Fabien Potencier fabpot merged commit 17f51a1 into from November 12, 2012
Fabien Potencier fabpot closed this November 12, 2012
Fabien Potencier
Owner

Merged! Thanks you very much to everyone who participated in this PR, especially @arnaud-lb and @Tobion.

Enjoy!

Arnaud Le Blanc

Thanks!

Florin Patan

Thank you for this awesome feature @arnaud-lb @Tobion and everyone else!

Patrik Patie Gmitter

THANKS!

Evan Owens nurikabe referenced this pull request in FriendsOfSymfony/FOSUserBundle November 12, 2012
Open

How to approach multi-tenancy with FOSUserBundle #413

bitomule

Any chance I can use this feature on symfony 2.1? I really need this feature and can't wait for 2.2.

I want to change this:

domain.com/username (my current route)

to:

username.domain.com (how I want routing to work)

Of course I've a wildcard on my dns so all the traffic from subdomains goes to the same server. That works.

Thanks!

Philipp Hoffmann

@bitomule: you can use apache redirects (RewriteCond and RewriteRule) in the mean time to get this accomplished

bitomule

I'm trying to use apache redirects, but that changes my browser url. This are my apache conditions and rules:

RewriteCond %{HTTP_HOST} !^www\.mydomain\.net$
RewriteCond %{HTTP_HOST} ^(.*)\.mydomain\.net$

RewriteRule ^(.*)$ http://mydomain.net\/%1 [QSA,L]

Thanks!

Chris Lush

@bitomule: and this RewriteRule isn't being matched?

Philipp Hoffmann

try something like this:

RewriteCond %{HTTP_HOST} (.+)\.domain\.com
RewriteRule ^(.+)$ /%1/$1 [L]
bitomule

The RewriteRule is matched, but it changes browser url, I don't want that url to change.

@philipphoffmann , yours doesn't even work :(

Philipp Hoffmann

try replacing the + in RewriteRule with *, like this:

RewriteRule ^(.*)$ /%1/$1 [L]
Dmytro Chekaliuk

@bitomule , you're making rewrite to an absolute URL with another hostname, that's why Apache performs an external redirect. To avoid this, use an URL-path, or try the proxy flag:

RewriteCond %{HTTP_HOST} ^([^\.]++)\.mydomain\.net$
RewriteCond %1 !=www
RewriteRule .* /%1/$0 [QSA,L]
# or
# RewriteRule .* http://mydomain.net/%1/$0 [QSA,P]
bitomule

Thanks for all your help, but it doesn't work :(

I've tryed @philipphoffmann version and both @lazyhammer options so maybe my problem is in other part of the virtual host, so I paste the full virtualhost configuration.

This is my virtualhost configuration, the one that works but changes url:

<VirtualHost *:80>
ServerName mydomain.net
ServerAlias *.mydomain.net

DocumentRoot /var/www/prod/web
ServerAdmin webmaster@mydomain.com

<Directory "/var/www/prod/web">
   DirectoryIndex app.php
   AllowOverride All
   Order allow,deny
   Allow from all
   RewriteEngine On

   #Remove final slash
RewriteRule ^(.+[^/])/$ http://%{HTTP_HOST}/$1 [R=301]


  RewriteCond %{REQUEST_FILENAME} !-f
  RewriteCond %{REQUEST_URI} !^/(media|skin|js|css)/
  RewriteCond %{HTTP_HOST} !^www\.mydomain\.net$
  RewriteCond %{HTTP_HOST} ^(.*)\.mydomain\.net$

  RewriteRule ^(.*)$ http://mydomain.net\/%1 [QSA,L]

  RewriteCond %{REQUEST_FILENAME} !-f
  RewriteRule ^(.*)$ app.php [QSA,L]
  RedirectMatch permanent ^/app\.php/(.*) /$1

</Directory>

bitomule

I'm finally waiting for symfony 2.2. When would symfony 2.2 be released? How can I update to beta? Should I?

Thanks!

Ueli Banholzer

Is there a plan to use the hostname in the Symfony2 Firewall (security.yml)?

Example:

security:
    firewalls:
        example:
          pattern: ^/example
          host: ^user.example.com$

@bitomule: Symfony 2.2 will be released at the end of February 2013 (http://symfony.com/doc/current/contributing/community/releases.html#schedule). Update your requirements in the composer.json to 2.2 and run composer update to upgrade (see: https://github.com/symfony/symfony-standard/blob/master/composer.json)

mmucklo mmucklo referenced this pull request from a commit October 19, 2012
Fabien Potencier merged branch arnaud-lb/routing-php-dumper-simplification (PR #5734)
This PR was merged into the master branch.

Commits
-------

e54d749 [Routing] Simplified php matcher dumper (and optimized generated matcher)

Discussion
----------

[Routing] Simplified php matcher dumper (and optimized generated matcher)

Bug fix: no
Feature addition: no
Related: #3378
Backwards compatibility break: no
Symfony2 tests pass: yes

This simplifies the php matcher dumper by allowing the dumper to re-organize routes in the dumper's own structure.

As a result, dumping is made a little simpler. This is also helps much for my hostname-based routes PR #3378.

Reorganizing routes also allows to find more optimization opportunities:

Currently the dumper wraps some collections of routes in a `if (0 === strpos($pathinfo, '/someprefix')` if the collection has user-defined prefix, and if it contains more than one direct child Route. This can miss many optimization opportunities.

The PR changes this by building a prefix tree of routes based on the static prefix extracted from routes' patterns. Then every leave having a prefix and more than one child (route or collection) will be wrapped in a `if` statement.

Example:

```
// No explicit prefix is specified
@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"
}
```

Some tests have many white space changes, much more easier to review [here](https://github.com/symfony/symfony/pull/5734/files?w=1)

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

by Tobion at 2012-10-13T02:27:54Z

I'm not sure if adding these specific classes just for dumping is the best implementation because they duplicate some logic and this optimization should also work out-of-the-box with the standard RouteCollection etc.
What I have in mind is a new method in RouteCollection like `RouteCollection::optimizeTree` that returns a new RouteCollection with the Routes that includes these optimization you do here. So there would probably be no need for the new classes.

It think it requires some changes in RouteCollection like the handling of prefix that must start with a slash currently, which is too restrictive. But it should be possible.

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

by arnaud-lb at 2012-10-13T12:52:32Z

@Tobion

> I'm not sure if adding these specific classes just for dumping is the best implementation because they duplicate some logic and this optimization should also work out-of-the-box with the standard RouteCollection etc.

I think RouteCollection and DumperCollection do not share the same concerns; and RouteCollection does things that don't allow to reorganize routes freely. For instance when adding a collection to a RouteCollection this changes all the child routes' prefix, requirements, options, etc. When setting a collection's prefix, this prepends the prefix to every child route's pattern, etc.

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

by arnaud-lb at 2012-10-15T08:50:23Z

squashed the CS commits

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

by arnaud-lb at 2012-10-15T13:50:16Z

@fabpot @Tobion this PR is ready to be merged if everyone agrees

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

by Tobion at 2012-10-16T18:10:36Z

When the above is fixed, I think it's good to be merged.

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

by arnaud-lb at 2012-10-17T08:40:20Z

Fixed; thanks @Tobion @stof for your reviews

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

by Tobion at 2012-10-19T03:30:10Z

@arnaud-lb could you please test whether your PR fixes #5780 as a side-effect?
I can image that it's fixed because you use `$route->compile()->getStaticPrefix();` for prefix optimization.
If it's fixed please add a test case. If not, that's fine, and we need to fix it in another PR.
1b1c7d9
mmucklo mmucklo referenced this pull request from a commit November 12, 2012
Fabien Potencier 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

Showing 1 unique commit by 1 author.

Nov 12, 2012
Arnaud Le Blanc Merge pull request #6 from Tobion/hostname-routes
fix API of RouteCollection
17f51a1
This page is out of date. Refresh to see the latest.

Showing 0 changed files with 0 additions and 0 deletions. Show diff stats Hide diff stats

Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.