Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Routing] Overriding route puts route on end of collection. #24748

Closed
mimol91 opened this issue Oct 29, 2017 · 8 comments
Closed

[Routing] Overriding route puts route on end of collection. #24748

mimol91 opened this issue Oct 29, 2017 · 8 comments

Comments

@mimol91
Copy link

mimol91 commented Oct 29, 2017

Q A
Bug report? no
Feature request? yes?
BC Break report? no
RFC? no
Symfony version all

I am working on application which define different route parameters depending on subdomain (for simplicity lets call them languages)

For example there are en, pl, gb sub domains. Which may override some global routes (defined in domain routing)

#routing.yml
_domain:
    resource: routing_domain.yml

_subdomain:
    resource: routing_subdomain.yml
#routing_domain.yml
app_default_index:
    path:     /{language}/index
    defaults: { _controller: AppBundle:Default:index }
    methods: ['GET']
    requirements:
        language: 'en'

app_default_catch_all:
    path:     /{language}/{slug}
    defaults: { _controller: AppBundle:Default:catchAll }
    methods: ['GET']
#routing_subdomain.yml
app_default_index:
    path:     /{language}/index
    defaults: { _controller: AppBundle:Default:index }
    methods: ['GET']
    requirements:
        language: 'en|gb'

Because of priority of sub domain routes it has to be imported last (To override app_default_index from routing_domain.yaml)

Problem with this approach is that it puts app_default_index on the bottom of routes priority, causing that app_default_catch_all is before app_default_index.

 ----------------------- -------- -------- ------ -------------------- 
  Name                    Method   Scheme   Host   Path                
 ----------------------- -------- -------- ------ -------------------- 
  app_default_catch_all   GET      ANY      ANY    /{language}/{slug}  
  app_default_index       GET      ANY      ANY    /{language}/index   
 ----------------------- -------- -------- ------ -------------------- 

It there any way that I am able to override app_default_index route, but keep its initial position?
I've found an place where this logic is set https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/RouteCollection.php#L125

// we need to remove all routes with the same names first because just replacing them
// would not place the new route at the end of the merged array.

What is an reason that route should be put on end of array? How can I solve this problem, without using dynamic route loader and keep everything as simple as possible?

@sroze
Copy link
Contributor

sroze commented Oct 31, 2017

@mimol91 FYI, the lines of code you refer to have been introduced by @Tobion in the following PR: #6120. Maybe he has more context :)

@Tobion
Copy link
Contributor

Tobion commented Oct 31, 2017

It has always been like this even before #6120. Changing this would be a BC break. And I don't see how to make this easily configurable. @mimol91 can't you just redefine app_default_catch_all at the end as well, so that it is put at the end?

@mimol91
Copy link
Author

mimol91 commented Oct 31, 2017

I just want to show simple example. Of course I could do it how you suggest, but it will require to copy more than one hundred routes. - I think it can be solved by creating some custom route loader.

Maybe just by adding an 'replace' as route option will be enough? Similar like utf-8 ? (In some future if we agree that there is more benefits to keep route position instead moving it on end set it as a default behavior?)

@Tobion
Copy link
Contributor

Tobion commented Oct 31, 2017

Sure a custom route loader can solve that. I don't know how you create the routes with annotation, yaml, php generated. But can't you identify your catch-all routes and import them last? So after you overwrote the others.

@mimol91
Copy link
Author

mimol91 commented Nov 1, 2017

I m using dynamic loader (which dynamically read yaml files). I think it would be possible (There should be few catch-all routes). However its not the best solution, so I will try add replace option I think it should be possible to remember route positions read collection, and then 'sort' it.

What do You think about making this option available in Route? If I am the first one with such problem I guess it will not be good idea, Do you know anybody else with similar problem?

@nicolas-grekas
Copy link
Member

Since this issue is about i18n, and we have a PR on the topic in #26143, is there any alternative based on that PR that would allow achieving what you want @mimol91 in the end, without resorting to overriding anything?

@mimol91
Copy link
Author

mimol91 commented Feb 23, 2018

@nicolas-grekas PR that you mention 'works' only for _locale in example which I've used it is different pattern for subdomain route

To illustrate it I will give more complex example.
Lets assume that its an 'github' clone project. Its 'global' - but has features which are enabled only for specific country. (For simplification lets assume that currently its available only in:

  • United Kingdom
  • France
  • Poland
  • Germany

Because of different features set for each of country: applications read Country env variable and loads specific config file. (For UK it will be config_uk.yaml) - which imports config.yaml (because some of parts are common - to avoid duplication its better to keep them in one place instead of splitting configuration four different config files). This same approach is with routing.

So in routing.yml (common)

repository_details:
    path:     /{repositoryId}/details
    defaults: { _controller: AppBundle:Default:index }
    methods: ['GET']
    requirements:
        repositoryId: \d+

repository_slug:
    path:     /{repositoryId}/{slug}
    defaults: { _controller: AppBundle:Default:catchAll }
    methods: ['GET']
    requirements:
        repositoryId: \d+

However only for France repository_details is changed a little

repository_details:
    path:     /{repositoryId}/détails   --Notice é instead of e
    defaults: { _controller: AppBundle:Default:index }
    methods: ['GET']
    requirements:
        repositoryId: \d+

Of course I can define all repository_details separately for each of country instead of putting them in common part.

//
It does not have to be related anyhow with countries. It can be as well used in e-commerce platform. Where clients buy an 'shop platform' to sell theirs products, - Most of the configuration will be common for all of the clients, - but some 'Premium client' can have some special requirements (for example 'clientA.shop-platform.com/contactUs' instead of 'anyOtherClient.shop-platform.com/contact-us'.

I am aware that it can be done by 'generating' from template separate routing file (using Ansible / any other tool) -so there will not be any 'common' routing.

I just was not sure why while overriding route definition new route does not keep 'previous position'.
I think possibility to define route priority (Mentioned in #26132 ) could solve this problem in nice manner

@nicolas-grekas
Copy link
Member

I'm closing as I think the current ongoing PRs have the potential to provide an alternative solution for this, and because changing the current logic would be a BC break anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants