[Routing] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader #32582
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fancyweb
commented
Jul 17, 2019
fancyweb
commented
Jul 17, 2019
3f70fa0 to
c21dc3f
Compare
src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoader.php
Outdated
Show resolved
Hide resolved
chalasr
reviewed
Jul 17, 2019
2593241 to
90c0d7f
Compare
fancyweb
commented
Jul 18, 2019
90c0d7f to
3d68427
Compare
src/Symfony/Component/Routing/Tests/Fixtures/TestObjectRouteLoader.php
Outdated
Show resolved
Hide resolved
nicolas-grekas
approved these changes
Jul 18, 2019
3d68427 to
234fe4b
Compare
fancyweb
commented
Jul 18, 2019
src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml
Outdated
Show resolved
Hide resolved
234fe4b to
d0b5ec1
Compare
nicolas-grekas
approved these changes
Jul 19, 2019
…r of ContainerLoader and ObjectLoader
d0b5ec1 to
1548101
Compare
|
Thank you @fancyweb. |
nicolas-grekas
added a commit
that referenced
this pull request
Jul 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. ### ServiceRouterLoader 1. 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. ### 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 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
fabpot
added a commit
that referenced
this pull request
Jul 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 #32582 Commits ------- 4dc9ff6 [Routing][FrameworkBundle] Remove deprecated ServiceRouterLoader, ObjectRouteLoader and routing.loader.service
fabpot
added a commit
that referenced
this pull request
Aug 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#11337 Continuation of #30926. ~Please review only the 2nd commit, I'm building this on top of #32582 Commits ------- 64aa2c8 [FrameworkBundle][Routing] Private service route loaders
symfony-splitter
pushed a commit
to symfony/framework-bundle
that referenced
this pull request
Aug 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#11337 Continuation of symfony/symfony#30926. ~Please review only the 2nd commit, I'm building this on top of symfony/symfony#32582 Commits ------- 64aa2c8529 [FrameworkBundle][Routing] Private service route loaders
This was referenced Nov 12, 2019
Merged
Merged
fabpot
added a commit
that referenced
this pull request
Nov 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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims at deprecating some things to have a more consistent code.
ServiceRouterLoader
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 to
ContainerRouteLoader. Actually I think it's better to rename it toContainerLoadersince all others route loaders does not have "Route" in their name. Let's be consistent!This class is in a
DependencyInjectionsub directory for historical reasons. Let's remove that! It accepts any PSR-11 container.ObjectRouteLoader
This class has "Route" in its name too. Let's rename it!
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
supportsmethod from it to let extending classes implement it.IMHO, this abstract implementation is useless. We sould just deprecate the whole class and move the implemention in the
ContainerLoaderclass.