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

[DependencyInjection] Improve ServiceLocatorTagPass service matching#28571

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

Merged

nicolas-grekas merged 1 commit intosymfony:masterfromcodedmonkey:service-locator

Oct 3, 2018

Merged

[DependencyInjection] Improve ServiceLocatorTagPass service matching#28571

nicolas-grekas merged 1 commit intosymfony:masterfromcodedmonkey:service-locator

Oct 3, 2018

Conversation

Copy link

Contributor

codedmonkey commentedSep 23, 2018•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#26892
License	MIT
Doc PR	symfony/symfony-docs#10397

Allows omitting of keys for service locator arguments (it will automatically take over the original definition alias).

carsonbot added Status: Needs Review DependencyInjection Feature labels

Sep 23, 2018

codedmonkey force-pushed theservice-locator branch 2 times, most recently from831ebf6 toe58293bCompare

September 23, 2018 20:11

nicolas-grekas reviewed

Sep 23, 2018

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, do you think you could add some tests please?

src/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php Outdated

		continue;
		}

		if (\is_string($type =$v) &&preg_match('/^\??[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]+(?:\\\\[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]+)*+$/',$type)) {

Copy link

Member

nicolas-grekasSep 23, 2018•
edited
Loading

Choose a reason for hiding this comment

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

this will allow<argument>Psr\Logger\LoggerInterface</argument>
alongside with<argument type="service" />
I read too fast this part of your proposal. I think it's not a good idea. It would create an inconsistency in the way the configuration works.
But being able to omit the "key" part looks like a good idea to me.

nicolas-grekas reviewed

Sep 24, 2018

View reviewed changes

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

		}

		if (\is_int($k)) {
		unset($arguments[0][$k]);

Copy link

Member

nicolas-grekasSep 24, 2018•
edited
Loading

Choose a reason for hiding this comment

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

I'd suggest moving "unset" outside of the "if": that will allow preserving the order in the list

Copy link

ContributorAuthor

codedmonkeySep 24, 2018

Choose a reason for hiding this comment

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

I agree but right after the for loop it does aksort so I presume that's unnecessary.

Copy link

Member

nicolas-grekasSep 24, 2018

Choose a reason for hiding this comment

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

I forgot about it, no need then. Thanks.

nicolas-grekas added this to thenext milestone

Sep 24, 2018

codedmonkey force-pushed theservice-locator branch 2 times, most recently from0357dc0 to55e1075Compare

September 24, 2018 19:13

codedmonkey mentioned this pull request

Sep 24, 2018

[DependencyInjection] Add documentation for service locator changessymfony/symfony-docs#10397

Merged

chalasr reviewed

Sep 25, 2018

View reviewed changes

src/Symfony/Component/DependencyInjection/Tests/Compiler/ServiceLocatorTagPassTest.php Outdated


		$this->assertEquals(CustomDefinition::class,\get_class($locator('bar')));
		$this->assertEquals(CustomDefinition::class,\get_class($locator('baz')));
		$this->assertEquals(CustomDefinition::class,\get_class($locator('some.service')));

Copy link

Member

chalasrSep 25, 2018

Choose a reason for hiding this comment

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

assertSame

chalasr approved these changes

Sep 25, 2018

View reviewed changes

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

Sep 25, 2018

stof reviewed

Sep 25, 2018

View reviewed changes

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

		if (\is_int($k)) {
		unset($arguments[0][$k]);

		$k = (string)$v;

Copy link

Member

stofSep 25, 2018

Choose a reason for hiding this comment

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

should we check whether this key already exists in the configured arguments ?

Copy link

Member

nicolas-grekasSep 25, 2018

Choose a reason for hiding this comment

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

I'm not sure: if the key existsafter the numbered one, it will override it - and if it came before, then the numbered one should win to me. works for you also?

Copy link

Member

stofSep 27, 2018

Choose a reason for hiding this comment

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

nope, it willalways override it here. If it comes after the numbered one, processing it will see the value set by the processing of the numbered one, not the original one, as it would have been replaced

Copy link

Member

nicolas-grekasSep 27, 2018

Choose a reason for hiding this comment

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

Really? foreach operates on a copy of array so that it will yield the original value at this iteration, and overwrite the value set by the numbered step to me.

Copy link

ContributorAuthor

codedmonkeySep 30, 2018

Choose a reason for hiding this comment

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

Added some basic tests that should confirm@nicolas-grekas description. I do like the idea of preventing unexepected behavior for the user but it works either way for me.

codedmonkey force-pushed theservice-locator branch froma467c0a tod44c37fCompare

September 27, 2018 09:58

nicolas-grekas reviewed

Sep 27, 2018

View reviewed changes

src/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php Outdated

		$k = (string)$v;

		if (isset($arguments[0][$k])) {
		thrownewInvalidArgumentException(sprintf('Invalid definition for service "%s": service key "%s" cannot be inherited from the service definition because the key already exists.',$this->currentId,$k));

Copy link

Member

nicolas-grekasSep 27, 2018

Choose a reason for hiding this comment

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

Per my comments above, I'm not sure about this. It should be removed to me.

Copy link

Member

nicolas-grekasSep 27, 2018

Choose a reason for hiding this comment

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

(but please add tests showing that the later wins over the former if you can, that'll help prove who is right here :) )

Copy link

ContributorAuthor

codedmonkeySep 27, 2018

Choose a reason for hiding this comment

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

Fine by me if all differences have been set aside. I've checked how the different configuration formats handled duplicate declarations and they don't seem work the same at all, so there doesn't seem to be a best way.

Copy link

Member

nicolas-grekasSep 29, 2018

Choose a reason for hiding this comment

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

Can you elaborate? What's the difference you spotted?
If we want to keep the exception (I'm fine with it also), I'd suggest a message like:
`Invalid service locator definition for "%s": key "%s" is defined twice.', $this->currentId, $k));

Copy link

ContributorAuthor

codedmonkeySep 30, 2018

Choose a reason for hiding this comment

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

Here's my examples:

// PHP - Result: service_b    ->setArguments([[        'service_a' => new Reference('service_a'),        'service_a' => new Reference('service_b'),    ]])// YAML - Result: service_a        arguments:            -                service_a: '@service_a'                service_a: '@service_b'// XML - Result: service_b      <argument type="collection">        <argument type="service" key="service_a" />        <argument type="service" key="service_a" />      </argument>

Not that how PHP handles duplicate array keys matters a lot, just thought it might give a direction for the right solution.

codedmonkey force-pushed theservice-locator branch fromd44c37f to2d6050eCompare

September 30, 2018 17:10

Copy link

Member

nicolas-grekas commentedSep 30, 2018

Thanks for the update. Can you add a test case that throws the new exception please?

Copy link

ContributorAuthor

codedmonkey commentedOct 1, 2018

I removed the additional exception if that's all right with you?

nicolas-grekas approved these changes

Oct 1, 2018

View reviewed changes

[DependencyInjection] Improve ServiceLocatorTagPass service matching

1c1210a

nicolas-grekas force-pushed theservice-locator branch from2d6050e to1c1210aCompare

October 3, 2018 07:27

Copy link

Member

nicolas-grekas commentedOct 3, 2018

Thank you@codedmonkey.

nicolas-grekas merged commit1c1210a intosymfony:master

Oct 3, 2018

nicolas-grekas added a commit that referenced this pull request

Oct 3, 2018

feature#28571[DependencyInjection] Improve ServiceLocatorTagPass se…

104e922

…rvice matching (codedmonkey)This PR was squashed before being merged into the 4.2-dev branch (closes#28571).Discussion----------[DependencyInjection] Improve ServiceLocatorTagPass service matching| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#26892| License       | MIT| Doc PR        |symfony/symfony-docs#10397Allows omitting of keys for service locator arguments (it will automatically take over the original definition alias).Commits-------1c1210a [DependencyInjection] Improve ServiceLocatorTagPass service matching

nicolas-grekas modified the milestones:next,4.2

Nov 1, 2018

fabpot mentioned this pull request

Nov 3, 2018

Release v4.2.0-BETA1#29072

Closed

fabpot mentioned this pull request

Nov 3, 2018

Release v4.2.0-BETA1#29073

Merged

codedmonkey deleted the service-locator branch

December 14, 2018 23:26

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

Sep 23, 2019

minor#10397[DependencyInjection] Add documentation for service loca…

de28f4c

…tor changes (codedmonkey, javiereguiluz)This PR was submitted for the master branch but it was merged into the 4.3 branch instead (closes#10397).Discussion----------[DependencyInjection] Add documentation for service locator changesAdds documentation for PRsymfony/symfony#28571Commits-------8d8c65e Rewordbc5ef85 [DependencyInjection] Add documentation for service locator changes

Labels

DependencyInjection Feature Status: Reviewed

Movatterモバイル変換

Uh oh!

[DependencyInjection] Improve ServiceLocatorTagPass service matching#28571

[DependencyInjection] Improve ServiceLocatorTagPass service matching#28571

Uh oh!

Conversation

codedmonkey commentedSep 23, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasSep 23, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasSep 24, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedSep 30, 2018

Uh oh!

codedmonkey commentedOct 1, 2018

Uh oh!

nicolas-grekas commentedOct 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codedmonkey commentedSep 23, 2018•
edited
Loading

nicolas-grekasSep 23, 2018•
edited
Loading

nicolas-grekasSep 24, 2018•
edited
Loading