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.

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

21842: don't cache absolute file paths in validator factory #21856

103 changes: 30 additions & 73 deletions lib/internal/Magento/Framework/Validator/Factory.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,33 @@

namespace Magento\Framework\Validator;

use Magento\Framework\Module\Dir\Reader;
use Magento\Framework\ObjectManagerInterface;
use Magento\Framework\Phrase;
use Magento\Framework\Validator;
use Magento\Framework\Cache\FrontendInterface;

/**
* Factory for \Magento\Framework\Validator and \Magento\Framework\Validator\Builder.
*/
class Factory
{
/** cache key */
/**
* cache key
*
* @deprecated
*/
const CACHE_KEY = __CLASS__;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, we are not allowed to remove the constants according to our Backward Compatible Development Guide. Just mark it as deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback. I addressed this in my latest commit.


/**
* @var \Magento\Framework\ObjectManagerInterface
* @var ObjectManagerInterface
*/
protected $_objectManager;

/**
* Validator config files
*
* @var array|null
* @var iterable|null
*/
protected $_configFiles = null;

Expand All @@ -31,40 +42,24 @@ class Factory
private $isDefaultTranslatorInitialized = false;

/**
* @var \Magento\Framework\Module\Dir\Reader
* @var Reader
*/
private $moduleReader;

/**
* @var FrontendInterface
*/
private $cache;

/**
* @var \Magento\Framework\Serialize\SerializerInterface
*/
private $serializer;

/**
* @var \Magento\Framework\Config\FileIteratorFactory
*/
private $fileIteratorFactory;

/**
* Initialize dependencies
*
* @param \Magento\Framework\ObjectManagerInterface $objectManager
* @param \Magento\Framework\Module\Dir\Reader $moduleReader
* @param FrontendInterface $cache
* @param ObjectManagerInterface $objectManager
* @param Reader $moduleReader
* @param FrontendInterface $cache @deprecated
*/
public function __construct(
\Magento\Framework\ObjectManagerInterface $objectManager,
\Magento\Framework\Module\Dir\Reader $moduleReader,
ObjectManagerInterface $objectManager,
Reader $moduleReader,
FrontendInterface $cache
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, leave FrontendInterface as constructor parameter and mark it as deprecated, otherwise it's backward incompatible change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback. I addressed this in my latest commit.

) {
$this->_objectManager = $objectManager;
$this->moduleReader = $moduleReader;
$this->cache = $cache;
}

/**
Expand All @@ -75,32 +70,23 @@ public function __construct(
protected function _initializeConfigList()
{
if (!$this->_configFiles) {
$this->_configFiles = $this->cache->load(self::CACHE_KEY);
if (!$this->_configFiles) {
$this->_configFiles = $this->moduleReader->getConfigurationFiles('validation.xml');
$this->cache->save(
$this->getSerializer()->serialize($this->_configFiles->toArray()),
self::CACHE_KEY
);
} else {
$filesArray = $this->getSerializer()->unserialize($this->_configFiles);
$this->_configFiles = $this->getFileIteratorFactory()->create(array_keys($filesArray));
}
$this->_configFiles = $this->moduleReader->getConfigurationFiles('validation.xml');
}
}

/**
* Create and set default translator to \Magento\Framework\Validator\AbstractValidator.
*
* @return void
* @throws \Zend_Translate_Exception
*/
protected function _initializeDefaultTranslator()
{
if (!$this->isDefaultTranslatorInitialized) {
// Pass translations to \Magento\Framework\TranslateInterface from validators
$translatorCallback = function () {
$argc = func_get_args();
return (string)new \Magento\Framework\Phrase(array_shift($argc), $argc);
return (string)new Phrase(array_shift($argc), $argc);
};
/** @var \Magento\Framework\Translate\Adapter $translator */
$translator = $this->_objectManager->create(\Magento\Framework\Translate\Adapter::class);
Expand All @@ -115,14 +101,15 @@ protected function _initializeDefaultTranslator()
*
* Will instantiate \Magento\Framework\Validator\Config
*
* @return \Magento\Framework\Validator\Config
* @return Config
* @throws \Zend_Translate_Exception
*/
public function getValidatorConfig()
{
$this->_initializeConfigList();
$this->_initializeDefaultTranslator();
return $this->_objectManager->create(
\Magento\Framework\Validator\Config::class,
Config::class,
['configFiles' => $this->_configFiles]
);
}
Expand All @@ -133,7 +120,8 @@ public function getValidatorConfig()
* @param string $entityName
* @param string $groupName
* @param array|null $builderConfig
* @return \Magento\Framework\Validator\Builder
* @return Builder
* @throws \Zend_Translate_Exception
*/
public function createValidatorBuilder($entityName, $groupName, array $builderConfig = null)
{
Expand All @@ -147,43 +135,12 @@ public function createValidatorBuilder($entityName, $groupName, array $builderCo
* @param string $entityName
* @param string $groupName
* @param array|null $builderConfig
* @return \Magento\Framework\Validator
* @return Validator
* @throws \Zend_Translate_Exception
*/
public function createValidator($entityName, $groupName, array $builderConfig = null)
{
$this->_initializeDefaultTranslator();
return $this->getValidatorConfig()->createValidator($entityName, $groupName, $builderConfig);
}

/**
* Get serializer
*
* @return \Magento\Framework\Serialize\SerializerInterface
* @deprecated 100.2.0
*/
private function getSerializer()
{
if ($this->serializer === null) {
$this->serializer = $this->_objectManager->get(
\Magento\Framework\Serialize\SerializerInterface::class
);
}
return $this->serializer;
}

/**
* Get file iterator factory
*
* @return \Magento\Framework\Config\FileIteratorFactory
* @deprecated 100.2.0
*/
private function getFileIteratorFactory()
{
if ($this->fileIteratorFactory === null) {
$this->fileIteratorFactory = $this->_objectManager->get(
\Magento\Framework\Config\FileIteratorFactory::class
);
}
return $this->fileIteratorFactory;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,6 @@ class FactoryTest extends \PHPUnit\Framework\TestCase
*/
private $validatorConfigMock;

/**
* @var \Magento\Framework\Cache\FrontendInterface|\PHPUnit_Framework_MockObject_MockObject
*/
private $cacheMock;

/**
* @var \Magento\Framework\Serialize\SerializerInterface|\PHPUnit_Framework_MockObject_MockObject
*/
private $serializerMock;

/**
* @var \Magento\Framework\Config\FileIteratorFactory|\PHPUnit_Framework_MockObject_MockObject
*/
private $fileIteratorFactoryMock;

/**
* @var \Magento\Framework\Config\FileIterator|\PHPUnit_Framework_MockObject_MockObject
*/
Expand All @@ -55,11 +40,6 @@ class FactoryTest extends \PHPUnit\Framework\TestCase
*/
private $factory;

/**
* @var string
*/
private $jsonString = '["\/tmp\/moduleOne\/etc\/validation.xml"]';

/**
* @var array
*/
Expand Down Expand Up @@ -99,23 +79,9 @@ protected function setUp()
\Magento\Framework\Validator\Factory::class,
[
'objectManager' => $this->objectManagerMock,
'moduleReader' => $this->readerMock,
'cache' => $this->cacheMock
'moduleReader' => $this->readerMock
]
);

$this->serializerMock = $this->createMock(\Magento\Framework\Serialize\SerializerInterface::class);
$this->fileIteratorFactoryMock = $this->createMock(\Magento\Framework\Config\FileIteratorFactory::class);
$objectManager->setBackwardCompatibleProperty(
$this->factory,
'serializer',
$this->serializerMock
);
$objectManager->setBackwardCompatibleProperty(
$this->factory,
'fileIteratorFactory',
$this->fileIteratorFactoryMock
);
}

/**
Expand Down Expand Up @@ -147,46 +113,6 @@ public function testGetValidatorConfig()
);
}

public function testGetValidatorConfigCacheNotExist()
{
$this->cacheMock->expects($this->once())
->method('load')
->willReturn(false);
$this->readerMock->expects($this->once())
->method('getConfigurationFiles')
->willReturn($this->fileIteratorMock);
$this->fileIteratorMock->method('toArray')
->willReturn($this->data);
$this->cacheMock->expects($this->once())
->method('save')
->with($this->jsonString);
$this->serializerMock->expects($this->once())
->method('serialize')
->with($this->data)
->willReturn($this->jsonString);
$this->factory->getValidatorConfig();
$this->factory->getValidatorConfig();
}

public function testGetValidatorConfigCacheExist()
{
$this->cacheMock->expects($this->once())
->method('load')
->willReturn($this->jsonString);
$this->readerMock->expects($this->never())
->method('getConfigurationFiles');
$this->cacheMock->expects($this->never())
->method('save');
$this->serializerMock->expects($this->once())
->method('unserialize')
->with($this->jsonString)
->willReturn($this->data);
$this->fileIteratorFactoryMock->method('create')
->willReturn($this->fileIteratorMock);
$this->factory->getValidatorConfig();
$this->factory->getValidatorConfig();
}

public function testCreateValidatorBuilder()
{
$this->readerMock->method('getConfigurationFiles')
Expand Down