NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[DependencyInjection] Define default priority inside service class#31943

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

Jump to bottom

Closed

pcabreus wants to merge13 commits intosymfony:4.4frompcabreus:patch-1

Closed

[DependencyInjection] Define default priority inside service class#31943

pcabreus wants to merge13 commits intosymfony:4.4frompcabreus:patch-1

Conversation

Copy link

pcabreus commentedJun 7, 2019•
edited
Loading

Q	A
Branch?	4.4 for features
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#...
License	MIT

With autoconfigure functions we don't have to do much to inject tagged services into a collector using interfaces, using!tagged is the easy way. But if we need some order we have to use priority, the problem is that priority only can be set by configuration file when it is a simple value.
This PR take a look if the static methodgetDefaultPriority exists in the service class and return its default value for the service instead of current 0 by defualt.

Example of a service with default priority:

<?phpnamespace App;use App\ServiceTagged;class Service implement ServiceTagged{    public static function getDefaultPriority(): int    {        return 50; //This will be the default priority if it's not defined in a configuration file    }}?>

PD: First PR please by nice!

Define default priority inside service class

e2138ad

With autoconfigure functions we don't have to do anything to inject tagged services into a collector using interfaces to define tags. But if we need some order we have to use priority, the problem is that priority only can be set by configuration file when it is a simple value.With this change you could instead of put 0 by defualt take a look if the static method `getDefaultPriority` exists in the service class and return its default value for the service.Example:    <?php    namespace App;    use App\ServiceTagged;    class Service implement ServiceTagged    {        public static function getDefaultPriority(): int        {            return 50; //This will be the default priority if it's not defined in a configuration file        }    }    ?>

carsonbot added Status: Needs Review Feature labels

Jun 7, 2019

nicolas-grekas added this to thenext milestone

Jun 7, 2019

yceruto reviewed

Jun 7, 2019

View reviewed changes

Copy link

Member

yceruto left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Hola! Congratulations for your first attempt :) 🎉

src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php OutdatedShow resolvedHide resolved

pcabreus added4 commits

June 9, 2019 23:02

Update YamlFileLoader.php

bd88e1e

It looks like `YamlFileLoader` makes `index_by` mandatory when you define the tagged service using `arguments: [!tagged { tag: 'app.handler'}]` syntax. It's needed to accept others parameters like the new `default_priority_method` without specify `index_by` and the `XmlFielLoader` seems to accept it.

added ability to define an priority method for a tagged collection

2df617e

Merge branch '4.4' into patch-1

4fed2a3

fixed fabbot.io code standard

b6069b2

yceruto reviewed

Jun 10, 2019

View reviewed changes

Copy link

Member

yceruto left a comment

Choose a reason for hiding this comment

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

Others code standard that aren't cover by fabbot.io:

src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php OutdatedShow resolvedHide resolved

src/Symfony/Component/DependencyInjection/Loader/Configurator/ContainerConfigurator.php OutdatedShow resolvedHide resolved

vudaltsov suggested changes

Jun 11, 2019

View reviewed changes

src/Symfony/Component/DependencyInjection/CHANGELOG.md OutdatedShow resolvedHide resolved

src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php OutdatedShow resolvedHide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Jun 11, 2019

pcabreusand others added5 commits

June 11, 2019 10:10

fixed spell

5498824

Co-Authored-By: Valentin Udaltsov <udaltsov.valentin@gmail.com>

compare using null

8ab9cfd

Co-Authored-By: Valentin Udaltsov <udaltsov.valentin@gmail.com>

better exception decription

f48eea9

Co-Authored-By: Valentin Udaltsov <udaltsov.valentin@gmail.com>

fixed some code standard

c021fc8

fixed some code standard

2d8fd1f

kevin-verschaeve reviewed

Jun 11, 2019

View reviewed changes

src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php OutdatedShow resolvedHide resolved

pcabreusand others added3 commits

June 12, 2019 08:24

fixed arguments order

6b51ac5

Co-Authored-By: Kevin Verschaeve <kevin.verschaeve@live.fr>

using thetagged_iterator instead deprecatedtagged

b9dd5eb

optional priority method unless you define it

41f5e99

pcabreus mentioned this pull request

Jun 12, 2019

adding a custom method to define the priority of servicessymfony/symfony-docs#11731

Closed

pcabreus added a commit to pcabreus/symfony-docs that referenced this pull request

Jun 20, 2019

adding a custom method to define the priority of services

3b328b8

This documentssymfony/symfony#31943 about adding a custom method to define the priority of services

pcabreus mentioned this pull request

Jun 20, 2019

adding a custom method to define the priority of servicespcabreus/symfony-docs#1

Open

nicolas-grekas changed the title~~Define default priority inside service class~~[DependencyInjection] Define default priority inside service class

Sep 3, 2020

nicolas-grekas reviewed

Sep 4, 2020

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks and sorry for the late review.
Here are some comments. Please rebase also.
Looks good to me otherwise.

src/Symfony/Component/DependencyInjection/Loader/Configurator/ContainerConfigurator.php

		* @deprecated since Symfony 4.4, to be removed in 5.0, use "tagged_iterator" instead.
		*/
		functiontagged(string$tag,string$indexAttribute =null,string$defaultIndexMethod =null):TaggedIteratorArgument
		functiontagged(string$tag,string$indexAttribute =null,string$defaultIndexMethod =null,bool$needsIndexes =false,string$defaultPriorityMethod =null):TaggedIteratorArgument

Copy link

Member

nicolas-grekasSep 4, 2020

Choose a reason for hiding this comment

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

should be reverted, this function is deprecated, no need to alter it

src/Symfony/Component/DependencyInjection/Loader/Configurator/ContainerConfigurator.php

		* Creates a lazy iterator by tag name.
		*/
		functiontagged_iterator(string$tag,string$indexAttribute =null,string$defaultIndexMethod =null):TaggedIteratorArgument
		functiontagged_iterator(string$tag,string$indexAttribute =null,string$defaultIndexMethod =null,bool$needsIndexes =false,string$defaultPriorityMethod =null):TaggedIteratorArgument

Copy link

Member

nicolas-grekasSep 4, 2020

Choose a reason for hiding this comment

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

$needsIndexes should be removed, it's always false for iterators

src/Symfony/Component/DependencyInjection/Loader/Configurator/ContainerConfigurator.php

		* Creates a service locator by tag name.
		*/
		functiontagged_locator(string$tag,string$indexAttribute,string$defaultIndexMethod =null):ServiceLocatorArgument
		functiontagged_locator(string$tag,string$indexAttribute,string$defaultIndexMethod =null,bool$needsIndexes =true,string$defaultPriorityMethod =null):ServiceLocatorArgument

Copy link

Member

nicolas-grekasSep 4, 2020

Choose a reason for hiding this comment

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

$needsIndexes should be removed, it's always true for locators

src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php

		$priority =isset($attributes[0]['priority']) ?$attributes[0]['priority'] :0;
		$class =$container->getDefinition($serviceId)->getClass();
		$class =$container->getParameterBag()->resolveValue($class) ?:null;
		$reflectionClass =$container->getReflectionClass($class);

Copy link

Member

nicolas-grekasSep 4, 2020

Choose a reason for hiding this comment

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

this line must be executed only ifnull !== $defaultPriorityMethod and$attributes[0]['priority'] is not set, so that we don't autoload a class for nothing when not needed