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] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader#32582

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

Merged

Conversation

@fancyweb
Copy link
Contributor

QA
Branch?4.4
Bug fix?no
New feature?no
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets#30926 (comment)
LicenseMIT
Doc PR-

This PR aims at deprecating some things to have a more consistent code.

ServiceRouterLoader

  1. This class actually fetches an object from a container. In[FrameworkBundle][Routing] Add a new tag to be able to use a private service as a service route loader #30926 (comment), it was suggested that it should be renamed toContainerRouteLoader. Actually I think it's better to rename it toContainerLoader since all others route loaders does not have "Route" in their name. Let's be consistent!

  2. This class is in aDependencyInjection sub directory for historical reasons. Let's remove that! It accepts any PSR-11 container.

ObjectRouteLoader

  1. This class has "Route" in its name too. Let's rename it!

  2. This class is supposed to be an abstract implementation for "object" loaders to reuse, but in its code it has a lot of references to "services". Let's remove those references! That means renaming some methods, altering messages, etc.. That also means removing thesupports method from it to let extending classes implement it.

  3. IMHO, this abstract implementation is useless. We sould just deprecate the whole class and move the implemention in theContainerLoader class.

@fancywebfancywebforce-pushed thedeprecate-service-router-loader branch 4 times, most recently from2593241 to90c0d7fCompareJuly 18, 2019 09:32
@fancywebfancywebforce-pushed thedeprecate-service-router-loader branch from3d68427 to234fe4bCompareJuly 18, 2019 16:04
@fancywebfancywebforce-pushed thedeprecate-service-router-loader branch from234fe4b tod0b5ec1CompareJuly 19, 2019 07:59
Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

with minor comment

@fancywebfancywebforce-pushed thedeprecate-service-router-loader branch fromd0b5ec1 to1548101CompareJuly 22, 2019 21:07
@nicolas-grekas
Copy link
Member

Thank you@fancyweb.

nicolas-grekas added a commit that referenced this pull requestJul 23, 2019
…eLoader in favor of ContainerLoader and ObjectLoader (fancyweb)This PR was merged into the 4.4 branch.Discussion----------[Routing] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |#30926 (comment)| License       | MIT| Doc PR        | -This PR aims at deprecating some things to have a more consistent code.### ServiceRouterLoader1. This class actually fetches an object from a container. In#30926 (comment), it was suggested that it should be renamed to `ContainerRouteLoader`. Actually I think it's better to rename it to `ContainerLoader` since all others route loaders does not have "Route" in their name. Let's be consistent!2. This class is in a `DependencyInjection` sub directory for historical reasons. Let's remove that! It accepts any PSR-11 container.### ObjectRouteLoader1. This class has "Route" in its name too. Let's rename it!2. This class is supposed to be an abstract implementation for "object" loaders to reuse, but in its code it has a lot of references to "services". Let's remove those references! That means renaming some methods, altering messages, etc.. That also means removing the `supports` method from it to let extending classes implement it.3. IMHO, this abstract implementation is useless. We sould just deprecate the whole class and move the implemention in the `ContainerLoader` class.Commits-------1548101 [Routing] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader
@fancywebfancyweb deleted the deprecate-service-router-loader branchJuly 23, 2019 12:08
fabpot added a commit that referenced this pull requestJul 24, 2019
…erLoader, ObjectRouteLoader and routing.loader.service (fancyweb)This PR was merged into the 5.0-dev branch.Discussion----------[Routing][FrameworkBundle] Remove deprecated ServiceRouterLoader, ObjectRouteLoader and routing.loader.service| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | yes| Deprecations? | no| Tests pass?   | yes| Fixed tickets | è| License       | MIT| Doc PR        | -Remove deprecations from#32582Commits-------4dc9ff6 [Routing][FrameworkBundle] Remove deprecated ServiceRouterLoader, ObjectRouteLoader and routing.loader.service
fabpot added a commit that referenced this pull requestAug 9, 2019
…rs (fancyweb)This PR was merged into the 4.4 branch.Discussion----------[FrameworkBundle][Routing] Private service route loaders| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#30402| License       | MIT| Doc PR        |symfony/symfony-docs#11337Continuation of#30926.~Please review only the 2nd commit, I'm building this on top of#32582Commits-------64aa2c8 [FrameworkBundle][Routing] Private service route loaders
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull requestAug 9, 2019
…rs (fancyweb)This PR was merged into the 4.4 branch.Discussion----------[FrameworkBundle][Routing] Private service route loaders| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |symfony/symfony#30402| License       | MIT| Doc PR        |symfony/symfony-docs#11337Continuation ofsymfony/symfony#30926.~Please review only the 2nd commit, I'm building this on top ofsymfony/symfony#32582Commits-------64aa2c8529 [FrameworkBundle][Routing] Private service route loaders
@nicolas-grekasnicolas-grekas modified the milestones:next,4.4Oct 27, 2019
This was referencedNov 12, 2019
fabpot added a commit that referenced this pull requestNov 26, 2019
… loaders (fancyweb)This PR was merged into the 4.4 branch.Discussion----------[Routing] Continue supporting single colon in object route loaders| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       |#34612| License       | MIT| Doc PR        | -#32582 (comment) was a bad idea. The new `ObjectLoader` class is used directly on 4.4 since we detagged the old service (and the old one). So we need to support the old notation on it. It changes the exception message but it should be alright.Commits-------3c796e1 [Routing] Continue supporting single colon in object route loaders
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrchalasr approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

6 participants

@fancyweb@nicolas-grekas@stof@chalasr@yceruto@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp