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

[DI] Rework config hierarchy: defaults > instanceof > service config#22294

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

Closed

Conversation

@weaverryan
Copy link
Member

@weaverryanweaverryan commentedApr 5, 2017•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?no (because instanceof is new)
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRinstanceofsymfony/symfony-docs#7538

This reworks howdefaults,instanceof and service config override each other. For example:

services:_defaults:autowire:true_instanceof:Symfony\Component\Security\Core\Authorization\Voter\VoterInterface:public:falseautowire:falsetags:[security.voter]AppBundle\Security\PostVoter:public:truetags:            -{ name: security.voter, priority: 100 }
BeforeAfterChanged?
publicfalse
(taken from_instanceof		true
(service overrides_instanceof)		changed
autowiredfalsefalseno change
tags2security.voter tags2security.voter tagsno change
Logic_defaults > service config >_instanceof_defaults >_instanceof > service config

tl;dr;_instanceof now acts like_defaults, but for specific instances. As a user, this is how I expected it to work originally.

NOTE Much of the config merging behavior in this PR - i.e.tags,calls - could be accomplished without such a big diff. But any scalar config - i.e.public,autowire,configurator, etc -requires all of the changes in this PR.

Important Notes:

  • _instanceof has its own override/merge logic. Mostly, it's easy - scalar values override. Some are more opinionated:
    • tags from_instanceof and service are merged - all tags are kept. It's up to the compiler pass to decide of 2 tags should be merged into 1 or if both are needed
    • calls from_instanceof and service are merged, unless the method name of the call match, then only the one from the service wins (i.e. ability to override a call)._instanceof calls are calledfirst
  • I removed support forarguments,factory,deprecated andabstract from_instanceof, and I think we could remove more. These feel like an abuse of_instanceof when a parent class is more proper (and the behavior of how parent-child config is merged is more predictable in general)
  • This also works with parent-child definitions. The parent-child relationship is resolved first and theninstanceof is applied. This means the real hierarchy is:

_defaults >_instanceof >parent definition >child definition

And yep, there's a test for this inIntegrationTest :)

  • [Bug Fix] instanceofConditionals were being lost when a service had a parent. This was fixed in sha:6202909. We simply take the conditionals from the child... which in practice will likely match the parent conditionals, unless they were created in different YAML files (in which case, using the_instanceof from the file where the child was configured is what we want)

Cheers!

jvasseur, linaori, sstok, theofidry, yceruto, and fbourigault reacted with thumbs up emoji
$this->beforeOptimizationPasses =array(
100 =>array(
$resolveClassPass =newResolveClassPass(),
newResolveDefinitionInheritancePass(),
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I moved this down to beafterResolveDefinitionTemplatesPass on purpose: this allows the parent-child definitions to be resolved first. Then, theResolveDefinitionInheritancePass doesn't need to worry about this.

Copy link
Member

@nicolas-grekasnicolas-grekasApr 7, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Thinking about that, this should be kept here: all following compiler passes need to always win. Moving the pass after all DI extension passes is what makes change-tracking a nightmare.
unless I missed something, the full target order proposed in this PR should be:
defaults > instanceof > parents > service > passes

Choose a reason for hiding this comment

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

(vs defaults > parents > service > instanceof > passes as implemented today)

* @internal
*/
publicstaticfunctionmergeDefinition(Definition$def,ChildDefinition$definition)
{
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This class was simply changed to look like it did before#21530 (adjusted for changes since then)

}

$class =$valueinstanceof ChildDefinition ?$this->resolveDefinition($value) :$value->getClass();
$class =$value->getClass();
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This entire class/file could/should be renamed (ResolveInstanceofPass) - I didn't do it to keep the diff more manageable

*
* @return array An array of configured parts for this Definition
*/
publicfunctiongetConfiguredParts()

Choose a reason for hiding this comment

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

should this replace getChanges on ChildDefinition? (or have getChanges migrate here instead?)

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 5, 2017•
edited
Loading

don't add a tag if one by that name was already added

this is already doable with a minor change in the existing code base, isn't it?
if yes, then what's the purpose of this PR? I mean: either the description of the PR should be updated to expain why the other changes are required, or the patch be simplified drastically.
Or do I miss something?

@stof
Copy link
Member

stof commentedApr 5, 2017

@weaverryan We still must have instanceof winning over defaults. Otherwise, it removes the possibility to say "I want all services in this file to be private, except for commands because they are required to be public".

Service-level config should win over instanceof, but not service defaults.

@stof
Copy link
Member

stof commentedApr 5, 2017

the order needs to be_defaults > _instanceof > service, whcih each step winning over the previous one.

@weaverryanweaverryan changed the title[DI] Reworking instanceof to function as "default" values[DI] Rework config hierarchy: defaults > instanceof > service configApr 6, 2017
Copy link
Member

@nicolas-grekasnicolas-grekas 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.

This PR implements another merging logic than the current one. If we agree that this is the most intuitive behavior, then I'm fine with it :)

For the reader, the current behavior (pre this PR) is defaults -> service -> instanceof,
my reasoning doing so was from lessexplicit to most explicit:

  1. less explicit: I don't know what I'm wiring => defaults
  2. a bit more explicit: I know which class I'm wiring => service
  3. most explicit: I know the type hierarchy of my class => instanceof

This matches the ordering of the configuration loading (since the code discovers things in this order also - this PR can't change that).

The logic in this PR is from lessspecific to most specific:

  1. less specific: all services in that file => defaults
  2. a bit more specific: some services in that file => instanceof
  3. most specific:this service in that file => service

$properties =$def->getProperties();
foreach ($instanceofDefinition->getProperties()as$k =>$v) {
// don't override properties set explicitly on the service
if (!isset($properties[$k])) {

Choose a reason for hiding this comment

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

should use array_key_exists, or the+ operator (would replace the foreach)

}
// append method calls
if ($calls =$instanceofDefinition->getMethodCalls()) {
$def->setMethodCalls(array_merge($def->getMethodCalls(),$calls));

Choose a reason for hiding this comment

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

same-name existing calls should not be added IMHO

return$this;
}

privatefunctiontrackChange($type)

Choose a reason for hiding this comment

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

could be called "touch" (first read did tell me what it did - ie could have been the setter also, with boolean arg)

$definition->setTrackChanges(false);
}
$definition->setAutowired($autowire);
if ($autowireDefaultsUsed) {

Choose a reason for hiding this comment

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

"if" not needed

thrownewInvalidArgumentException(sprintf('Invalid autowire attribute: "by_type", "by_id", true or false expected, "%s" given in "%s".',is_string($autowire) ?$autowire :gettype($autowire),$file));
}

if ($autowireDefaultsUsed) {

Choose a reason for hiding this comment

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

"if" not needed, callsetTrackChanges(!$autowireDefaultsUsed)

}
// merge method calls
if ($instanceofCalls =$instanceofDefinition->getMethodCalls()) {
$currentCallMethods =array_map(function($call) {

Choose a reason for hiding this comment

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

array_column to the rescue

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Ah, but now I also need tostrtolower for the case insensitive matching later :)


$uniqueInstanceofCalls =array_filter($instanceofCalls,function($instanceofCall)use ($currentCallMethods) {
// don't add an instanceof call if it was overridden on the service
return !in_array($instanceofCall[0],$currentCallMethods);

Choose a reason for hiding this comment

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

should be case insensitive

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

+1! Got it + test update

…ke a ChildDefinitiontl;dr Before, instanceof values would replace those explicitly set on services. Now, itacts more like a default: only setting a value on a service if that value wasn't explicitlyset on that service (i.e. don't override). For things like tags and calls, they're mergedintelligently and on a case-by-case basis (merging tags works different than merging calls)
… bugThe bug is where _defaults was overriding _instanceof config. To handle that,we ignore change tracking when setting from _defaults
This uncovered a bug, where in ResolveDefinitionTemplatePass, we need to be more carefulwhen setting attributes so that we don't trigger change tracking when there really werenot any changes.
@weaverryan
Copy link
MemberAuthor

This is ready to go once again. I updated the description quite a bit to be as clear as possible.

Other than general code review, the question simply comes down to this: how do we want the override logic to work with_defaults,_instanceof and the service def?@nicolas-grekas explains the 2 here:#22294 (review)

Cheers!

$value->setFile($this->bag->resolveValue($value->getFile()));
$value->setProperties($this->bag->resolveValue($value->getProperties()));
$value->setMethodCalls($this->bag->resolveValue($value->getMethodCalls()));
$value->setTrackChanges(true);
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This is a side-effect of the change tracking... changes. This wasn't actually needed to get the new behavior working correctly, it was just needed to get some tests to pass (i.e. the tests were compiling the container and then comparing Definition objects together... one of the Definition objects now had a few extra "changes")

@weaverryan
Copy link
MemberAuthor

weaverryan commentedApr 6, 2017•
edited
Loading

One more update:

  1. @nicolas-grekas and I agree that we shouldn't merge/override tags like this, but should update the compiler passes to intelligently handle multiple tags (because sometimes 1 tag should replace another, but other times 2 tags should really stay as 2 tags - i.e.controller.service_argument). I've made that change and updated the PR description.

  2. @nicolas-grekas discussed with me that the new "changes" tracking may be difficult to maintain. Basically, it's not possible for theservice definition config to override_instanceof without this. If we want the behavior described in this PR, it's necessary. If we hate thechanges too much, then we need to stay with the old logic.

The logic is that compiler passes are the only ones that truly have enough informationto know if one tag should replace another, or if both are needed.
@nicolas-grekasnicolas-grekas added this to the3.3 milestoneApr 7, 2017
}
$definition->setAutowired($autowire)
->setTrackChanges(true)
;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about updating this to:

if (isset($service['autowire'])) {$definition->setAutowired($service['autowire']);        }elseif (isset($defaults['autowire'])) {$definition                ->setTrackChanges(false)                ->setAutowired($defaults['autowire'])                ->setTrackChanges(true);        }

*
* @return $this
*/
publicfunctionsetTrackChanges($trackChanges)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about marking this as@internal ? I don't think we should bother about untracked changes due to a choice of the user.
Maybe we could even use a bound callable where necessary.

foreach ($definition->getAutowiringTypes(false)as$autowiringType) {
$def->addAutowiringType($autowiringType);
$parentChanges =$parentDef->getChanges();
if (isset($parentChanges['factory'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see one fragility here.
What if the parent uses defaults?

services:_defaults:autowire:trueparent:~
services:Foo\Bar:parent:parent

ThenFoo\Bar won't be autowired, right?

I think the defaults should always be tracked but we should allow to differentiate changes from defaults (maybe just internally, I see no need to do this in userland):

if (isset($defaults['autowire'])) {$definition->setTrackChanges('defaults')        ->setAutowired($defaults['autowire'])        ->setTrackChanges(true);}

and then use something like the following in the passes needing it:

$definition->getChanges($withoutDefaults =true);

Copy link
Contributor

@GuilhemNGuilhemNApr 10, 2017•
edited
Loading

Choose a reason for hiding this comment

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

We could also fix#22345 using this by tracking the detected class name differently (or maybe even by not tracking it).

@weaverryan
Copy link
MemberAuthor

Closing in favor of#22356

@weaverryanweaverryan deleted the instanceof_as_defaults branchApril 11, 2017 14:25
fabpot added a commit that referenced this pull requestApr 11, 2017
…service config (weaverryan, nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Rework config hierarchy: defaults > instanceof > service config| Q             | A| ------------- | ---| Branch?       | 3.3| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -Replaces#22294.The main change is that ChildDefinition does not have "instanceof" applied anymore. All the complexity of the pass came from the merging logic required to deal with them. But in fact, we'd better not have any such logic and just not apply "instanceof" to ChildDefinition (but have them inherit from their parents with the usual logic).Commits-------6d6116b Adding an integration test for the hirarchy of defaults, instanceof, child, parent definitionsab86457 [DI] Rework config hierarchy: defaults > instanceof > service configcbaee55 [DI] Track changes at the "Definition" level
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

+1 more reviewer

@GuilhemNGuilhemNGuilhemN left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

5 participants

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