-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Replacing Zend_Cache with symfony/cache component #40058
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.4-develop
Are you sure you want to change the base?
Conversation
Hi @jakwinkler. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
@ihor-sviziev check it out ;-) something we can merge before 2.5? ;-) |
not sure. @engcom-Delta could someone from internal team review and decide whether we can merge it to 2.4 or not? |
@magento run all tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates Magento's cache system from Zend_Cache to Symfony Cache components, addressing a significant architectural change in the framework's caching infrastructure. The changes introduce Symfony Cache as the new caching backend while maintaining compatibility with existing Magento cache interfaces.
Key changes include:
- Introduction of a new Symfony Cache adapter that implements Magento's FrontendInterface
- Replacement of Zend Cache constants with new constants in FrontendInterface
- Updates to Core class to use PSR-6 CacheItemPoolInterface instead of Zend_Cache_Core
- Migration of cache backend factory to create Symfony adapters
Reviewed Changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 8 comments.
Show a summary per file
File | Description |
---|---|
composer.json | Updates dependencies to replace magento/zend-cache with symfony/cache |
lib/internal/Magento/Framework/Cache/FrontendInterface.php | Adds cache cleaning mode constants to replace Zend Cache constants |
lib/internal/Magento/Framework/Cache/Frontend/Adapter/Symfony.php | New Symfony Cache adapter implementing FrontendInterface |
lib/internal/Magento/Framework/Cache/Core.php | Complete rewrite to use PSR-6 CacheItemPoolInterface |
lib/internal/Magento/Framework/App/Cache/Frontend/Factory.php | Factory updated to create Symfony Cache adapters |
Multiple test files | Updated to use new constants and mock PSR interfaces |
public function test($identifier) | ||
{ | ||
$item = $this->cache->getItem($this->cleanIdentifier($identifier)); | ||
return $item->isHit() ? ($item->getMetadata()['mtime'] ?? time()) : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getMetadata() method is not part of the PSR-6 CacheItemInterface standard. This will cause a fatal error when used with standard PSR-6 implementations. Consider using a different approach to track modification time or document that this requires a specific cache adapter implementation.
return $item->isHit() ? ($item->getMetadata()['mtime'] ?? time()) : false; | |
if ($item->isHit()) { | |
$data = $item->get(); | |
return $data['mtime'] ?? time(); | |
} | |
return false; |
Copilot uses AI. Check for mistakes.
public function test($identifier) | ||
{ | ||
$item = $this->cachePool->getItem($this->_id($identifier)); | ||
return $item->isHit() ? ($item->getMetadata()['mtime'] ?? time()) : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getMetadata() method is not part of the PSR-6 CacheItemInterface standard. This will cause a fatal error when used with standard PSR-6 implementations. Consider using a different approach to track modification time or document that this requires a specific cache adapter implementation.
return $item->isHit() ? ($item->getMetadata()['mtime'] ?? time()) : false; | |
return $item->isHit() ? ($item->get()['mtime'] ?? time()) : false; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakwinkler I see all these issues related to PSR-6 are caused by using wrong interface for the cachePool. I guess the right interface should be \Symfony\Component\Cache\Adapter\AdapterInterface
, which extends Psr\Cache\CacheItemPoolInterface
*/ | ||
public function remove($identifier) | ||
{ | ||
return $this->cachePool->delete($this->_id($identifier)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The delete() method is not part of the PSR-6 CacheItemPoolInterface. The correct method name is deleteItem(). This will cause a fatal error.
return $this->cachePool->delete($this->_id($identifier)); | |
return $this->cachePool->deleteItem($this->_id($identifier)); |
Copilot uses AI. Check for mistakes.
$metadata = $item->getMetadata(); | ||
return [ | ||
'expire' => $metadata['expiry'] ?? 0, | ||
'mtime' => $metadata['mtime'] ?? time(), | ||
'tags' => [], // Tags are not directly supported by PSR-6 metadata | ||
]; | ||
} | ||
return false; | ||
} | ||
|
||
return; | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getMetadata() method used on line 234 is not part of the PSR-6 CacheItemInterface standard. This will cause a fatal error when used with standard PSR-6 implementations.
$metadata = $item->getMetadata(); | |
return [ | |
'expire' => $metadata['expiry'] ?? 0, | |
'mtime' => $metadata['mtime'] ?? time(), | |
'tags' => [], // Tags are not directly supported by PSR-6 metadata | |
]; | |
} | |
return false; | |
} | |
return; | |
/** | |
// Retrieve metadata from a custom storage mechanism | |
$metadata = $this->retrieveMetadata($cacheId); | |
return [ | |
'expire' => $metadata['expiry'] ?? 0, | |
'mtime' => $metadata['mtime'] ?? time(), | |
'tags' => $metadata['tags'] ?? [], // Tags retrieved from custom storage | |
]; | |
} | |
return false; | |
} | |
/** | |
* Retrieve metadata for a given cache ID from custom storage. | |
* | |
* @param string $cacheId | |
* @return array | |
*/ | |
private function retrieveMetadata($cacheId) | |
{ | |
// Example implementation: Replace with actual storage logic | |
$metadataStorage = $this->getMetadataStorage(); | |
return $metadataStorage[$cacheId] ?? []; | |
} | |
/** | |
* Get the metadata storage. | |
* | |
* @return array | |
*/ | |
private function getMetadataStorage() | |
{ | |
// Example implementation: Replace with actual storage logic | |
return [ | |
'exampleCacheId' => [ | |
'expiry' => time() + 3600, | |
'mtime' => time(), | |
'tags' => ['tag1', 'tag2'], | |
], | |
]; | |
} | |
/** |
Copilot uses AI. Check for mistakes.
@@ -455,9 +455,9 @@ public static function removeCache() | |||
public static function clearCache($tag = null): void | |||
{ | |||
if ($tag) { | |||
self::$cache->clean(\Zend_Cache::CLEANING_MODE_MATCHING_TAG, $tag); | |||
self::$cache->clean(\Magento\Framework\Cache\FrontendInterface::CLEANING_MODE_MATCHING_TAG, (array)$tag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clean() method is being called on self::$cache which is typed as FrontendInterface, but the parameter casting to (array) suggests the original code expected a single string. This change in behavior could cause issues if calling code passes a string instead of an array.
self::$cache->clean(\Magento\Framework\Cache\FrontendInterface::CLEANING_MODE_MATCHING_TAG, (array)$tag); | |
$tags = is_array($tag) ? $tag : [$tag]; | |
self::$cache->clean(\Magento\Framework\Cache\FrontendInterface::CLEANING_MODE_MATCHING_TAG, $tags); |
Copilot uses AI. Check for mistakes.
$frontendFactory = function () use ($adaptee) { | ||
return $adaptee; | ||
}; | ||
$lowLevelFrontend = new Zend($frontendFactory); | ||
$lowLevelFrontend = new Symfony($frontendFactory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable $frontendFactory is a closure that returns a CacheItemPoolInterface mock, but the Symfony constructor expects a CacheItemPoolInterface directly, not a factory function. This will cause a type error.
$lowLevelFrontend = new Symfony($frontendFactory); | |
$cacheItemPool = $frontendFactory(); | |
$lowLevelFrontend = new Symfony($cacheItemPool); |
Copilot uses AI. Check for mistakes.
break; | ||
// Symfony Cache does not have a direct SQLite adapter. PdoAdapter can be used with SQLite DSN. | ||
// For now, fallback to FilesystemAdapter or throw an error. | ||
throw new \Exception('SQLite cache backend is not directly supported by Symfony Cache adapters.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using the generic Exception class instead of a more specific exception type makes error handling less precise. Consider using a more specific exception class like UnsupportedOperationException or creating a custom exception.
throw new \Exception('SQLite cache backend is not directly supported by Symfony Cache adapters.'); | |
throw new \LogicException('SQLite cache backend is not directly supported by Symfony Cache adapters.'); |
Copilot uses AI. Check for mistakes.
break; | ||
// Symfony Cache does not have an eAccelerator adapter. | ||
// For now, fallback to FilesystemAdapter or throw an error. | ||
throw new \Exception('eAccelerator cache backend is not directly supported by Symfony Cache adapters.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using the generic Exception class instead of a more specific exception type makes error handling less precise. Consider using a more specific exception class like UnsupportedOperationException or creating a custom exception.
throw new \Exception('eAccelerator cache backend is not directly supported by Symfony Cache adapters.'); | |
throw new \LogicException('eAccelerator cache backend is not directly supported by Symfony Cache adapters.'); |
Copilot uses AI. Check for mistakes.
"magento/zend-db": "^1.16", | ||
"magento/zend-pdf": "^1.16", | ||
"monolog/monolog": "^3.6", | ||
"opensearch-project/opensearch-php": "^2.3", | ||
"pelago/emogrifier": "^7.0", | ||
"php-amqplib/php-amqplib": "^3.2", | ||
"phpseclib/mcrypt_compat": "^2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was removed by misteke, it's not relates to this PR
@jakwinkler, please review the comments from Copilot and let us know if you would like anything to be fixed based on them. If no, I'll review the pull request |
@ihor-sviziev I'm not entire sure about PSR-6 starndard with Magento, can you elaborate on that? |
@magento run all tests BTW, I checked - the
Looks like you've restricted pushing by maintainers, so I can't add the fix |
@ihor-sviziev thanks for checking, I'll get back on this in 2-3 days max. |
@magento create issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please review the comments from Copilot and let us know if you would like anything to be fixed based on them.
Also, please fix the issue described here #40058 (comment)
Description (*)
My biggest PR for Magento Open Source.
This PR removes
Zend_Cache
adapter and introducessymfony-cache
to be used instead.Manual testing scenarios (*)
Contribution checklist (*)
Resolved issues: