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

[Cache] Symfony PSR-6 implementation #17408

Merged
merged 1 commit into from Jan 19, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.php
Expand Up @@ -39,7 +39,7 @@

$packages[$package->name][$package->version] = $package;

$versions = file_get_contents('https://packagist.org/packages/'.$package->name.'.json');
$versions = @file_get_contents('https://packagist.org/packages/'.$package->name.'.json') ?: '{"package":{"versions":[]}}';
$versions = json_decode($versions);

foreach ($versions->package->versions as $version => $package) {
Expand Down
2 changes: 1 addition & 1 deletion .travis.yml
Expand Up @@ -51,7 +51,7 @@ install:
- if [[ $deps != skip ]]; then COMPONENTS=$(find src/Symfony -mindepth 3 -type f -name phpunit.xml.dist -printf '%h\n'); fi;
- if [[ $deps != skip && $deps ]]; then php .travis.php $TRAVIS_COMMIT_RANGE $TRAVIS_BRANCH $COMPONENTS; fi;
- if [[ $deps = high && $TRAVIS_BRANCH = master ]]; then SYMFONY_VERSION=$(git ls-remote --heads | grep -o '/[1-9].*' | tail -n 1 | sed s/.//); else SYMFONY_VERSION=$(cat composer.json | grep '^ *"dev-master". *"[1-9]' | grep -o '[0-9.]*'); fi;
- if [[ $deps = high && $TRAVIS_BRANCH = master ]]; then git fetch origin $SYMFONY_VERSION; git checkout -m FETCH_HEAD; fi;
- if [[ $deps = high && $TRAVIS_BRANCH = master ]]; then git fetch origin $SYMFONY_VERSION; git checkout -m FETCH_HEAD; COMPONENTS=$(find src/Symfony -mindepth 3 -type f -name phpunit.xml.dist -printf '%h\n'); fi;
- if [[ $deps = high && ${SYMFONY_VERSION%.*} != $(git show $(git ls-remote --heads | grep -FA1 /$SYMFONY_VERSION | tail -n 1):composer.json | grep '^ *"dev-master". *"[1-9]' | grep -o '[0-9]*' | head -n 1) ]]; then LEGACY=,legacy; fi;
- export COMPOSER_ROOT_VERSION=$SYMFONY_VERSION.x-dev;
- if [[ ! $deps ]]; then composer update --prefer-dist; else export SYMFONY_DEPRECATIONS_HELPER=weak; fi;
Expand Down
4 changes: 4 additions & 0 deletions composer.json
Expand Up @@ -19,6 +19,7 @@
"php": ">=5.5.9",
"doctrine/common": "~2.4",
"twig/twig": "~1.23|~2.0",
"psr/cache": "~1.0",
"psr/log": "~1.0",
"symfony/polyfill-apcu": "~1.0,>=1.0.2",
"symfony/polyfill-intl-icu": "~1.0",
Expand All @@ -30,6 +31,7 @@
"replace": {
"symfony/asset": "self.version",
"symfony/browser-kit": "self.version",
"symfony/cache": "self.version",
"symfony/class-loader": "self.version",
"symfony/config": "self.version",
"symfony/console": "self.version",
Expand Down Expand Up @@ -74,6 +76,8 @@
"symfony/yaml": "self.version"
},
"require-dev": {
"cache/integration-tests": "^0.6",
"doctrine/cache": "~1.6",
"doctrine/data-fixtures": "1.0.*",
"doctrine/dbal": "~2.4",
"doctrine/orm": "~2.4,>=2.4.5",
Expand Down
2 changes: 2 additions & 0 deletions phpunit.xml.dist
Expand Up @@ -54,6 +54,8 @@
<listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener">
<arguments>
<array>
<element><string>Cache\IntegrationTests</string></element>
<element><string>Doctrine\Common\Cache</string></element>
<element><string>Symfony\Component\HttpFoundation</string></element>
</array>
</arguments>
Expand Down
3 changes: 3 additions & 0 deletions src/Symfony/Component/Cache/.gitignore
@@ -0,0 +1,3 @@
composer.lock
phpunit.xml
vendor/
292 changes: 292 additions & 0 deletions src/Symfony/Component/Cache/Adapter/AbstractAdapter.php
@@ -0,0 +1,292 @@
<?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\Cache\Adapter;

use Psr\Cache\CacheItemInterface;
use Psr\Cache\CacheItemPoolInterface;
use Symfony\Component\Cache\CacheItem;
use Symfony\Component\Cache\Exception\InvalidArgumentException;

/**
* @author Nicolas Grekas <p@tchwork.com>
*/
abstract class AbstractAdapter implements CacheItemPoolInterface
{
private $namespace;
private $deferred = array();
private $createCacheItem;
private $mergeByLifetime;

protected function __construct($namespace = '', $defaultLifetime = 0)
{
$this->namespace = $namespace;
$this->createCacheItem = \Closure::bind(
function ($key, $value, $isHit) use ($defaultLifetime) {
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 adding a constructor to CacheItem marked with @internal? It will be easier to understand. (Currently even PHPStorm isn't able to interpret this snippet and marks it in red).

Copy link
Member

Choose a reason for hiding this comment

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

Btw it will remove the duplication of this factory closure in ProxyAdapter.

Copy link
Member Author

Choose a reason for hiding this comment

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

I preferred raw speed over phpstorm compliance: a constructor would slow down things for no valid reason (adding one function call in the chain). I doubt @internal would be enough to prevent erroneous user side instantiation. The current no-constructor-nor-setter (for key and isHit) means we technically annoy users enough so that they are free to missuse but forced to think about it :)

Copy link
Member

Choose a reason for hiding this comment

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

Isn't a constructor call or a closure call in the chain similar in term of perfs?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes of course, but making the constructor public has drawback IMHO (as detailed above). This drawback could be fixed by using a private constructor + a rebound closure, which is when an additional function call comes up. That's what I meant :)

$item = new CacheItem();
$item->key = $key;
$item->value = $value;
$item->isHit = $isHit;
$item->defaultLifetime = $defaultLifetime;

return $item;
},
$this,
CacheItem::class
);
$this->mergeByLifetime = \Closure::bind(
function ($deferred, $namespace) {
$byLifetime = array();

foreach ($deferred as $key => $item) {
if (0 <= $item->lifetime) {
$byLifetime[(int) $item->lifetime][$namespace.$key] = $item->value;
}
}

return $byLifetime;
},
$this,
CacheItem::class
);
}

/**
* Fetches several cache items.
*
* @param array $ids The cache identifiers to fetch.
*
* @return array The corresponding values found in the cache.
*/
abstract protected function doFetch(array $ids);

/**
* Confirms if the cache contains specified cache item.
*
* @param string $id The identifier for which to check existence.
*
* @return bool True if item exists in the cache, false otherwise.
*/
abstract protected function doHave($id);

/**
* Deletes all items in the pool.
*
* @return bool True if the pool was successfully cleared, false otherwise.
*/
abstract protected function doClear();

/**
* Removes multiple items from the pool.
*
* @param array $ids An array of identifiers that should be removed from the pool.
*
* @return bool True if the items were successfully removed, false otherwise.
*/
abstract protected function doDelete(array $ids);

/**
* Persists several cache items immediately.
*
* @param array $values The values to cache, indexed by their cache identifier.
* @param int $lifetime The lifetime of the cached values, 0 for persisting until manual cleaning.
*
* @return array|bool The identifiers that failed to be cached or a boolean stating if caching succeeded or not.
*/
abstract protected function doSave(array $values, $lifetime);

/**
* {@inheritdoc}
*/
public function getItem($key)
{
$id = $this->getId($key);

if ($this->deferred) {
$this->commit();
}
if (isset($this->deferred[$key])) {
return $this->deferred[$key];
}

$f = $this->createCacheItem;
$isHit = false;
$value = null;

foreach ($this->doFetch(array($id)) as $value) {
$isHit = true;
}

return $f($key, $value, $isHit);
}

/**
* {@inheritdoc}
*/
public function getItems(array $keys = array())
{
if ($this->deferred) {
$this->commit();
}
$f = $this->createCacheItem;
$ids = array();
$items = array();

foreach ($keys as $key) {
$id = $this->getId($key);

if (isset($this->deferred[$key])) {
$items[$key] = $this->deferred[$key];
} else {
$ids[$key] = $id;
}
}

$values = $this->doFetch($ids);

foreach ($ids as $key => $id) {
$isHit = isset($values[$id]);
$items[$key] = $f($key, $isHit ? $values[$id] : null, $isHit);
}

return $items;
}

/**
* {@inheritdoc}
*/
public function hasItem($key)
{
if ($this->deferred) {
$this->commit();
}

return $this->doHave($this->getId($key));
}

/**
* {@inheritdoc}
*/
public function clear()
{
$this->deferred = array();

return $this->doClear();
}

/**
* {@inheritdoc}
*/
public function deleteItem($key)
{
return $this->deleteItems(array($key));
}

/**
* {@inheritdoc}
*/
public function deleteItems(array $keys)
{
$ids = array();

foreach ($keys as $key) {
$ids[] = $this->getId($key);
unset($this->deferred[$key]);
}

return $this->doDelete($ids);
}

/**
* {@inheritdoc}
*/
public function save(CacheItemInterface $item)
{
if (!$item instanceof CacheItem) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to do this?
It breaks the usage of CacheItemInterface interface used as type hinting.

Copy link
Contributor

Choose a reason for hiding this comment

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

@blanchonvincent the PSR specifies that implementation are not compatible. So the type hinting is specified by the PSR interface, but the $item MUST be an instance from the same implementation (i.e: I cannot make work my CacheItem from my custom Redis implementation to work with your custom Memcached implementation of CacheItemPool).

Copy link
Contributor

Choose a reason for hiding this comment

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

You breaking the whole idea of PSR-6 you can interchange multiple libraries together... That's why Interfaces exists

Copy link
Member

Choose a reason for hiding this comment

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

@GCDS did you read @xavierleune's comment just in top of yours?

Copy link
Member

Choose a reason for hiding this comment

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

From the PSR itself:

Calling Libraries SHOULD NOT assume that an Item created by one Implementing Library is compatible with a Pool from another Implementing Library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, sorry I thought differently...

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, Interoperability between implementations is possible using something like the attached ProxyAdapter

return false;
}
$key = $item->getKey();
$this->deferred[$key] = $item;
$this->commit();

return !isset($this->deferred[$key]);
}

/**
* {@inheritdoc}
*/
public function saveDeferred(CacheItemInterface $item)
{
if (!$item instanceof CacheItem) {
return false;
}
try {
$item = clone $item;
} catch (\Error $e) {
} catch (\Exception $e) {
}
if (isset($e)) {
@trigger_error($e->__toString());

return false;
}
$this->deferred[$item->getKey()] = $item;

return true;
}

/**
* {@inheritdoc}
*/
public function commit()
{
$f = $this->mergeByLifetime;
$ko = array();
$namespaceLen = strlen($this->namespace);

foreach ($f($this->deferred, $this->namespace) as $lifetime => $values) {
if (true === $ok = $this->doSave($values, $lifetime)) {
continue;
}
if (false === $ok) {
$ok = array_keys($values);
}
foreach ($ok as $failedId) {
$key = substr($failedId, $namespaceLen);
$ko[$key] = $this->deferred[$key];
}
}

return !$this->deferred = $ko;
}

public function __destruct()
{
if ($this->deferred) {
$this->commit();
}
}

private function getId($key)
{
if (!is_string($key)) {
throw new InvalidArgumentException(sprintf('Cache key must be string, "%s" given', is_object($key) ? get_class($key) : gettype($key)));
}
if (!isset($key[0])) {
throw new InvalidArgumentException('Cache key length must be greater than zero');
}
if (isset($key[strcspn($key, '{}()/\@:')])) {
throw new InvalidArgumentException('Cache key contains reserved characters {}()/\@:');
}

return $this->namespace.$key;
}
}