Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

EZP-30546: Cache service has wrong instance on command line #2634

Merged
merged 2 commits into from
May 13, 2019
Merged

Conversation

andrerom
Copy link
Contributor

Question Answer
JIRA issue EZP-30546
Bug/Improvement yes
Target version 7.5
BC breaks no
Tests pass yes
Doc needed no

Cache service being in wrong instance seems to be caused by a few things:

  • Changes in 2.5 where ezplublish.cache_pool.inner service was introduced ( 8baa4f4) seems to cause it to no longer be lazy loaded.
    • I'm actually a bit unsure why, however opted to move lazy definition to same as had this before: ezpublish.cache_pool.
  • The loading was caused by command services that rely on repo services, these are loaded long before console event is triggered and siteaccess is set. To avoid similar issues like this in the future, take advantage of Symfony 3.4 feature to lazy load command services by setting name on tag.

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

While not the bug itself, these commands where loading cache before siteaccess where set worsening the issue with cache pool not being right instance.
Make them lazy to avoid similar issues for other services, and avoid having to load them unless used.
@andrerom
Copy link
Contributor Author

PR is ready for Review, failure on AppVeyor is unrelated.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, I rather aimed to autowire and autoconfigure commands at some point :(
+1 anyway.

@andrerom
Copy link
Contributor Author

Uh, I rather aimed to autowire and autoconfigure commands at some point :(

And there is no way to make them lazy then unless you change to make them container aware instead then right? ... :P

@andrerom andrerom merged commit bf255c8 into 7.5 May 13, 2019
@andrerom andrerom deleted the EZP-30546 branch May 13, 2019 12:58
@alongosz
Copy link
Member

Uh, I rather aimed to autowire and autoconfigure commands at some point :(

And there is no way to make them lazy then unless you change to make them container aware instead then right? ... :P

Yeah, I know, just complaining to complain ;) Seems we have no other choice for now.

@andrerom
Copy link
Contributor Author

andrerom commented May 13, 2019

Well, lazy commands is not an option for 6.7 and 6.13 as it's Symfony 3.4+ only. So if you dislike them we can skip them.

The solution is then always 1. Use Config Resolver 2. For services made with factory that depends on SA config, opt for using lazy service.

And of course somehow figure out to detect when loaded to early as is WIP on the other PR.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants