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

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

Conversation

@antanas-arvasevicius
Copy link
Contributor

@antanas-arvaseviciusantanas-arvasevicius commentedJan 20, 2017
edited by javiereguiluz
Loading

QA
Branch?master / 3.2
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#21339
LicenseMIT

Moved "cache.annotations" injenction logic fromCachePoolClearerPass which was called afterRemovePrivateAliasesPass intoCacheAnnotationsMonologInjectorPass which is now called just beforeRemovePrivateAliasesPass so 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.

@xabbuh
Copy link
Member

Can you also add a test that would fail without your changes?

@javiereguiluzjaviereguiluz changed the titlebug #21339 CachePoolClearerPass fails if "cache.annotations" service is createdFixed CachePoolClearerPass fails if "cache.annotations" service is createdJan 21, 2017
@antanas-arvaseviciusantanas-arvaseviciusforce-pushed thecache-annotations-compiler-pass-fix branch 2 times, most recently from2708b19 tofcd76eaCompareJanuary 21, 2017 12:28
@antanas-arvasevicius
Copy link
ContributorAuthor

antanas-arvasevicius commentedJan 21, 2017
edited
Loading

Hi, added functional test which fails in 3.2 branch and works with this PR.
Test simulates behavior written in#21339 ticket.
Short scenario:

  • compilation starts
  • AnnotationConfigurationPass, it uses "cache.annotations" service
  • CacheCollectorPass, it aliases "cache.annotations" and other tagged services (in debug mode it wraps around pools to collect debug info)
  • RemovePrivateAliasesPass , it removes all private aliases so after this step we cannot find "cache.annotations"
  • CachePoolClearerPass, throw error as it cannot find any info about "cache.annotations"
  • end of story

*/
public function process(ContainerBuilder $container)
{
if (!($container->hasDefinition('cache.annotations') || $container->hasAlias('cache.annotations'))) {
Copy link
Contributor

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'))

Copy link
ContributorAuthor

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.

Copy link
Member

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.

Copy link
Contributor

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

Copy link
ContributorAuthor

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 )

@antanas-arvaseviciusantanas-arvaseviciusforce-pushed thecache-annotations-compiler-pass-fix branch 3 times, most recently from62c601e to94acf45CompareJanuary 22, 2017 09:38
@antanas-arvaseviciusantanas-arvaseviciusforce-pushed thecache-annotations-compiler-pass-fix branch from94acf45 tocbae126CompareJanuary 22, 2017 10:28
@antanas-arvasevicius
Copy link
ContributorAuthor

Tests failed in not related bundle to this PR in TwigBundle.

@nicolas-grekas
Copy link
Member

Closed in favor of#21381, thanks@antanas-arvasevicius!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@xabbuhxabbuhxabbuh left review comments

+1 more reviewer

@linaorilinaorilinaori left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@antanas-arvasevicius@xabbuh@nicolas-grekas@linaori@javiereguiluz

[8]ページ先頭

©2009-2025 Movatter.jp