Fixed CachePoolClearerPass fails if "cache.annotations" service is created #21362
Conversation
fc98e4e to
25fa964
Compare
25fa964 to
ef4a474
Compare
|
Can you also add a test that would fail without your changes? |
2708b19 to
fcd76ea
Compare
|
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.
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.
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.
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.
The less parenthesis and OR present in a condition, the easier it is to understand. Especially in guard clauses you want it to be as specific as possible with AND:
if x AND y AND z; then return
There was a problem hiding this comment.
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. ( see http://www.grammar-monster.com/glossary/double_negative.htm )
62c601e to
94acf45
Compare
…che.annotations" service is created
94acf45 to
cbae126
Compare
|
Tests failed in not related bundle to this PR in TwigBundle. |
|
Closed in favor of #21381, thanks @antanas-arvasevicius! |
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.