IBX-819: Shortened persistence cache tags to save memory usage #3114
Conversation
|
What about backward compatibility? |
|
We need to introduce at least a toggle enabling/disabling short tags usage for keeping BC. Ideally, generating tags should be delegated to some dedicated abstraction but this would require some further work. Until it is done, I will make sure that tests are lit green at least. |
konradoboza
force-pushed
the
ibx-819-shortening-persistence-cache-tags
branch
2 times, most recently
from
611f624 to
fb839da
Compare
konradoboza
force-pushed
the
ibx-819-shortening-persistence-cache-tags
branch
from
fb839da to
387fd38
Compare
eZ/Publish/Core/Persistence/Cache/Tests/AbstractBaseHandlerTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Cache/Tags/TagGeneratorInterface.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Cache/Tags/TagIdentifierPatterns.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Legacy/Tests/Content/Language/CachingLanguageHandlerTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Cache/Tests/AbstractCacheHandlerTest.php
Outdated
Show resolved
Hide resolved
src/lib/Persistence/Cache/Tag/CacheIdentifierGeneratorInterface.php
Outdated
Show resolved
Hide resolved
|
All the remarks applied @Steveb-p, dropped usage of |
eZ/Bundle/EzPublishCoreBundle/Imagine/Cache/AliasGeneratorDecorator.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Cache/AbstractInMemoryPersistenceHandler.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Cache/Tests/AbstractCacheHandlerTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Cache/Tests/AbstractCacheHandlerTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Cache/Tests/AbstractInMemoryCacheHandlerTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Cache/Tests/AbstractInMemoryCacheHandlerTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Cache/Tests/AbstractInMemoryCacheHandlerTest.php
Outdated
Show resolved
Hide resolved
| ->method('generateKey') | ||
| ->withConsecutive(...$keyGeneratingArguments) | ||
| ->willReturnOnConsecutiveCalls(...$keyGeneratingResults); | ||
| } |
There was a problem hiding this comment.
The code for configuring the cache identifier generator is repeated multiple times throughout the PR, so ideally we'd want to get rid of duplication.
From my point of view even a trait would do, with property, getter and configure'ish method.
| { | ||
| $object = new SPILanguage(['id' => 5, 'languageCode' => 'eng-GB']); | ||
|
|
||
| // string $method, array $arguments, string $key, array? $tagGeneratingArguments, array? $tagGeneratingResults, array? $keyGeneratingArguments, array? $keyGeneratingResults, mixed? $data, bool $multi |
There was a problem hiding this comment.
For the future, this belongs to phpdoc as @return declaration:
/**
* @return array<array{string, string, string, array,... etc.}>
*/(for static analysis validation)
|
As discussed, the remaining two code review remarks are entirely optional. Due to time contraints I'm ok with skipping those, as they're partially out of scope / would require postponing the merger by too much while bringing not enough value. |
7.5JIRA excerpt:
We need to clearly state that the Redis cache needs to be flushed after upgrading the project to the version that will contain changes proposed within this PR. Otherwise, the persistence cache will get corrupted and its purging will be done against wrong keys.
Related PR: ezsystems/LegacyBridge#184
TODO:
$ composer fix-cs).