8000 [FrameworkBundle] fix ValidatorCacheWarmer: use serializing ArrayAdapter by dmaicher · Pull Request #23558 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

[FrameworkBundle] fix ValidatorCacheWarmer: use serializing ArrayAdapter #23558

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9b84d72
[FrameworkBundle] fix ValidatorCacheWarmer: use serializing ArrayAdapter
dmaicher Jul 17, 2017
8753b06
also apply for other cache warmers + fix array_filter
dmaicher Jul 18, 2017
71b41f7
review changes
dmaicher Jul 18, 2017
6bd0398
remove comment
dmaicher Jul 18, 2017
44ffbf5
make members private
dmaicher Jul 18, 2017
8ab2ad6
simplify
dmaicher Jul 18, 2017
73abab8
more review feedback
dmaicher Jul 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 93 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?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 This class is meant for internal use only.
Copy link
Member

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

*/
abstract class AbstractPhpFileCacheWarmer implements CacheWarmerInterface
Copy link
Member

Choose a reason for hiding this comment

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

should be marked @internal

{
protected $phpArrayFile;
protected $fallbackPool;
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed 👍


/**
* @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)
{
$phpArrayAdapter = new PhpArrayAdapter($this->phpArrayFile, $this->fallbackPool);
$arrayAdapter = new ArrayAdapter();

spl_autoload_register(array($phpArrayAdapter, 'throwOnRequiredClass'));
Copy link
Member

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)

try {
if (false === $this->doWarmUp($cacheDir, $phpArrayAdapter, $arrayAdapter)) {
return;
}
} finally {
spl_autoload_unregister(array($phpArrayAdapter, '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($phpArrayAdapter, $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 PhpArrayAdapter $phpArrayAdapter
* @param ArrayAdapter $arrayAdapter
*
* @return bool|void false if there is nothing to warm-up
*/
abstract protected function doWarmUp($cacheDir, PhpArrayAdapter $phpArrayAdapter, ArrayAdapter $arrayAdapter);
}
76 changes: 21 additions & 55 deletions src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php
View file
Open in desktop
< A3E2 td class="blob-num blob-num-deletion empty-cell">
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,19 @@
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
* and declared in DI bundle extensions using the addAnnotatedClassesToCache method.
*
* @author Titouan Galopin <galopintitouan@gmail.com>
*/
class AnnotationsCacheWarmer implements CacheWarmerInterface
class AnnotationsCacheWarmer extends AbstractPhpFileCacheWarmer
{
private $annotationReader;
private $phpArrayFile;
private $fallbackPool;

/**
* @param Reader $annotationReader
Expand All @@ -41,71 +36,42 @@ 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, PhpArrayAdapter $phpArrayAdapter, ArrayAdapter $arrayAdapter)
{
$adapter = new PhpArrayAdapter($this->phpArrayFile, $this->fallbackPool);
$annotatedClassPatterns = $cacheDir.'/annotations.map';

if (!is_file($annotatedClassPatterns)) {
$adapter->warmUp(array());
$phpArrayAdapter->warmUp(array());

return;
return false;
Copy link
Member

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)

< 67E6 /div>
}

$annotatedClasses = include $annotatedClassPatterns;

$arrayPool = new ArrayAdapter(0, false);
$reader = new CachedReader($this->annotationReader, new DoctrineProvider($arrayPool));

spl_autoload_register(array($adapter, 'throwOnRequiredClass'));
try {
foreach ($annotatedClasses as $class) {
try {
$this->readAllComponents($reader, $class);
} catch (\ReflectionException $e) {
// ignore failing reflection
} catch (AnnotationException $e) {
/*
* Ignore any AnnotationException to not break the cache warming process if an Annotation is badly
* 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.
*/
}
$reader = new CachedReader($this->annotationReader, new DoctrineProvider($arrayAdapter));

foreach ($annotatedClasses as $class) {
try {
$this->readAllComponents($reader, $class);
} catch (\ReflectionException $e) {
// ignore failing reflection
} catch (AnnotationException $e) {
/*
* Ignore any AnnotationException to not break the cache warming process if an Annotation is badly
* 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;
}

private function readAllComponents(Reader $reader, $class)
Expand Down
60 changes: 13 additions & 47 deletions src/Symfony/Bundle/FrameworkBundle/CacheWarmer/SerializerCacheWarmer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,8 @@

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;
Expand All @@ -30,11 +27,9 @@
*
* @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
Expand All @@ -43,61 +38,32 @@ 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, PhpArrayAdapter $phpArrayAdapter, 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);
$arrayPool = new ArrayAdapter(0, false);

$metadataFactory = new CacheClassMetadataFactory(new ClassMetadataFactory(new LoaderChain($this->loaders)), $arrayPool);
$metadataFactory = new CacheClassMetadataFactory(new ClassMetadataFactory(new LoaderChain($this->loaders)), $arrayAdapter);

spl_autoload_register(array($adapter, 'throwOnRequiredClass'));
try {
foreach ($this->extractSupportedLoaders($this->loaders) as $loader) {
foreach ($loader->getMappedClasses() as $mappedClass) {
try {
$metadataFactory->getMetadataFor($mappedClass);
} catch (\ReflectionException $e) {
// ignore failing reflection
} catch (AnnotationException $e) {
// ignore failing annotations
}
foreach ($this->extractSupportedLoaders($this->loaders) as $loader) {
foreach ($loader->getMappedClasses() as $mappedClass) {
try {
$metadataFactory->getMetadataFor($mappedClass);
} 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;
}

/**
Expand Down
Loading
0