Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[FrameworkBundle] fix ValidatorCacheWarmer: use serializing ArrayAdapter#23558

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

@dmaicher
Copy link
Contributor

@dmaicherdmaicher commentedJul 17, 2017•
edited
Loading

QA
Branch?3.2
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#23544
LicenseMIT
Doc PR-

TheValidatorCacheWarmer was using anArrayAdapter with$storeSerialized=false. This is a problem as inside theLazyLoadingMetadataFactory the metaData objects are mutated (parent class constraints are merged into it) after they have been written into the cache.

So this means that currently when warming up the validator cache actually the merged metaData version is finally taken from theArrayAdapter and written into thePhpFilesAdapter.

Which then caused some duplicate constraints as the parent constraints are merged again after fetching from the cache insideLazyLoadingMetadataFactory.

This fix makes sure we serialize objects into theArrayAdapter.

Writing a test case for this does not seem easy to me. Any ideas?

EDIT: Maybe its even safer to just clone the object when writing it into the cache?

diff --git a/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php b/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.phpindex 79ad1f2..88eaf33 100644--- a/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php+++ b/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php@@ -117,7 +117,7 @@ class LazyLoadingMetadataFactory implements MetadataFactoryInterface         }          if (null !== $this->cache) {-            $this->cache->write($metadata);+            $this->cache->write(clone $metadata);         }

Opinions?

@dmaicherdmaicherforce-pushed thevalidator-cache-warmer-fix branch fromddd9afd to9b84d72CompareJuly 17, 2017 20:35
@nicolas-grekasnicolas-grekas added this to the3.2 milestoneJul 17, 2017
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedJul 18, 2017•
edited
Loading

Good catch!
I think we should do the same in AnnotationsCacheWarmer and in SerializerCacheWarmer.
There is also one thing to fix: the array_filter should happenbefore the array_map (misses are stored asnull, which can't "unserialized" and shouldn't be cached.)
Dunno about patching LazyLoadingMetadataFactory, but anyway, I'm sure this one is needed :)

dmaicher reacted with thumbs up emoji


$adapter =newPhpArrayAdapter($this->phpArrayFile,$this->fallbackPool);
$arrayPool =newArrayAdapter(0,false);
$arrayPool =newArrayAdapter(0);
Copy link
Member

@nicolas-grekasnicolas-grekasJul 18, 2017•
edited
Loading

Choose a reason for hiding this comment

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

0 is the default so it can be removed also

@dmaicherdmaicherforce-pushed thevalidator-cache-warmer-fix branch 3 times, most recently from7eb5bf8 tod8e5fa7CompareJuly 18, 2017 17:50
@dmaicherdmaicherforce-pushed thevalidator-cache-warmer-fix branch fromd8e5fa7 to8753b06CompareJuly 18, 2017 17:51
$this->assertCount(2,$values);
$this->assertCount(1,$values);
$this->assertArrayHasKey('Symfony.Bundle.FrameworkBundle.Tests.Fixtures.Validation.Category',$values);
$this->assertArrayHasKey('Symfony.Bundle.FrameworkBundle.Tests.Fixtures.Validation.SubCategory',$values);
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@nicolas-grekas due to moving thearray_filter I needed to fix this test. It seems before we stored empty data in the cache?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Before the change the array contained this:

array:2 [  "Symfony.Bundle.FrameworkBundle.Tests.Fixtures.Validation.Category" => "O:49:"Symfony\Component\Validator\Mapping\ClassMetadata":10:{s:11:"constraints";a:0:{}s:18:"constraintsByGroup";a:0:{}s:17:"traversalStrategy";i:1;s:7:"getters";a:0:{}s:13:"groupSequence";a:0:{}s:21:"groupSequenceProvider";b:0;s:7:"members";a:2:{s:2:"id";a:1:{i:0;O:52:"Symfony\Component\Validator\Mapping\PropertyMetadata":7:{s:11:"constraints";a:1:{i:0;O:44:"Symfony\Component\Validator\Constraints\Type":4:{s:7:"message";s:40:"This value should be of type {{ type }}.";s:4:"type";s:3:"int";s:7:"payload";N;s:6:"groups";a:2:{i:0;s:7:"Default";i:1;s:8:"Category";}}}s:18:"constraintsByGroup";a:2:{s:7:"Default";a:1:{i:0;r:12;}s:8:"Category";a:1:{i:0;r:12;}}s:17:"cascadingStrategy";i:1;s:17:"traversalStrategy";i:2;s:5:"class";s:65:"Symfony\Bundle\FrameworkBundle\Tests\Fixtures\Validation\Category";s:4:"name";s:2:"id";s:8:"property";s:2:"id";}}s:4:"name";a:1:{i:0;O:52:"Symfony\Component\Validator\Mapping\PropertyMetadata":7:{s:11:"constraints";a:1:{i:0;O:44:"Symfony\Component\Validator\Constraints\Type":4:{s:7:"message";s:40:"This value should be of type {{ type }}.";s:4:"type";s:6:"string";s:7:"payload";N;s:6:"groups";a:2:{i:0;s:7:"Default";i:1;s:8:"Category";}}}s:18:"constraintsByGroup";a:2:{s:7:"Default";a:1:{i:0;r:32;}s:8:"Category";a:1:{i:0;r:32;}}s:17:"cascadingStrategy";i:1;s:17:"traversalStrategy";i:2;s:5:"class";s:65:"Symfony\Bundle\FrameworkBundle\Tests\Fixtures\Validation\Category";s:4:"name";s:4:"name";s:8:"property";s:4:"name";}}}s:4:"name";s:65:"Symfony\Bundle\FrameworkBundle\Tests\Fixtures\Validation\Category";s:10:"properties";a:2:{s:2:"id";r:10;s:4:"name";r:30;}s:12:"defaultGroup";s:8:"Category";}"  "Symfony.Bundle.FrameworkBundle.Tests.Fixtures.Validation.SubCategory" => "b:0;"]

Choose a reason for hiding this comment

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

the behavior should not change at all so this test must not be changed

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

The change you requested makes this test fail though:

There is also one thing to fix: the array_filter should happen before the array_map (misses are stored as null, which can't "unserialized" and shouldn't be cached.)

// the ArrayAdapter stores the values serialized
// to avoid mutation of the data after it was written to the cache
// so here we un-serialize the values first
$values =array_map(function ($val) {returnunserialize($val); },array_filter($arrayAdapter->getValues()));
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@nicolas-grekas is it fine to applyarray_filter here as well for the other cache warmers? Tests are fine although theAnnotationsCacheWarmer does not have any tests 😢

Choose a reason for hiding this comment

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

$values = array_map(function ($val) { return null !== $val ? unserialize($val) : null; }, $arrayAdapter->getValues());
and array_filter should be called only for ValidatorCacheWarmer, and only when feeding phpArrayAdapter.
the foreach then should iterate over all values, null or not

dmaicher reacted with thumbs up emoji
* {@inheritdoc}
*/
publicfunctionwarmUp($cacheDir)
publicfunctionisOptional()

Choose a reason for hiding this comment

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

this could also be moved to the parent I guess

dmaicher reacted with thumbs up emoji
useSymfony\Component\Cache\Adapter\ProxyAdapter;
useSymfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;

abstractclass AbstractPhpFileCacheWarmerimplements CacheWarmerInterface

Choose a reason for hiding this comment

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

should be marked@internal

dmaicher reacted with thumbs up emoji
@dmaicherdmaicherforce-pushed thevalidator-cache-warmer-fix branch fromff47aa1 to71b41f7CompareJuly 18, 2017 20:50
@dmaicher
Copy link
ContributorAuthor

@nicolas-grekas changes done 👍

useSymfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;

/**
* @internal This class is meant for internal use only.

Choose a reason for hiding this comment

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

usually we don't add the additional comment so we should remove it I guess

abstractclass AbstractPhpFileCacheWarmerimplements CacheWarmerInterface
{
protected$phpArrayFile;
protected$fallbackPool;

Choose a reason for hiding this comment

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

shouldn't these both be private?

dmaicher reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Indeed 👍

$phpArrayAdapter->warmUp(array());

return;
returnfalse;

Choose a reason for hiding this comment

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

this makes me think that doWarmUp doesn't need the $phpArrayAdapter argument: it's used only here, but returning !== false here would ship the same behavior without the need for this arg, isn't it?
which then triggers: shouldn't we have a clean "bool" return type for all dowarmup methods? ie return true on success? (and here also then)


spl_autoload_register(array($phpArrayAdapter,'throwOnRequiredClass'));
try {
$this->doWarmUp($cacheDir,$arrayAdapter);

Choose a reason for hiding this comment

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

you still need the if (!doWarmUp) then return; check

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Copy link
Member

@nicolas-grekasnicolas-grekasJul 18, 2017•
edited
Loading

Choose a reason for hiding this comment

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

it will if we make dowarmup return true or false to allow/prevent the warmup

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Ah now I see what you meant 😄 👍 Done.

@dmaicherdmaicherforce-pushed thevalidator-cache-warmer-fix branch fromadd8c87 to8ab2ad6CompareJuly 18, 2017 21:09
$phpArrayAdapter =newPhpArrayAdapter($this->phpArrayFile,$this->fallbackPool);
$arrayAdapter =newArrayAdapter();

spl_autoload_register(array($phpArrayAdapter,'throwOnRequiredClass'));

Choose a reason for hiding this comment

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

since we don't need the instance yet, let's use PhpArrayAdapter::class here instead, and create the object later, (or never when dowarmup returns false)

dmaicher reacted with thumbs up emoji
if (!is_file($annotatedClassPatterns)) {
$adapter->warmUp(array());

return;

Choose a reason for hiding this comment

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

return true

protectedfunctiondoWarmUp($cacheDir,ArrayAdapter$arrayAdapter)
{
if (!class_exists(CacheClassMetadataFactory::class) || !method_exists(XmlFileLoader::class,'getMappedClasses') || !method_exists(YamlFileLoader::class,'getMappedClasses')) {
return;

Choose a reason for hiding this comment

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

return false

protectedfunctiondoWarmUp($cacheDir,ArrayAdapter$arrayAdapter)
{
if (!method_exists($this->validatorBuilder,'getLoaders')) {
return;

Choose a reason for hiding this comment

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

return false

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your reactivity tonight :)

@fabpot
Copy link
Member

Thank you@dmaicher.

fabpot added a commit that referenced this pull requestJul 19, 2017
…g ArrayAdapter (dmaicher)This PR was squashed before being merged into the 3.2 branch (closes#23558).Discussion----------[FrameworkBundle] fix ValidatorCacheWarmer: use serializing ArrayAdapter| Q             | A| ------------- | ---| Branch?       | 3.2| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#23544| License       | MIT| Doc PR        | -The `ValidatorCacheWarmer` was using an `ArrayAdapter` with `$storeSerialized=false`. This is a problem as inside the `LazyLoadingMetadataFactory` the metaData objects are mutated (parent class constraints are merged into it) after they have been written into the cache.So this means that currently when warming up the validator cache actually the merged metaData version is finally taken from the `ArrayAdapter` and written into the `PhpFilesAdapter`.Which then caused some duplicate constraints as the parent constraints are merged again after fetching from the cache inside `LazyLoadingMetadataFactory`.This fix makes sure we serialize objects into the `ArrayAdapter`.Writing a test case for this does not seem easy to me. Any ideas?EDIT: Maybe its even safer to just clone the object when writing it into the cache?```diffdiff --git a/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php b/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.phpindex79ad1f2..88eaf33 100644--- a/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php+++ b/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php@@ -117,7 +117,7 @@ class LazyLoadingMetadataFactory implements MetadataFactoryInterface         }         if (null !== $this->cache) {-            $this->cache->write($metadata);+            $this->cache->write(clone $metadata);         }```Opinions?Commits-------c0556cb [FrameworkBundle] fix ValidatorCacheWarmer: use serializing ArrayAdapter
@fabpotfabpot closed thisJul 19, 2017
This was referencedAug 1, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

3.2

Development

Successfully merging this pull request may close these issues.

4 participants

@dmaicher@nicolas-grekas@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp