Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Closed

Conversation

@weaverryan
Copy link
Member

QA
Bug fix?behavior change
New feature?behavior change
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRn/a

Based on conversation starting here:#15990 (comment).

// Before:$routes->mount('/admin',$routes->import(__DIR__.'/config/admin.yml');// After:$routes->import(__DIR__.'/config/admin.yml','/admin');

This makesimport() actually add theRouteCollectionBuilder into 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

string|null

@weaverryan
Copy link
MemberAuthor

Ping @symfony/deciders - this is totally subjective, but we should decide either way.

@henrikbjorn
Copy link
Contributor

Would it be possible to changeload to be public and return a new builder instance? That way it would support both type of usage (mutable,immutable)

@weaverryan
Copy link
MemberAuthor

@henrikbjorn Theimport methoddoes return the newRouteCollectionBuilder - so you could still mutate it after it's been added:

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

@weaverryan almost, i can think of cases where i want the "old" behavior where it isnt automatically added. That is what i wantload() to do, and have import be load & mount.

No problem :)

@weaverryan
Copy link
MemberAuthor

@henrikbjorn What's your use-case for that? Obviously makingload() public is trivial, but we want to keep the public API as small as possible.

@henrikbjorn
Copy link
Contributor

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
Copy link
MemberAuthor

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

The change currently does exactly what is was assuming it would do eg$builder->import('@MuseBundle/Resources/routing/routing.xml') 👍

@GromNaN
Copy link
Member

I see an inconsistency in method signatures:

function import($resource,$prefix ='/',$type =null)function mount($prefix,RouteCollectionBuilder$builder)

I perceive$resource and$builder as similar things in this context. They both define a set of routes.

@weaverryan
Copy link
MemberAuthor

@GromNaN I don't understand, what would you change?

@weaverryan
Copy link
MemberAuthor

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

@weaverryan he wants the $builder to be the first argument to mount, which makes sense to me.

@henrikbjorn
Copy link
Contributor

👍

@weaverryan
Copy link
MemberAuthor

I'd still like to see this merged - as I was just thinking howimport() for all other loaders (e.g.YamlFileLoader) actually imports the configuration, whereas here it really just "loads" and returns it.

@symfony/deciders can we either close or merge this?

@DavidBadura
Copy link
Contributor

This change makes it more understandable what happens 👍

@fabpot
Copy link
Member

Thank you@weaverryan.

This was referencedNov 30, 2015
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@weaverryan@henrikbjorn@GromNaN@DavidBadura@fabpot@Tobion@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp