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

[FrameworkBundle][Routing] Add a new tag to be able to use a private service as a service route loader#30926

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

Conversation

@fancyweb
Copy link
Contributor

@fancywebfancyweb commentedApr 6, 2019•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets#30402
LicenseMIT
Doc PRsymfony/symfony-docs#11337

#eufossa

This PR adds a new tagrouting.route_loader autoconfigured thanks to a newServiceRouterLoader interface to be able to use private route loader services.

The deprecation layer is done through a temporary container that will be removed in 5.0.

TODO :

  • Changelog and upgrade entry
  • Doc PR

sstok reacted with thumbs up emoji
@fancywebfancywebforce-pushed theservice-router-loader-private-service- branch 3 times, most recently frome05abac tod3a2998CompareApril 6, 2019 19:55
@chalasrchalasr added this to thenext milestoneApr 7, 2019
@fancywebfancywebforce-pushed theservice-router-loader-private-service- branch fromd3a2998 to93584ebCompareApril 7, 2019 08:52
@chalasrchalasr self-requested a reviewApril 7, 2019 10:17
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

changelog+upgrade files need an update also

@fancyweb
Copy link
ContributorAuthor

fancyweb commentedApr 7, 2019•
edited
Loading

@nicolas-grekas I added the UPGRADE entries already.

For the CHANGELOG of the Routing component there is nothing to say IMO. We added a marker interface, an instantly deprecated / internal class, and a compiler pass used by the Framework Bundle 😕

@javiereguiluz
Copy link
Member

Ping@fancyweb. Will you have some time to make the last requested changes? It'd be nice to have this for Symfony 4.3. Thanks!

@fancyweb
Copy link
ContributorAuthor

I will definitely finish this work before the end of the month.

@fancywebfancywebforce-pushed theservice-router-loader-private-service- branch 2 times, most recently from3a30564 to9c5e31aCompareApril 29, 2019 16:43
<serviceid="routing.loader.service"class="Symfony\Component\Routing\Loader\DependencyInjection\ServiceRouterLoader">
<tagname="routing.loader" />
<argumenttype="service"id="service_container" />
<argumenttype="service"id="routing.loader.service.container" />
Copy link
ContributorAuthor

@fancywebfancywebApr 29, 2019•
edited
Loading

Choose a reason for hiding this comment

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

In 5.0, replace this argument with the above tagged locator.

@chalasr
Copy link
Member

Although I findroute loader more clear, naming should be consistent between the interface and the tag (RouterLoader vsRouteLoader). We should stick with the current naming and rename the tag torouting.router_loader IMHO

fancyweb reacted with thumbs up emoji

@fancywebfancywebforce-pushed theservice-router-loader-private-service- branch from9c5e31a to0bb19b7CompareMay 5, 2019 20:22
options have been deprecated.
* Implementing `Serializable` for `Route` and `CompiledRoute` is deprecated; if you serialize them, please
ensure your unserialization logic can recover from a failure related to an updated serialization format
* Not tagging the route loader services with `routing.router_loader` has been deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

route loader vsrouter_loader

Copy link
Member

Choose a reason for hiding this comment

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

I think router loader is way better.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

In this case, if the "public" name is now router loader, I guess we should update the whole documentation page (https://symfony.com/doc/current/routing/custom_route_loader.html)

Copy link
Member

Choose a reason for hiding this comment

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

and I meantroute loader, sorry

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Do you mean you want to move back to the original tag namerouting.route_loader?

Copy link
Member

Choose a reason for hiding this comment

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

Works for me, let's rename the interface + existing implementation, I suggest deprecatingServiceRouterLoader in favor ofContainerRouteLoader.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

WDYT of renaming everything in another PR after this one is merged for clarity ? First, finish this one (I just need to rename the tag back torouting.route_loader). Then, another one of pure refacto.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

friendly ping@fabpot@chalasr, would be cool to have this in 4.3

@fabpotfabpot modified the milestones:4.3,nextMay 6, 2019
@fancywebfancywebforce-pushed theservice-router-loader-private-service- branch from0bb19b7 to205497bCompareMay 6, 2019 11:55
*
* @deprecated since Symfony 4.3, to be removed in 5.0
*/
class ServiceRouterLoaderContainerimplements ContainerInterface

Choose a reason for hiding this comment

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

RouterLoaderContainer?

Copy link
Member

Choose a reason for hiding this comment

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

fancyweb reacted with thumbs up emoji
fabpot added a commit that referenced this pull requestMay 13, 2019
…gged locators (nicolas-grekas)This PR was merged into the 4.3 branch.Discussion----------[DI] default to service id - *not* FQCN - when building tagged locators| Q             | A| ------------- | ---| Branch?       | 4.3| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | no| Fixed tickets | -| License       | MIT| Doc PR        | -While reviewing#30926 I realized that defaulting to the FQCN is a shortcut that isn't useful enough.Defaulting to the service id provides the same experience in practice because service ids are FQCN by default.But when they aren't, the service id is the proper index to default to when building the locator.Commits-------52e827c [DI] default to service id - *not* FQCN - when building tagged locators
@nicolas-grekas
Copy link
Member

#31463 is now merged, this can be rebased to leverage it and take comments into account. Thanks :)

@fancyweb
Copy link
ContributorAuthor

I'm closing here becaus it's a mess and I will reopen new splitted PRs 👀

@fancywebfancyweb deleted the service-router-loader-private-service- branchJuly 18, 2019 13:28
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
symfony-splitter pushed a commit to symfony/framework-bundle 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 |symfony/symfony#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. Insymfony/symfony#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-------154810119d [Routing] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader
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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@OskarStarkOskarStarkOskarStark left review comments

@chalasrchalasrchalasr approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

7 participants

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