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

SimpleCacheBridge::setMultiple() not compatible with NamespacedCachePool? #139

Open
beporter opened this issue Dec 4, 2018 · 4 comments
Open

Comments

@beporter
Copy link

@beporter beporter commented Dec 4, 2018

I've encountered an issue trying to combine a NamespacedCachePool with the SimpleCacheBridge.

Environment:

$ php --version
PHP 7.2.12-1+ubuntu16.04.1+deb.sury.org+1 (cli) (built: Nov 12 2018 09:55:12) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.2.12-1+ubuntu16.04.1+deb.sury.org+1, Copyright (c) 1999-2018, by Zend Technologies
    with Xdebug v2.6.1, Copyright (c) 2002-2018, by Derick Rethans

$ composer info
cache/cache                           1.0.0   Library of all the php-cache adapters
cache/simple-cache-bridge             1.0.0   A PSR-6 bridge to PSR-16. This will make any PSR-6 cache compatible with SimpleCache.
psr/cache                             1.0.1   Common interface for caching libraries
psr/simple-cache                      1.0.1   Common interfaces for simple caching
# **snip**

Example php file to reproduce:

#!/usr/bin/env php
<?php
require 'vendor/autoload.php';

use Cache\Adapter\PHPArray\ArrayCachePool;
use Cache\Bridge\SimpleCache\SimpleCacheBridge;
use Cache\Namespaced\NamespacedCachePool;

$pool = new SimpleCacheBridge(
    new NamespacedCachePool(
        new ArrayCachePool(),
        'my_prefix'
    )
);

$pool->setMultiple([
    'key1' => 'val1',
    'key2' => 'val2',
]);

Error:

$ php cachetest.php
PHP Notice:  Undefined index: |my_prefix|key1 in ./vendor/cache/simple-cache-bridge/SimpleCacheBridge.php on line 177
PHP Stack trace:
PHP   1. {main}() ./cachetest.php:0
PHP   2. Cache\Bridge\SimpleCache\SimpleCacheBridge->setMultiple() ./cachetest.php:17

The issue seems to be here:

        $items = $this->cacheItemPool->getItems($keys);

        // **snip**

        foreach ($items as $key => $item) {
            /* @var $item CacheItemInterface */
            $item->set($arrayValues[$key]);
            // ...

In $arrayValues[$key], the $key being used includes the prefix that was auto-added by NamespacedCachePool:: prefixValue($key), but $arrayValues is provided by the end user and is therefore not prefixed yet (nor should it be.) One possible fix might be to somehow strip the prefix (if present) from $key before attempting to access $arrayValues[$key].

@beporter
Copy link
Author

@beporter beporter commented Dec 4, 2018

As a related question: Why does NamespacedCachePool::getItems(...) not strip the prefix from the indices of the values it returns? For example:

$pool = new NamespacedCachePool(
    new ArrayCachePool(),
    'my_prefix'
);

$data = [
    'key1' => 'val1',
    'key2' => 'val2',
];

foreach ($data as $key =>$val) {
    $item = $pool->getItem($key);
    $item->set($val);
    $pool->save($item);
}

$identityTest = $pool->getItems(array_keys($data));

// Expected: $identityTest === $data
// Actual: $identityTest !== $data  (Keys in $identityTest contain `|my_prefix|` whereas keys in $data do not.)

Correcting this could be another possible fix for the current issue, but seems like much more of a breaking change.

@beporter
Copy link
Author

@beporter beporter commented Dec 5, 2018

As a workaround (and in case it helps anyone else), I managed to override the NamespacedCachePool to remove the prefix from the indices returned from ::getItems().

<?php
/**
 * Fixes https://github.com/php-cache/issues/issues/139
 *
 * @TODO: Remove this class when php-cache/issues#139 is fixed.
 */

namespace App\Lib;

use Cache\Hierarchy\HierarchicalPoolInterface;
use Cache\Namespaced\NamespacedCachePool as NamespacedCachePoolOrig;
use \Traversable;

/**
 * \App\Lib\NamespacedCachePool
 */
class NamespacedCachePool extends NamespacedCachePoolOrig
{
    /**
     * Store the calculated prefix string.
     *
     * @var string
     */
    protected $prefix;

    /**
     * Have to override the constructor to get access to $namespace because
     * ::$namespace and ::prefixValue() are both declared as `private` in
     * the parent class.
     *
     * @param HierarchicalPoolInterface $cachePool
     * @param string                    $namespace
     */
    public function __construct(HierarchicalPoolInterface $cachePool, $namespace)
    {
        parent::__construct($cachePool, $namespace);

        $this->prefix = HierarchicalPoolInterface::HIERARCHY_SEPARATOR
            . $namespace
            . HierarchicalPoolInterface::HIERARCHY_SEPARATOR;
    }

    /**
     * Overrides the normal ::getItems() to properly strip the namespace prefix
     * from the returned indices.
     *
     * @param array $keys
     * @return array Sets of [un-prefixed-keys => values].
     */
    public function getItems(array $keys = [])
    {
        return iterator_to_array($this->yieldWithoutPrefix(parent::getItems($keys)));
    }

    /**
     * Generator that does the work of stripping the namespace prefix from keys in $items.
     *
     * @param array|\Traversable $items The results returned from the underlying cache store.
     * @return array Same results with key names stripped of the namespace prefix.
     */
    protected function yieldWithoutPrefix($items)
    {
        foreach ($items as $key => $val) {
            yield str_replace($this->prefix, '', $key) => $val;
        }
    }
}
@sbol-coolblue
Copy link

@sbol-coolblue sbol-coolblue commented Jan 23, 2019

Thanks @beporter ! Just chiming in to report that this issue is also present in the PrefixedCachePool: getItems() prefixes the keys, then does a return $this->cachePool->getItems();, thus yielding the prefixed keys. Fully agree that this seems like a mistake.

@prisis
Copy link
Member

@prisis prisis commented Jun 7, 2019

I will take a look on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants