Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] Changing RouteCollectionBuilder::import() behavior to add to the builder#16477
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
string|null
weaverryan commentedNov 9, 2015
Ping @symfony/deciders - this is totally subjective, but we should decide either way. |
henrikbjorn commentedNov 9, 2015
Would it be possible to change |
weaverryan commentedNov 9, 2015
@henrikbjorn The $adminRoutes =$routes->import(__DIR__.'/config/admin.yml','/admin');$adminRoutes->setDefault('_locale','fr'); Is that what you're after? Btw, I saw yourMuse test app - thanks for testing things. |
henrikbjorn commentedNov 9, 2015
@weaverryan almost, i can think of cases where i want the "old" behavior where it isnt automatically added. That is what i want No problem :) |
weaverryan commentedNov 9, 2015
@henrikbjorn What's your use-case for that? Obviously making |
henrikbjorn commentedNov 10, 2015
If i wanted to build something using this class by itself then it would be a good idea to have a immutable option of loading routes. But maybe i am grasping for straws (thats a thing right?) |
weaverryan commentedNov 10, 2015
@henrikbjorn Ha, it is a thing ;). I still don't quite understand the mutable versus immutable thing and what use-case you're thinking of. But not necessarily because what you're asking doesn't make sense - I may just need more info. Can you give a code example of what you might want to accomplish that isn't possible/easy with the current interface? Would the change in this PR improve things, or not matter? |
henrikbjorn commentedNov 11, 2015
The change currently does exactly what is was assuming it would do eg |
GromNaN commentedNov 11, 2015
I see an inconsistency in method signatures: function import($resource,$prefix ='/',$type =null)function mount($prefix,RouteCollectionBuilder$builder) I perceive |
weaverryan commentedNov 11, 2015
@GromNaN I don't understand, what would you change? |
weaverryan commentedNov 11, 2015
Btw, if we're going to merge this or make any other changes... it needs to happen... like yesterday :). So any constructive 👍 , 👎 or specific changes would be ideal |
henrikbjorn commentedNov 11, 2015
@weaverryan he wants the $builder to be the first argument to mount, which makes sense to me. |
henrikbjorn commentedNov 11, 2015
👍 |
weaverryan commentedNov 19, 2015
I'd still like to see this merged - as I was just thinking how @symfony/deciders can we either close or merge this? |
DavidBadura commentedNov 23, 2015
This change makes it more understandable what happens 👍 |
fabpot commentedNov 23, 2015
Thank you@weaverryan. |
Based on conversation starting here:#15990 (comment).
This makes
import()actually add theRouteCollectionBuilderinto itself. We didn't do this before at Fabien's request, and actually the current implementation (before this PR) is quite "clean". However, I agree with@wouterj thatimport()really sounds/looks like it will actuallyimport those routesinto thisRouteCollectionBuilder.This change is subjective - we just need to pick which way we like better and run full steam with it.