Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fixed CachePoolClearerPass fails if "cache.annotations" service is created#21362
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fc98e4e to25fa964Compare25fa964 toef4a474Comparexabbuh commentedJan 21, 2017
Can you also add a test that would fail without your changes? |
2708b19 tofcd76eaCompareantanas-arvasevicius commentedJan 21, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hi, added functional test which fails in 3.2 branch and works with this PR.
|
| */ | ||
| public function process(ContainerBuilder $container) | ||
| { | ||
| if (!($container->hasDefinition('cache.annotations') || $container->hasAlias('cache.annotations'))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Personally I would write this inversed, saving some parenthesis and reads a lot easier:
if (!$container->hasDefinition('cache.annotations') && !$container->hasAlias('cache.annotations'))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is it a preferred way in this project? From practice I can say that thinking in NOT (x OR y) is less confused than NOT x AND NOT y as first you could reason by simple positive predicate and then just negate it:
hasDefinitions := X OR Y; and negation would be: hasNotDefinition := NOT hasDefinition;
In second case it would be confusion as we immediately start thinking from negation hasNotDefinition := NOT x AND NOT y; and then positive would be: hasDefinition := NOT hasNotDefinition -> NOT (NOT x AND NOT y) (pretty complicated?) we need more transformations: (NOT NOT x) OR (NOT NOT y) -> x OR y;
Generally we use more positive logic in daily life than negative and it's easier to reason about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Personally, I find it less confusing when an inversed condition is not a composed one. So I find@iltar's suggestion more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The less parenthesis andOR present in a condition, the easier it is to understand. Especially in guard clauses you want it to be as specific as possible withAND:if x AND y AND z; then return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, changed to AND if it's better. But if it would be if x AND y then yes it's better, but now there is if NOT x AND NOT y and is more difficult to reason than if NOT (x OR y); In natural language the more negations is in the sentence the harder to know a real meaning. ( seehttp://www.grammar-monster.com/glossary/double_negative.htm )
62c601e to94acf45Compare…che.annotations" service is created
94acf45 tocbae126Compareantanas-arvasevicius commentedJan 22, 2017
Tests failed in not related bundle to this PR in TwigBundle. |
nicolas-grekas commentedJan 23, 2017
Closed in favor of#21381, thanks@antanas-arvasevicius! |
Uh oh!
There was an error while loading.Please reload this page.
Moved "cache.annotations" injenction logic from
CachePoolClearerPasswhich was called afterRemovePrivateAliasesPassintoCacheAnnotationsMonologInjectorPasswhich is now called just beforeRemovePrivateAliasesPassso that we can track "cache.annotations" service definition propery.Also using findDefinition() instead of getDefinition() as it can be an alias too.
And do not using has() as it searches not only definition, definition aliases but also in instantiated services too.