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

Making tags under _defaults always apply#22530

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

Merged

Conversation

@weaverryan
Copy link
Member

@weaverryanweaverryan commentedApr 26, 2017
edited
Loading

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#22497
LicenseMIT
Doc PRn/a

Obviously, things under_defaults are applied to all services in that file. However, tags was an exception: it wasnot applied unless you haveinherit_tags. The correct behavior is subjective, but after talking about it today, we (mostly) decided that tagsshould always apply. This does exactly that.

One side-effect (explained in the commit message) is that if you have a parent and child service both in the same file, the tag from_defaults is applied twice. I think that makes sense, and at some point, we can't protect the users from their own configuration :). This kind of "weird" behavior is likely not a problem, as compiler passes now handle multiple tags well AND it only affects a case where the user has added tags to_defaultsand is using parent-child definitions. That's quite a strange mixture of conditions :).

Cheers!

linaori and jvasseur reacted with thumbs up emoji
if (null !==$inheritTag) {
$definition->setInheritTags($inheritTag);
}
$defaults =array();
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 below the condition :elseif (isset($service['parent'])) {.

Emptying$defaults meant that services with a parent did not have_defaults applied. I think the intention was to not double-apply defaults (since the parent will already have them applied). Removing this causes double tags (as I described in my description). BUT, I think this was a bug. What if I had a child in this file, but my parent in another file? In that case, the neither the child nor the parent would have the_defaults applied.

@weaverryanweaverryanforce-pushed thedefaults_tags_always_apply branch fromd74da57 toe0bd1caCompareApril 26, 2017 03:31
@fabpot
Copy link
Member

As discussed together, I think this is the right thing to do.

@fabpot
Copy link
Member

👍

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 26, 2017
edited
Loading

👍 also
I think we need to remove the "inherit-tag" feature now. I introduced it to allow configuring this behavior. But now that tags are always inherited, the remaining use case (inherit tags from parents) looks like something I'd prefer to remove and drop some complexity from the core.

@nicolas-grekasnicolas-grekas added this to the3.3 milestoneApr 26, 2017
@fabpot
Copy link
Member

👍 for removinginherit-tag.@weaverryan Can you do that?

@stof
Copy link
Member

👍 for removing it too

@curry684
Copy link
Contributor

Linked issue should be#22497 I suppose 😉

@weaverryan
Copy link
MemberAuthor

Boom!inherit_tags removed!

  • Like in 3.2, child definitionsnever inherit parent tags
  • important After this change, tags from_instanceof are NEVER applied to child definitions. But this is because it's complicated to merge. In another PR, I'm going to propose an exception related to this.

@curry684 you're right! Updated with the issue number - your issue is what pushed me to corner Fabien and Nicolas about this ;).

curry684 reacted with hooray emoji

@nicolas-grekasnicolas-grekas self-requested a reviewApril 27, 2017 15:47
@weaverryan
Copy link
MemberAuthor

There are a few other issues still with this PR - specifically that whiletags are applied from_defaults, the other values (autowire,autoconfigure andpublic) are not. I've just fixed this in the last commit, but there's an issue related to parent definitions that@nicolas-grekas will try to solve in another PR (and the tests fail currently due to this).

Status: Needs Work

@stof
Copy link
Member

important After this change, tags from _instanceof are NEVER applied to child definitions. But this is because it's complicated to merge. In another PR, I'm going to propose an exception related to this.

this looks weird to me, as a common use case is to tag event subscribers or twig extensions for instance.

curry684 reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 27, 2017
edited
Loading

Parent/child definitions, mixed with defaults, instanceof or autoconfig create conflicting expectations. They do not play well with each other.
After spending too much time trying to resolve these, Ryan and I are now confident that we should make parent/child definitions incompatible with these new 3.3 features​, by throwing an exception when used together.

@weaverryan
Copy link
MemberAuthor

See#22563: Almost all of the complexity of handling_defaults,_instanceof and service definition config is coming from children services... which is silly because that's an edge case. In that PR, I propose not supporting them. Then, in this PR, we can applytags from_defaults (which will close#22497), removeinherit-tags, but not have any weird functionality left.

weaverryan:
important After this change, tags from _instanceof are NEVER applied to child definitions. But this is because it's complicated to merge. In another PR, I'm going to propose an exception related to this.

stof:
this looks weird to me, as a common use case is to tag event subscribers or twig extensions for instance.

Yep, I agree. It's very complex. I just added some test cases to#22563, which include a few I highlighted as potential problems (like this situation).

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
@nicolas-grekas
Copy link
Member

Rebase unlocked :)

@weaverryanweaverryanforce-pushed thedefaults_tags_always_apply branch from50ad8b4 to3f5014aCompareMay 1, 2017 13:18
-{ name: bar_tag }
# the tag from defaults do NOT cascade (but see #22530)
# - { name: from_defaults }
-{ name: from_defaults }
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 the key fix for this PR: thefrom_defaults tag is now inherited:

@weaverryan
Copy link
MemberAuthor

Rebased! This PR is now simple:tags under_defaults always apply (like the other things under_defaults) andinherit_tags was removed, which means that parent-child services have the 3.2 functionality (i.e. tags do not inherit).

@weaverryanweaverryanforce-pushed thedefaults_tags_always_apply branch from3f5014a to2b087b8CompareMay 1, 2017 13:31
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.

👍 last PR for 3.3 :)

…tirelyNow that inherit_tags has been removed, 3.3 has the same functionality as 3.2: tagsare *never* cascaded from parent to child (but you tags do inherit from defaultsto a service and instanceof to a service).
@weaverryanweaverryanforce-pushed thedefaults_tags_always_apply branch from2b087b8 to037a782CompareMay 1, 2017 13:36
@fabpot
Copy link
Member

Thank you@weaverryan.

@fabpotfabpot merged commit037a782 intosymfony:masterMay 1, 2017
fabpot added a commit that referenced this pull requestMay 1, 2017
This PR was merged into the 3.3-dev branch.Discussion----------Making tags under _defaults always apply| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#22497| License       | MIT| Doc PR        | n/aObviously, things under `_defaults` are applied to all services in that file. However, tags was an exception: it was *not* applied unless you have `inherit_tags`. The correct behavior is subjective, but after talking about it today, we (mostly) decided that tags *should* always apply. This does exactly that.One side-effect (explained in the commit message) is that if you have a parent and child service both in the same file, the tag from `_defaults` is applied twice. I think that makes sense, and at some point, we can't protect the users from their own configuration :). This kind of "weird" behavior is likely not a problem, as compiler passes now handle multiple tags well AND it only affects a case where the user has added tags to `_defaults` *and* is using parent-child definitions. That's quite a strange mixture of conditions :).Cheers!Commits-------037a782 Making tags under _defaults always apply and removing inherit_tags entirely
@weaverryanweaverryan deleted the defaults_tags_always_apply branchMay 1, 2017 14:27
@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

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

6 participants

@weaverryan@fabpot@nicolas-grekas@stof@curry684@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp