bug #23558 [FrameworkBundle] fix ValidatorCacheWarmer: use serializing 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 | https://github.com/symfony/symfony/issues/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?

```diff
diff --git a/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php b/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php
index 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?

Commits
-------

c0556cb204 [FrameworkBundle] fix ValidatorCacheWarmer: use serializing ArrayAdapter

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php

@@ -0,0 +1,92 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\CacheWarmer;
+
+use Psr\Cache\CacheItemPoolInterface;
+use Symfony\Component\Cache\Adapter\AdapterInterface;
+use Symfony\Component\Cache\Adapter\ArrayAdapter;
+use Symfony\Component\Cache\Adapter\PhpArrayAdapter;
+use Symfony\Component\Cache\Adapter\ProxyAdapter;
+use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;
+
+/**
+ * @internal
+ */
+abstract class AbstractPhpFileCacheWarmer implements CacheWarmerInterface
+{
+    private $phpArrayFile;
+    private $fallbackPool;
+
+    /**
+     * @param string                 $phpArrayFile The PHP file where metadata are cached
+     * @param CacheItemPoolInterface $fallbackPool The pool where runtime-discovered metadata are cached
+     */
+    public function __construct($phpArrayFile, CacheItemPoolInterface $fallbackPool)
+    {
+        $this->phpArrayFile = $phpArrayFile;
+        if (!$fallbackPool instanceof AdapterInterface) {
+            $fallbackPool = new ProxyAdapter($fallbackPool);
+        }
+        $this->fallbackPool = $fallbackPool;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function isOptional()
+    {
+        return true;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function warmUp($cacheDir)
+    {
+        $arrayAdapter = new ArrayAdapter();
+
+        spl_autoload_register(array(PhpArrayAdapter::class, 'throwOnRequiredClass'));
+        try {
+            if (!$this->doWarmUp($cacheDir, $arrayAdapter)) {
+                return;
+            }
+        } finally {
+            spl_autoload_unregister(array(PhpArrayAdapter::class, 'throwOnRequiredClass'));
+        }
+
+        // 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) { return null !== $val ? unserialize($val) : null; }, $arrayAdapter->getValues());
+
+        $this->warmUpPhpArrayAdapter(new PhpArrayAdapter($this->phpArrayFile, $this->fallbackPool), $values);
+
+        foreach ($values as $k => $v) {
+            $item = $this->fallbackPool->getItem($k);
+            $this->fallbackPool->saveDeferred($item->set($v));
+        }
+        $this->fallbackPool->commit();
+    }
+
+    protected function warmUpPhpArrayAdapter(PhpArrayAdapter $phpArrayAdapter, array $values)
+    {
+        $phpArrayAdapter->warmUp($values);
+    }
+
+    /**
+     * @param string       $cacheDir
+     * @param ArrayAdapter $arrayAdapter
+     *
+     * @return bool false if there is nothing to warm-up
+     */
+    abstract protected function doWarmUp($cacheDir, ArrayAdapter $arrayAdapter);
+}

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php

@@ -15,12 +15,8 @@ use Doctrine\Common\Annotations\AnnotationException;
 use Doctrine\Common\Annotations\CachedReader;
 use Doctrine\Common\Annotations\Reader;
 use Psr\Cache\CacheItemPoolInterface;
-use Symfony\Component\Cache\Adapter\AdapterInterface;
 use Symfony\Component\Cache\Adapter\ArrayAdapter;
-use Symfony\Component\Cache\Adapter\PhpArrayAdapter;
-use Symfony\Component\Cache\Adapter\ProxyAdapter;
 use Symfony\Component\Cache\DoctrineProvider;
-use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;
 
 /**
  * Warms up annotation caches for classes found in composer's autoload class map
@@ -28,11 +24,9 @@ use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;
  *
  * @author Titouan Galopin <galopintitouan@gmail.com>
  */
-class AnnotationsCacheWarmer implements CacheWarmerInterface
+class AnnotationsCacheWarmer extends AbstractPhpFileCacheWarmer
 {
     private $annotationReader;
-    private $phpArrayFile;
-    private $fallbackPool;
 
     /**
      * @param Reader                 $annotationReader
@@ -41,70 +35,41 @@ class AnnotationsCacheWarmer implements CacheWarmerInterface
      */
     public function __construct(Reader $annotationReader, $phpArrayFile, CacheItemPoolInterface $fallbackPool)
     {
+        parent::__construct($phpArrayFile, $fallbackPool);
         $this->annotationReader = $annotationReader;
-        $this->phpArrayFile = $phpArrayFile;
-        if (!$fallbackPool instanceof AdapterInterface) {
-            $fallbackPool = new ProxyAdapter($fallbackPool);
-        }
-        $this->fallbackPool = $fallbackPool;
     }
 
     /**
      * {@inheritdoc}
      */
-    public function warmUp($cacheDir)
+    protected function doWarmUp($cacheDir, ArrayAdapter $arrayAdapter)
     {
-        $adapter = new PhpArrayAdapter($this->phpArrayFile, $this->fallbackPool);
         $annotatedClassPatterns = $cacheDir.'/annotations.map';
 
         if (!is_file($annotatedClassPatterns)) {
-            $adapter->warmUp(array());
+            return true;
-
-            return;
         }
 
         $annotatedClasses = include $annotatedClassPatterns;
+        $reader = new CachedReader($this->annotationReader, new DoctrineProvider($arrayAdapter));
 
-        $arrayPool = new ArrayAdapter(0, false);
+        foreach ($annotatedClasses as $class) {
-        $reader = new CachedReader($this->annotationReader, new DoctrineProvider($arrayPool));
+            try {
-
+                $this->readAllComponents($reader, $class);
-        spl_autoload_register(array($adapter, 'throwOnRequiredClass'));
+            } catch (\ReflectionException $e) {
-        try {
+                // ignore failing reflection
-            foreach ($annotatedClasses as $class) {
+            } catch (AnnotationException $e) {
-                try {
+                /*
-                    $this->readAllComponents($reader, $class);
+                 * Ignore any AnnotationException to not break the cache warming process if an Annotation is badly
-                } catch (\ReflectionException $e) {
+                 * configured or could not be found / read / etc.
-                    // ignore failing reflection
+                 *
-                } catch (AnnotationException $e) {
+                 * In particular cases, an Annotation in your code can be used and defined only for a specific
-                    /*
+                 * environment but is always added to the annotations.map file by some Symfony default behaviors,
-                     * Ignore any AnnotationException to not break the cache warming process if an Annotation is badly
+                 * and you always end up with a not found Annotation.
-                     * configured or could not be found / read / etc.
+                 */
-                     *
-                     * In particular cases, an Annotation in your code can be used and defined only for a specific
-                     * environment but is always added to the annotations.map file by some Symfony default behaviors,
-                     * and you always end up with a not found Annotation.
-                     */
-                }
             }
-        } finally {
-            spl_autoload_unregister(array($adapter, 'throwOnRequiredClass'));
         }
 
-        $values = $arrayPool->getValues();
-        $adapter->warmUp($values);
-
-        foreach ($values as $k => $v) {
-            $item = $this->fallbackPool->getItem($k);
-            $this->fallbackPool->saveDeferred($item->set($v));
-        }
-        $this->fallbackPool->commit();
-    }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function isOptional()
-    {
         return true;
     }
 

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/SerializerCacheWarmer.php

@@ -13,11 +13,7 @@ namespace Symfony\Bundle\FrameworkBundle\CacheWarmer;
 
 use Doctrine\Common\Annotations\AnnotationException;
 use Psr\Cache\CacheItemPoolInterface;
-use Symfony\Component\Cache\Adapter\AdapterInterface;
 use Symfony\Component\Cache\Adapter\ArrayAdapter;
-use Symfony\Component\Cache\Adapter\PhpArrayAdapter;
-use Symfony\Component\Cache\Adapter\ProxyAdapter;
-use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;
 use Symfony\Component\Serializer\Mapping\Factory\CacheClassMetadataFactory;
 use Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactory;
 use Symfony\Component\Serializer\Mapping\Loader\LoaderChain;
@@ -30,11 +26,9 @@ use Symfony\Component\Serializer\Mapping\Loader\YamlFileLoader;
  *
  * @author Titouan Galopin <galopintitouan@gmail.com>
  */
-class SerializerCacheWarmer implements CacheWarmerInterface
+class SerializerCacheWarmer extends AbstractPhpFileCacheWarmer
 {
     private $loaders;
-    private $phpArrayFile;
-    private $fallbackPool;
 
     /**
      * @param LoaderInterface[]      $loaders      The serializer metadata loaders
@@ -43,60 +37,33 @@ class SerializerCacheWarmer implements CacheWarmerInterface
      */
     public function __construct(array $loaders, $phpArrayFile, CacheItemPoolInterface $fallbackPool)
     {
+        parent::__construct($phpArrayFile, $fallbackPool);
         $this->loaders = $loaders;
-        $this->phpArrayFile = $phpArrayFile;
-        if (!$fallbackPool instanceof AdapterInterface) {
-            $fallbackPool = new ProxyAdapter($fallbackPool);
-        }
-        $this->fallbackPool = $fallbackPool;
     }
 
     /**
      * {@inheritdoc}
      */
-    public function warmUp($cacheDir)
+    protected function doWarmUp($cacheDir, ArrayAdapter $arrayAdapter)
     {
         if (!class_exists(CacheClassMetadataFactory::class) || !method_exists(XmlFileLoader::class, 'getMappedClasses') || !method_exists(YamlFileLoader::class, 'getMappedClasses')) {
-            return;
+            return false;
         }
 
-        $adapter = new PhpArrayAdapter($this->phpArrayFile, $this->fallbackPool);
+        $metadataFactory = new CacheClassMetadataFactory(new ClassMetadataFactory(new LoaderChain($this->loaders)), $arrayAdapter);
-        $arrayPool = new ArrayAdapter(0, false);
 
-        $metadataFactory = new CacheClassMetadataFactory(new ClassMetadataFactory(new LoaderChain($this->loaders)), $arrayPool);
+        foreach ($this->extractSupportedLoaders($this->loaders) as $loader) {
-
+            foreach ($loader->getMappedClasses() as $mappedClass) {
-        spl_autoload_register(array($adapter, 'throwOnRequiredClass'));
+                try {
-        try {
+                    $metadataFactory->getMetadataFor($mappedClass);
-            foreach ($this->extractSupportedLoaders($this->loaders) as $loader) {
+                } catch (\ReflectionException $e) {
-                foreach ($loader->getMappedClasses() as $mappedClass) {
+                    // ignore failing reflection
-                    try {
+                } catch (AnnotationException $e) {
-                        $metadataFactory->getMetadataFor($mappedClass);
+                    // ignore failing annotations
-                    } catch (\ReflectionException $e) {
-                        // ignore failing reflection
-                    } catch (AnnotationException $e) {
-                        // ignore failing annotations
-                    }
                 }
             }
-        } finally {
-            spl_autoload_unregister(array($adapter, 'throwOnRequiredClass'));
         }
 
-        $values = $arrayPool->getValues();
-        $adapter->warmUp($values);
-
-        foreach ($values as $k => $v) {
-            $item = $this->fallbackPool->getItem($k);
-            $this->fallbackPool->saveDeferred($item->set($v));
-        }
-        $this->fallbackPool->commit();
-    }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function isOptional()
-    {
         return true;
     }
 

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php

@@ -13,11 +13,8 @@ namespace Symfony\Bundle\FrameworkBundle\CacheWarmer;
 
 use Doctrine\Common\Annotations\AnnotationException;
 use Psr\Cache\CacheItemPoolInterface;
-use Symfony\Component\Cache\Adapter\AdapterInterface;
 use Symfony\Component\Cache\Adapter\ArrayAdapter;
 use Symfony\Component\Cache\Adapter\PhpArrayAdapter;
-use Symfony\Component\Cache\Adapter\ProxyAdapter;
-use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;
 use Symfony\Component\Validator\Mapping\Cache\Psr6Cache;
 use Symfony\Component\Validator\Mapping\Factory\LazyLoadingMetadataFactory;
 use Symfony\Component\Validator\Mapping\Loader\LoaderChain;
@@ -31,11 +28,9 @@ use Symfony\Component\Validator\ValidatorBuilderInterface;
  *
  * @author Titouan Galopin <galopintitouan@gmail.com>
  */
-class ValidatorCacheWarmer implements CacheWarmerInterface
+class ValidatorCacheWarmer extends AbstractPhpFileCacheWarmer
 {
     private $validatorBuilder;
-    private $phpArrayFile;
-    private $fallbackPool;
 
     /**
      * @param ValidatorBuilderInterface $validatorBuilder
@@ -44,64 +39,43 @@ class ValidatorCacheWarmer implements CacheWarmerInterface
      */
     public function __construct(ValidatorBuilderInterface $validatorBuilder, $phpArrayFile, CacheItemPoolInterface $fallbackPool)
     {
+        parent::__construct($phpArrayFile, $fallbackPool);
         $this->validatorBuilder = $validatorBuilder;
-        $this->phpArrayFile = $phpArrayFile;
-        if (!$fallbackPool instanceof AdapterInterface) {
-            $fallbackPool = new ProxyAdapter($fallbackPool);
-        }
-        $this->fallbackPool = $fallbackPool;
     }
 
     /**
      * {@inheritdoc}
      */
-    public function warmUp($cacheDir)
+    protected function doWarmUp($cacheDir, ArrayAdapter $arrayAdapter)
     {
         if (!method_exists($this->validatorBuilder, 'getLoaders')) {
-            return;
+            return false;
         }
 
-        $adapter = new PhpArrayAdapter($this->phpArrayFile, $this->fallbackPool);
-        $arrayPool = new ArrayAdapter(0, false);
-
         $loaders = $this->validatorBuilder->getLoaders();
-        $metadataFactory = new LazyLoadingMetadataFactory(new LoaderChain($loaders), new Psr6Cache($arrayPool));
+        $metadataFactory = new LazyLoadingMetadataFactory(new LoaderChain($loaders), new Psr6Cache($arrayAdapter));
 
-        spl_autoload_register(array($adapter, 'throwOnRequiredClass'));
+        foreach ($this->extractSupportedLoaders($loaders) as $loader) {
-        try {
+            foreach ($loader->getMappedClasses() as $mappedClass) {
-            foreach ($this->extractSupportedLoaders($loaders) as $loader) {
+                try {
-                foreach ($loader->getMappedClasses() as $mappedClass) {
+                    if ($metadataFactory->hasMetadataFor($mappedClass)) {
-                    try {
+                        $metadataFactory->getMetadataFor($mappedClass);
-                        if ($metadataFactory->hasMetadataFor($mappedClass)) {
-                            $metadataFactory->getMetadataFor($mappedClass);
-                        }
-                    } catch (\ReflectionException $e) {
-                        // ignore failing reflection
-                    } catch (AnnotationException $e) {
-                        // ignore failing annotations
                     }
+                } catch (\ReflectionException $e) {
+                    // ignore failing reflection
+                } catch (AnnotationException $e) {
+                    // ignore failing annotations
                 }
             }
-        } finally {
-            spl_autoload_unregister(array($adapter, 'throwOnRequiredClass'));
         }
 
-        $values = $arrayPool->getValues();
+        return true;
-        $adapter->warmUp(array_filter($values));
-
-        foreach ($values as $k => $v) {
-            $item = $this->fallbackPool->getItem($k);
-            $this->fallbackPool->saveDeferred($item->set($v));
-        }
-        $this->fallbackPool->commit();
     }
 
-    /**
+    protected function warmUpPhpArrayAdapter(PhpArrayAdapter $phpArrayAdapter, array $values)
-     * {@inheritdoc}
-     */
-    public function isOptional()
     {
-        return true;
+        // make sure we don't cache null values
+        parent::warmUpPhpArrayAdapter($phpArrayAdapter, array_filter($values));
     }
 
     /**