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

Not allowing autoconfigure, instanceofConditionals or defaults for ChildDefinition#22563

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

@weaverryan
Copy link
Member

@weaverryanweaverryan commentedApr 28, 2017
edited
Loading

QA
Branch?master
Bug fix?yes (removing risky behavior)
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketssee#22530
LicenseMIT
Doc PRn/a

This PRprohibits usingautoconfigure,_instanceof and_defaults for ChildDefinition.

Additionally, I added many "integration" test cases: we need to test and prove all edge cases. These are in theintegration/ directory: themain.yml file is parsed and compared toexpected.yml. Both are in YAML to ease comparing the before/after. We need to check these out and make sure they're right and we're not missing anything else.

This PR removes MANY of the "wtf" cases, but there are still 4 that I know of... and of course they all deal with parent-child stuff :).

A) [MAJOR]autoconfigure_parent_child_tagsinstanceof tags from autoconfigure are NEVER applied to the child (you can't setautoconfigure directly on a Child, but you still can set it on a parent and inherit it... sneaky). We could throw an Exception I suppose to prevent thisautoconfigure from cascading from parent to child... but it's tricky due toinstanceof.

B( [MAJOR]instanceof_parent_childinstanceof tags that are applied to the parent, are not applied to the child. Again, you can't setinstanceof directly on a Child, but youcan set it on a parent, and have that cascade to the child. Like before, we could maybe throw an exception to prevent this.

C) [MINOR] (autoconfigure_child_not_applied) automaticinstanceof will not be applied to the child when the parent class has a different (non-instanceof-ed) class. If we could throw an exception for (A), then it would cover this too.

D)_tags from defaults are never used (unless you have inherit_tags) - fixed in#22530

A, B & C are effectively caused by there being a "sneaky" way to re-enableautoconfigure andinstanceof for ChildDefinition... which opens up wtf cases.

Wait, why not support_defaults,autoconfigure and_instanceof for child definitions?

1 big reason: reduction of wtf moments where we arbitrarily decide override logic. PLUS, since_defaults,instanceof andautoconfigureare applied to parent definitions, in practice (other than tags), this makes no difference: the configuration will still pass from parent down to child.

Also, using parent-child definitions is already an edge case, and thissimply preventsjust those services from using the new features.

Longer reasons why

The reason behind this is that parent-child definitions are a different mechanism for "inheritance"
than_instanceof and_defaults... creating some edge cases when trying to figure out which settings "win". For example:

# file1.ymlservices:_defaults:public:falseChildService:parent:parent_service# file2.ymlservices:_defaults:public:trueParentService:~

IsChildDefinitionpublic: true (so the parent
overrides the child, even though it only came from _defaults) orpublic: false (where
the child wins... even though it was only set from its _defaults)?

Or, if ParentService is explicitly set topublic: true, should that override thepublic: false of ChildService (which it got from its_defaults)? On one hand, ParentService is being explicitly
set. On the other hand, ChildService is explicitly in a file settings_defaultspublic: false
There's no correct answer.

There are also problems with_instanceof. The importance goes:

defaults < instanceof < service definition

But how do parent-child relationships fit into that? If a child has public: false
from an _instanceof, but the parent explicitly sets public: true, which wins? Should
we assume the parent definition wins because it's explicitly set? Or would the
_instanceof win, because that's being explicitly applied to the child definition's
class by an _instanceof that lives in the same file as that class (whereas the parent
definition may live in a different file).

Because of this,@nicolas-grekas and I (we also talked a bit to Fabien) decided that
the complexity was growing too much. The solution is to not allow any of these
new feature to be used by ChildDefinition objects. In other words, when you want some
sort of "inheritance" for your service, you shouldeither giving your service a
parentor using defaults and instanceof. And instead of silently not applying
defaults and instanceof to child definitions, I think it's better to scream that it's
not supported.

jvasseur and theofidry reacted with thumbs up emoji
@weaverryanweaverryanforce-pushed theexceptions_parent_instanceof_autoconfigure branch 3 times, most recently from335b11b to9939921CompareApril 28, 2017 02:18
@nicolas-grekasnicolas-grekas self-requested a reviewApril 28, 2017 10:30
@weaverryanweaverryanforce-pushed theexceptions_parent_instanceof_autoconfigure branch from4944a47 to710d9a8CompareApril 28, 2017 15:50
@nicolas-grekasnicolas-grekas added this to the3.3 milestoneApr 28, 2017
@weaverryan
Copy link
MemberAuthor

This should be finished now.

It highlights a clear philosophy: basically, when you set aparent key, you are opting out of theautoconfigure,instanceof functionality. And we try to warn you with exceptions where possible.

Yourparent classcan still take advantage of these... and most of those settings will cascade onto the child (notably, not tags, consistent with child/parent functionality). So, your parent's Definition is configured... andthen that's applied to the child.

Also in this PR (but a bit related): wepreventcalls andarguments from being applied to theautoconfigure instanceof. This is because you can't override / opt out of these. And by throwing an exception, we still leave it open to allow these for 3.4, without breaking BC.

Again, please check the test cases in theintegration/ directory!

Cheers!

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.

👍
100% agree with Ryan's comments, this is required to make things consistent and remove the need for multidimensional merges that have no generic solutions.

@simensen
Copy link
Contributor

In talking about this with@weaverryan today, I asked under what circumstances might people want to useparent in which they couldn't solve the problem with_defaults,_instanceof orautoconfigure? Neither of us could think of a concrete example.

Personally, I've only usedparent a few times. All (I think?) of those cases were to cut down on manually wiring the same thing across several similar objects. If_defaults and_instanceof/autoconfigure had been available I think I would have been able to avoidparent altogether.

Said another way, it feels like_defaults,_instanceof andautoconfigure eliminate the usual need forparent altogether. So it seems like disallowing their use together (parent plus the new features ) for children seems fair. At the same time, we could consider markingparent for deprecation to get people to use the new features.

…ildDefinitionAlso, not allowing arguments or method calls for autoconfigure. This is a safetymechanism, since we don't have merging logic. It will allow us to add this in thefuture if we want to.The reason is that parent-child definitions are a different mechanism for "inheritance"than instanceofConditionas and defaults... creating some edge cases when trying tofigure out which settings "win". For example:Suppose a child and parent definitions are defined in different YAML files. Thechild receives public: false from its _defaults, and the parent receives public: truefrom its _defaults. Should the final child definition be public: true (so the parentoverrides the child, even though it only came from _defaults) or public: false (wherethe child wins... even though it was only set from its _defaults). Or, if the parentis explicitly set to public: true, should that override the public: false of thechild (which it got from its _defaults)? On one hand, the parent is being explicitlyset. On the other hand, the child is explicitly in a file settings _defaults publicto false. There's no correct answer.There are also problems with instanceof. The importance goes:  defaults < instanceof < service definitionBut how does parent-child relationships fit into that? If a child has public: falsefrom an _instanceof, but the parent explicitly sets public: true, which wins? Shouldwe assume the parent definition wins because it's explicitly set? Or would the_instanceof win, because that's being explicitly applied to the child definition'sclass by an _instanceof that lives in the same file as that class (whereas the parentdefinition may live in a different file).Because of this,@nicolas-grekas and I (we also talked a bit to Fabien) decided thatthe complexity was growing too much. The solution is to not allow any of thesenew feature to be used by ChildDefinition objects. In other words, when you want somesort of "inheritance" for your service, you should *either* giving your service aparent *or* using defaults and instanceof. And instead of silently not applyingdefaults and instanceof to child definitions, I think it's better to scream that it'snot supported.
@weaverryanweaverryanforce-pushed theexceptions_parent_instanceof_autoconfigure branch from710d9a8 toa943b96CompareApril 28, 2017 21:09
@theofidry
Copy link
Contributor

👍 for that PR.

You could still need a parent to extend a definition of a library, so I don't think parent should be deprecated.

DavG reacted with thumbs up emoji

@fabpot
Copy link
Member

@simensen we had this discussion during DrupalCon with@weaverryan and@nicolas-grekas and we came to the same conclusion. We should deprecateparent in 3.4 and remove it in 4.0. I like when new features allows to remove some other ones.

chalasr reacted with confused emoji

@fabpot
Copy link
Member

Thank you@weaverryan.

@fabpotfabpot merged commita943b96 intosymfony:masterApr 29, 2017
fabpot added a commit that referenced this pull requestApr 29, 2017
…defaults for ChildDefinition (weaverryan)This PR was merged into the 3.3-dev branch.Discussion----------Not allowing autoconfigure, instanceofConditionals or defaults for ChildDefinition| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes (removing risky behavior)| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | see#22530| License       | MIT| Doc PR        | n/aThis PR *prohibits* using `autoconfigure`, `_instanceof` and `_defaults` for ChildDefinition.Additionally, I added many "integration" test cases: we need to test and prove all edge cases. These are in the `integration/` directory: the `main.yml` file is parsed and compared to `expected.yml`. Both are in YAML to ease comparing the before/after. We need to check these out and make sure they're right and we're not missing anything else.This PR removes MANY of the "wtf" cases, but there are still 4 that I know of... and of course they all deal with parent-child stuff :).A) [MAJOR] [autoconfigure_parent_child_tags](https://github.com/symfony/symfony/pull/22563/files#diff-fd6cf15470c5abd40156e4e7dc4e7f6d) `instanceof` tags from autoconfigure are NEVER applied to the child (you can't set `autoconfigure` directly on a Child, but you still can set it on a parent and inherit it... sneaky). We could throw an Exception I suppose to prevent this `autoconfigure` from cascading from parent to child... but it's tricky due to `instanceof`.B( [MAJOR] [instanceof_parent_child](https://github.com/symfony/symfony/pull/22563/files#diff-14666e9a25322d44b3c2c583b6814dc2) `instanceof` tags that are applied to the parent, are not applied to the child. Again, you can't set `instanceof` directly on a Child, but you *can* set it on a parent, and have that cascade to the child. Like before, we could maybe throw an exception to prevent this.C) [MINOR] ([autoconfigure_child_not_applied](https://github.com/symfony/symfony/pull/22563/files#diff-3372a1dcaf3af30d14a7d0a6c8bfa988))  automatic `instanceof` will not be applied to the child when the parent class has a different (non-instanceof-ed) class. If we could throw an exception for (A), then it would cover this too.D) `_tags` from defaults are never used (unless you have inherit_tags) - fixed in#22530A, B & C are effectively caused by there being a "sneaky" way to re-enable `autoconfigure` and `instanceof` for ChildDefinition... which opens up wtf cases.## Wait, why not support `_defaults`, `autoconfigure` and `_instanceof` for child definitions?1 big reason: reduction of wtf moments where we arbitrarily decide override logic. PLUS, since `_defaults`, `instanceof` and `autoconfigure` *are* applied to parent definitions, in practice (other than tags), this makes no difference: the configuration will still pass from parent down to child.Also, using parent-child definitions is already an edge case, and this *simply* prevents *just* those services from using the new features.## Longer reasons whyThe reason behind this is that parent-child definitions are a different mechanism for "inheritance"than `_instanceof` and `_defaults`... creating some edge cases when trying to figure out which settings "win". For example:```yml# file1.ymlservices:    _defaults:        public: false    ChildService:        parent: parent_service# file2.ymlservices:    _defaults:        public: true    ParentService: ~```Is `ChildDefinition` `public: true` (so the parentoverrides the child, even though it only came from _defaults) or `public: false` (wherethe child wins... even though it was only set from its _defaults)?Or, if ParentService is explicitly set to `public: true`, should that override the `public: false` of ChildService (which it got from its `_defaults`)? On one hand, ParentService is being explicitlyset. On the other hand, ChildService is explicitly in a file settings `_defaults` `public: false`There's no correct answer.There are also problems with `_instanceof`. The importance goes:> defaults < instanceof < service definitionBut how do parent-child relationships fit into that? If a child has public: falsefrom an _instanceof, but the parent explicitly sets public: true, which wins? Shouldwe assume the parent definition wins because it's explicitly set? Or would the_instanceof win, because that's being explicitly applied to the child definition'sclass by an _instanceof that lives in the same file as that class (whereas the parentdefinition may live in a different file).Because of this,@nicolas-grekas and I (we also talked a bit to Fabien) decided thatthe complexity was growing too much. The solution is to not allow any of thesenew feature to be used by ChildDefinition objects. In other words, when you want somesort of "inheritance" for your service, you should *either* giving your service aparent *or* using defaults and instanceof. And instead of silently not applyingdefaults and instanceof to child definitions, I think it's better to scream that it'snot supported.Commits-------a943b96 Not allowing autoconfigure, instanceofConditionals or defaults for ChildDefinition
@weaverryanweaverryan deleted the exceptions_parent_instanceof_autoconfigure branchMay 1, 2017 12:56
@fabpotfabpot mentioned this pull requestMay 1, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@stofstofstof approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

7 participants

@weaverryan@simensen@theofidry@fabpot@nicolas-grekas@stof@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp