Skip to content

[ExpressionLanguage] Making cache PSR6 compliant #19741

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

Merged
merged 1 commit into from
Oct 17, 2016
Merged

[ExpressionLanguage] Making cache PSR6 compliant #19741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions UPGRADE-3.2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ DependencyInjection
* Calling `get()` on a `ContainerBuilder` instance before compiling the
container is deprecated and will throw an exception in Symfony 4.0.

ExpressionLanguage
-------------------

* Passing a `ParserCacheInterface` instance to the `ExpressionLanguage` has been
deprecated and will not be supported in Symfony 4.0. You should use the
`CacheItemPoolInterface` interface instead.

Form
----

Expand Down
7 changes: 7 additions & 0 deletions UPGRADE-4.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ DependencyInjection
* Requesting a private service with the `Container::get()` method is no longer
supported.

ExpressionLanguage
----------

* The ability to pass a `ParserCacheInterface` instance to the `ExpressionLanguage`
class has been removed. You should use the `CacheItemPoolInterface` interface
instead.

Form
----

Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Bridge/Doctrine/ExpressionLanguage/DoctrineParserCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,16 @@

namespace Symfony\Bridge\Doctrine\ExpressionLanguage;

@trigger_error('The '.__NAMESPACE__.'\DoctrineParserCache class is deprecated since version 3.2 and will be removed in 4.0. Use the Symfony\Component\Cache\Adapter\DoctrineAdapter class instead.', E_USER_DEPRECATED);

use Doctrine\Common\Cache\Cache;
use Symfony\Component\ExpressionLanguage\ParsedExpression;
use Symfony\Component\ExpressionLanguage\ParserCache\ParserCacheInterface;

/**
* @author Adrien Brault <adrien.brault@gmail.com>
*
* @deprecated DoctrineParserCache class is deprecated since version 3.2 and will be removed in 4.0. Use the Symfony\Component\Cache\Adapter\DoctrineAdapter class instead.
*/
class DoctrineParserCache implements ParserCacheInterface
{
Expand Down
3 changes: 3 additions & 0 deletions src/Symfony/Bridge/Doctrine/Tests/ExpressionLanguage/DoctrineParserCacheTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@

use Symfony\Bridge\Doctrine\ExpressionLanguage\DoctrineParserCache;

/**
* @group legacy
*/
class DoctrineParserCacheTest extends \PHPUnit_Framework_TestCase
Copy link
Member

Choose a reason for hiding this comment

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

DoctrineParserCache should also be deprecated (and this whole test marked as legacy, no need for assertDeprecationsAreTriggered)

{
public function testFetch()
Expand Down
28 changes: 20 additions & 8 deletions src/Symfony/Component/ExpressionLanguage/ExpressionLanguage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@

namespace Symfony\Component\ExpressionLanguage;

use Symfony\Component\ExpressionLanguage\ParserCache\ArrayParserCache;
use Psr\Cache\CacheItemPoolInterface;
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 first (alpha ordering)

use Symfony\Component\Cache\Adapter\ArrayAdapter;
use Symfony\Component\ExpressionLanguage\ParserCache\ParserCacheAdapter;
use Symfony\Component\ExpressionLanguage\ParserCache\ParserCacheInterface;

/**
Expand All @@ -22,7 +24,7 @@
class ExpressionLanguage
{
/**
* @var ParserCacheInterface
* @var CacheItemPoolInterface
*/
private $cache;
private $lexer;
Expand All @@ -32,12 +34,21 @@ class ExpressionLanguage
protected $functions = array();

/**
* @param ParserCacheInterface $cache
* @param CacheItemPoolInterface $cache
* @param ExpressionFunctionProviderInterface[] $providers
*/
public function __construct(ParserCacheInterface $cache = null, array $providers = array())
public function __construct($cache = null, array $providers = array())
{
$this->cache = $cache ?: new ArrayParserCache();
if (null !== $cache) {
if ($cache instanceof ParserCacheInterface) {
@trigger_error(sprintf('Passing an instance of %s as constructor argument for %s is deprecated as of 3.2 and will be removed in 4.0. Pass an instance of %s instead.', ParserCacheInterface::class, self::class, CacheItemPoolInterface::class), E_USER_DEPRECATED);
$cache = new ParserCacheAdapter($cache);
} elseif (!$cache instanceof CacheItemPoolInterface) {
throw new \InvalidArgumentException(sprintf('Cache argument has to implement %s.', CacheItemPoolInterface::class));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

missing case when null !== $cache && !$cache instanceof CacheItemPoolInterface: throw InvalidArgumentException


$this->cache = $cache ?: new ArrayAdapter();
Copy link
Member

Choose a reason for hiding this comment

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

symfony/cache is for now a required dependency because of this very line (which maps to the removed one above).
To make symfony/cache an optional dependency, we could just set $this->cache to null and deal with it in the code.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep it as is. Previously, some sort of cache would always have been used. If we made the cache optional now, it would be possible that the performance decreased for some use cases.

Copy link
Member

Choose a reason for hiding this comment

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

What about wiring a cache pool in FrameworkBundle instead? That would mean less performance for those benefiting from the array cache and not using FrameworkBundle. But no perf drop for the other. WDYT?

$this->registerFunctions();
foreach ($providers as $provider) {
$this->registerProvider($provider);
Expand Down Expand Up @@ -91,13 +102,14 @@ public function parse($expression, $names)
$cacheKeyItems[] = is_int($nameKey) ? $name : $nameKey.':'.$name;
}

$key = $expression.'//'.implode('|', $cacheKeyItems);
$cacheItem = $this->cache->getItem(rawurlencode($expression.'//'.implode('|', $cacheKeyItems)));

if (null === $parsedExpression = $this->cache->fetch($key)) {
if (null === $parsedExpression = $cacheItem->get()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should check the isHit() instead

Copy link
Author

@ReDnAxE ReDnAxE Aug 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know if it does make a difference, $parsedExpression has to be set anyway, and will be set to null if !isHit()

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @ReDnAxE there is no need for isHit here and allows writting the code on a single line. Simpler to me.

$nodes = $this->getParser()->parse($this->getLexer()->tokenize((string) $expression), $names);
$parsedExpression = new ParsedExpression((string) $expression, $nodes);

$this->cache->save($key, $parsedExpression);
$cacheItem->set($parsedExpression);
$this->cache->save($cacheItem);
}

return $parsedExpression;
Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Component/ExpressionLanguage/ParserCache/ArrayParserCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,14 @@

namespace Symfony\Component\ExpressionLanguage\ParserCache;

@trigger_error('The '.__NAMESPACE__.'\ArrayParserCache class is deprecated since version 3.2 and will be removed in 4.0. Use the Symfony\Component\Cache\Adapter\ArrayAdapter class instead.', E_USER_DEPRECATED);

use Symfony\Component\ExpressionLanguage\ParsedExpression;

/**
* @author Adrien Brault <adrien.brault@gmail.com>
*
* @deprecated ArrayParserCache class is deprecated since version 3.2 and will be removed in 4.0. Use the Symfony\Component\Cache\Adapter\ArrayAdapter class instead.
*/
class ArrayParserCache implements ParserCacheInterface
{
Expand Down
120 changes: 120 additions & 0 deletions src/Symfony/Component/ExpressionLanguage/ParserCache/ParserCacheAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
<?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\Component\ExpressionLanguage\ParserCache;

use Psr\Cache\CacheItemInterface;
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 first

use Psr\Cache\CacheItemPoolInterface;
use Symfony\Component\Cache\CacheItem;

/**
* @author Alexandre GESLIN <alexandre@gesl.in>
*
* @internal This class should be removed in Symfony 4.0.
*/
class ParserCacheAdapter implements CacheItemPoolInterface
{
private $pool;
private $createCacheItem;

public function __construct(ParserCacheInterface $pool)
{
$this->pool = $pool;

$this->createCacheItem = \Closure::bind(
function ($key, $value, $isHit) {
$item = new CacheItem();
$item->key = $key;
$item->value = $value;
$item->isHit = $isHit;

return $item;
},
null,
CacheItem::class
);
}

/**
* {@inheritdoc}
*/
public function getItem($key)
Copy link
Member

Choose a reason for hiding this comment

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

I think you should not implement getItem, save nor __construct, but implement the protected do* method instead.

Copy link
Member

@nicolas-grekas nicolas-grekas Aug 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

OR change again and implement a subset of psr6 directly, like you did before, which was fine for an internal class.
(sorry for the flip flap)

Copy link
Author

@ReDnAxE ReDnAxE Aug 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

No problem ;) I will opt for an implementation of the PSR6 CacheItemPoolInterface directly. Less code, less dependencies, less work ^^

Copy link
Member

Choose a reason for hiding this comment

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

ok!

{
$value = $this->pool->fetch($key);
$f = $this->createCacheItem;
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Author

@ReDnAxE ReDnAxE Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, not working with $this->createCacheItem($key, $value, $isHit);
picked up here https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Adapter/ArrayAdapter.php#L75


return $f($key, $value, null !== $value);
}

/**
* {@inheritdoc}
*/
public function save(CacheItemInterface $item)
{
$this->pool->save($item->getKey(), $item->get());
}

/**
* {@inheritdoc}
*/
public function getItems(array $keys = array())
{
throw new \BadMethodCallException('Not implemented');
}

/**
* {@inheritdoc}
*/
public function hasItem($key)
{
throw new \BadMethodCallException('Not implemented');
}

/**
* {@inheritdoc}
*/
public function clear()
{
throw new \BadMethodCallException('Not implemented');
}

/**
* {@inheritdoc}
*/
public function deleteItem($key)
{
throw new \BadMethodCallException('Not implemented');
}

/**
* {@inheritdoc}
*/
public function deleteItems(array $keys)
{
throw new \BadMethodCallException('Not implemented');
}

/**
* {@inheritdoc}
*/
public function saveDeferred(CacheItemInterface $item)
{
throw new \BadMethodCallException('Not implemented');
}

/**
* {@inheritdoc}
*/
public function commit()
{
throw new \BadMethodCallException('Not implemented');
}
}
4 changes: 4 additions & 0 deletions src/Symfony/Component/ExpressionLanguage/ParserCache/ParserCacheInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,14 @@

namespace Symfony\Component\ExpressionLanguage\ParserCache;

@trigger_error('The '.__NAMESPACE__.'\ParserCacheInterface interface is deprecated since version 3.2 and will be removed in 4.0. Use Psr\Cache\CacheItemPoolInterface instead.', E_USER_DEPRECATED);

use Symfony\Component\ExpressionLanguage\ParsedExpression;

/**
* @author Adrien Brault <adrien.brault@gmail.com>
*
* @deprecated since version 3.2, to be removed in 4.0. Use Psr\Cache\CacheItemPoolInterface instead.
*/
interface ParserCacheInterface
{
Expand Down
Loading