Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Cleanup CachedContainer invalidation #3867

Merged
merged 10 commits into from
May 17, 2023
Merged

Cleanup CachedContainer invalidation #3867

merged 10 commits into from
May 17, 2023

Conversation

staabm
Copy link
Contributor

@staabm staabm commented May 16, 2023 •
edited
Loading

  • centralize cache invalidation

staabm
staabm commented May 16, 2023
View reviewed changes
src/Kernel/CacheInvalidatingContainer.php Outdated
Comment on lines 26 to 28
RectorKernel::clearCache();

throw $e;
Copy link
Contributor Author

@staabm staabm May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I though about automatically creating a fresh uncached container in this case, but it did not yet work for me.

will look into it again in another PR

@staabm staabm marked this pull request as ready for review May 16, 2023 08:34
@staabm staabm requested a review from TomasVotruba as a code owner May 16, 2023 08:34
staabm
staabm commented May 16, 2023
View reviewed changes
src/Kernel/CacheInvalidatingContainer.php
RectorKernel::clearCache();

throw new \RuntimeException(
'Container cache is outdated and was cleared. please re-run the command.',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets throw with a more explicit error message, so people know about the "auto-healing" capabilities

samsonasik
samsonasik requested changes May 16, 2023
View reviewed changes
Copy link
Member

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

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

can "v8" container cache be named generic name or still need increase the value?

@staabm
Copy link
Contributor Author

staabm commented May 16, 2023

can "v8" container cache be named generic name or still need increase the value?

I guess you are talking about the cache-key. using a generic string for this cache-key does not make sense.

I think there is still room for improvement but as of now, it should work as far as I know.

we need to try it for a bit longer to get an idea which cases are still problematic and which force us to increase the kernel-cache-key. if we have concrete repro steps we can think about how to progress further

@staabm staabm mentioned this pull request May 17, 2023
Merged
samsonasik
samsonasik approved these changes May 17, 2023
View reviewed changes
Copy link
Member

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

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

Ok, lets merge it so we have faster feedback 👍

@samsonasik samsonasik merged commit f0141af into rectorphp:main May 17, 2023
@staabm staabm deleted the cleanup branch May 17, 2023 13:00
@samsonasik
Copy link
Member

@staabm I need to revert this as I tried in CodeIgniter4 project with clear-cache and got error:

Fatal error: Uncaught Error: Class "RectorPrefix202305\Symplify\SmartFileSystem\SmartFileSystem" not found in /Users/samsonasik/www/CodeIgniter4/vendor/rector/rector/src/Kernel/CachedContainerBuilder.php:71
Stack trace:
#0 /Users/samsonasik/www/CodeIgniter4/vendor/rector/rector/src/Kernel/RectorKernel.php(76): Rector\Core\Kernel\CachedContainerBuilder->clearCache()
#1 /Users/samsonasik/www/CodeIgniter4/vendor/rector/rector/src/Console/Command/ProcessCommand.php(144): Rector\Core\Kernel\RectorKernel::clearCache()

samsonasik added a commit that referenced this pull request May 17, 2023
@samsonasik
Revert "Cleanup CachedContainer invalidation (#3867)"
2f8e42e 
This reverts commit f0141af.
samsonasik added a commit that referenced this pull request May 17, 2023
@samsonasik
Revert Cleanup CachedContainer invalidation (#3867)
dc171db 
This reverts commit f0141af.
samsonasik added a commit that referenced this pull request May 17, 2023
@samsonasik
Revert Cleanup CachedContainer invalidation (#3867) (#3880)
5e3afcb 
This reverts commit f0141af.
samsonasik added a commit that referenced this pull request May 17, 2023
@samsonasik
Revert Revert Cleanup CachedContainer invalidation (#3867) (#3880)
43e1bac 
This reverts commit 5e3afcb.
samsonasik added a commit that referenced this pull request May 17, 2023
@samsonasik
Revert Revert Cleanup CachedContainer invalidation (#3867) (#3880) (#…
41aabcb 
…3881)

* Revert Revert Cleanup CachedContainer invalidation (#3867) (#3880)

This reverts commit 5e3afcb.

* update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samsonasik samsonasik samsonasik approved these changes

@TomasVotruba TomasVotruba Awaiting requested review from TomasVotruba

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@staabm @samsonasik