php-cache / issues Goto Github PK

We need to update the readme with a link to the new issue tracker repo and also to be coherent with the Redis adapter. The composer.json file does also need an update. We need to set the homepage to http://www.php-cache.com.

This is a list of repositories that need to be updated:

• Apc
• Apcu
• Array
• Doctrine
• Filesystem
• Memcache
• Memcaced
• MongoDB
• Predis
• Redis
• Void
• Chain
• AdapterBundle
• Adapter common
• CacheBundle
• Doctrine Bridge
• Hierarchical cache
• Integration tets
• Session handler
• Tagglable cache

I added Twig_CacheInterface to the list. Reference: https://www.reddit.com/r/PHP/comments/413hpc/as_an_experiment_i_wrote_two_alternative/

I'm not sure about the name, but the interface from the psr.

This is a list of planned features:

• Logging - Log all cache transactions.
• Validation - It uses a cache from Symfony\Component\Validator\Mapping\Cache\CacheInterface
• Http - Instead of using disk, one maybe would like to use Redis for HttpCache
• Serializer - It uses a doctrine cache
• Annotation - It uses a doctrine cache
• Templating - This is just a string path to where templates should be stored
• Twig_CacheInterface

For each of these, investigate if it is a good idea to use a cache service.

From @Nyholm:

I added Twig_CacheInterface to the list. Reference: https://www.reddit.com/r/PHP/comments/413hpc/as_an_experiment_i_wrote_two_alternative/

From @andrerom:

Looking at this, I see there might be an issue here, depending on what the intention is here.

Current API

Cache tagging in the FOSHttpCache and Drupal 8 sense, is tags that you typically don't know about when requesting the item, you only know it once the item, or it's corresponding entity in database is loaded.

Example would be a cache item representing entity with id 40 of type=article with author id=10, then key might be 'entity:40' and tags would be ['type' => 'article', 'author', 10], allowing you to invalidate/expire all items tagged with either type article, or author 10 later on when something changes to them, including item 'entity:40'.

Further examples can be found in Drupal 8 API doc: https://api.drupal.org/api/drupal/core%21core.api.php/group/cache/8

Implementation of such approach

So afaik no cache backends supports tagging natively, I thought for a while Redis did, but can't find anything on that atm, pity.

For the items we can simply store tags inside same cache, ala ['value' => (...), 'tags' => [..]].

For invalidation we'll also need the reverse relation somewhat given we can't walk/query the cache.

Most likely the most performant and secure approach is to actually insert all tags to item key maps into a database*, this is what is done in D8 and currently also in progress in FOSHttpCache, at least I can't seem to avoid hitting the following issues when trying to look for a way to avoid that:

A. If we opt of storing tags separately where key is tag name and value is list of items tagged with it, then:

• Given most cache backends tend to drop items at any point if it starts to fill up some of it's cache blocks => if the tags to items map is wiped, effectively we don't know which items should be invalidated anymore.
• Storages like memcache does not have a notion of arrays, can use strings and append, but question is when you'll hit limits on that
• When clearing by tag we would need to be able to do a blocking read and replace/delete to be somewhat atomic on the tag itself
  • Once you have the item keys, a multi set / delete can wipe out or expire all items

B. If we opt for just handling tags using expiry (key is tag and value is expiry time), then:

• same as first cache tag being wiped issue above
• we now need to do several reads, once we have a cache item, if it has tags we will need to do a multi get to get expiry time stamp for all relevant tags => As I'm personally keen on avoiding several round trips to cache server, this is thus most likely not an option

In database on the other side, it is only read on invalidation, and only written to on save calls when item has tags.

Allow using DSN for connections on drivers like Redis and Mongo

redis://localhost:6379
mongodb://user:pass@localhost:27107/

Having a look at the CacheItem implementation.

The type hint on the getExpiration method should probably be DateTimeInterface (on CacheItem and HasExpirationDateInterface) as I believe the constructor allows these objects.

Also, all date time objects should be immediately cloned once they have been passed to __construct or expiresAt. Otherwise later changes to the object will change the expiration of the cache item.

From @AndrewCarterUK

I'm not sure if you will want to go down this route - but here is a proof of concept approach to handling expiresAt and expiresAfter without memory leaks or supplementing the PSR-6 interface with additional methods:

<?php

class Communicator
{
    private $store;

    public function __construct()
    {
        $this->store = array();
    }

    public function set($key, $expiration)
    {
        $this->store[$key] = $expiration;
    }

    public function get($key)
    {
        return isset($this->store[$key]) ? $this->store[$key] : null;
    }

    public function clear($key)
    {
        unset($this->store[$key]);
    }
}

class Item
{
    private $key;
    private $communicator;

    public function __construct($key, Communicator $communicator)
    {
        $this->key = $key;
        $this->communicator = $communicator;
    }

    public function __destruct()
    {
        echo 'Removing key from communicator...' . PHP_EOL;
        $this->communicator->clear($this->key);
    }

    public function getKey()
    {
        return $this->key;
    }

    public function setExpiration($expiration)
    {
        $this->communicator->set($this->key, $expiration);
    }
}

class Pool
{
    private $communicator;

    public function __construct()
    {
        $this->communicator = new Communicator();
    }

    public function getItem($key)
    {
        return new Item($key, $this->communicator);
    }

    public function save(Item $item)
    {
        $expiration = $this->communicator->get($item->getKey());
        echo 'Expiration: ' . $expiration . PHP_EOL;
    }
}

function test() {
    $pool = new Pool();
    $item = $pool->getItem('foo');
    $item->setExpiration('bar');
    $pool->save($item);
}

test();

echo 'Done' . PHP_EOL;

Output:

Expiration: bar
Removing key from communicator...
Done

https://3v4l.org/K1ceR

As far as I can see if two or more processes are using a TaggableCache appendListItem can cause race conditions.

Whenever an item is added to the cache and saveTags is called from the trait this uses appendListItem to add keys the the cached array of keys for that tag.

1. process A saves an item and thus gets the array of tags to update
2. process B saves and item and also get the array of tags to update
3. process A saves the array
4. process B saves the array
5. The key index now only includes the tagged key form process B and not process A

This could be solved with some kind of locks being used?

Fatal Error: Argument 3 passed to Cache\CacheBundle\Factory\DoctrineBridgeFactory::get() must be of the type array, string given

Defined at https://github.com/php-cache/cache-bundle/blob/master/src/DependencyInjection/CacheExtension.php#L127
making that array fixes the issue

Same problem may be with serializer. Not tested.
https://github.com/php-cache/cache-bundle/blob/master/src/DependencyInjection/CacheExtension.php#L136

When I require a cache pool that has taggs in my app I would like to type hint against something that allow me to use tag but also include the CacheItemPoolInterface::save.

I can't use TaggablePoolInterface since it does not have a save function. Is it a good solution to create an interface that extends both TaggablePoolInterface and CacheItemPoolInterface and make sure adapters implement the new interface?

Hi, I am getting what appears to be random Critical error messages from time to time on our live environment such as this (domain name replaced):

request.CRITICAL: Uncaught PHP Exception Cache\Adapter\Common\Exception\InvalidArgumentException: "Invalid key: "www.mydomain.co.uk__.https:..maps.google.co.uk.maps__GET__f=q". The key contains one or more characters reserved for future extension: {}()/@:" at /var/www/releases/20160317121445/vendor/cache/adapter-common/src/AbstractCachePool.php line 241 {"exception":"[object](Cache\Adapter\Common\Exception\InvalidArgumentException%28code: 0%29: Invalid key: "www.mydomain.co.uk__.https:..maps.google.co.uk.maps__GET__f=q". The key contains one or more characters reserved for future extension: {}%28%29/@: at /var/www/releases/20160317121445/vendor/cache/adapter-common/src/AbstractCachePool.php:241)"} []

and this:

request.CRITICAL: Uncaught PHP Exception Cache\Adapter\Common\Exception\InvalidArgumentException: "Invalid key: "www.mydomain.co.uk__.index.php__GET__q=user/register". The key contains one or more characters reserved for future extension: {}()/@:" at /var/www/releases/20160317121445/vendor/cache/adapter-common/src/AbstractCachePool.php line 241 {"exception":"[object](Cache\Adapter\Common\Exception\InvalidArgumentException%28code: 0%29: Invalid key: "www.mydomain.co.uk__.index.php__GET__q=user/register". The key contains one or more characters reserved for future extension: {}%28%29/@: at /var/www/releases/20160317121445/vendor/cache/adapter-common/src/AbstractCachePool.php:241)"} []

In the second case it looks like it is because of the forward slash, but I don't know about the first one. More interestingly I don't have an index.php so why is that in there? Is this a bug in the router cache?

Thanks

Sorry for being awfully late with this idea.
Should we run tests like this?: https://travis-ci.org/symfony/symfony/jobs/106087023

The benefit is that we will see which adapter that fails and that we make sure the composer.json is correct.

The Symfony bundle has a toolbar logo that needs to be updated. The new logo is I the documentation repo.

Something like this: http://symfony.com/blog/new-in-symfony-2-6-lockhandler

Should only support adapters that have atomic functionality (Redis, APC, etc)

We need Doctrine factories for:

• Apc
• Apcu
• Array
• Couchbase
• File
• Filesystem
• Memcache
• Memcached
• MongoDb
• PhpFile
• Predis
• Redis
• Riak
• SQLite3
• Void
• WinCache
• Xcache
• ZendData

We do not have to do them all. Just do some popular and the once we do not have any adapters for.

I noticed a strange behavior. When I use null as argument for expiresAfter, everything is fine. (I use Redis, so the non-expiring SET operation is used.)

$item = $pool->getItem('some key');
$item->set('some value');
$item->expiresAfter(null);
$pool->save($item); // works fine: "SET" "[some key][1]" ...

But when is use 0 as argument for expiresAfter, the NULL value is cached.

$item = $pool->getItem('some key');
$item->set('some value');
$item->expiresAfter(0);
$pool->save($item); // strange: "SET" "[some key][1]" "s:2:\"N;\";"

I think, the problem is, that $item->setExpiresAfter(0) immediately makes the $item->isHit() return false. This is ok, but the no-longer-hit $item is saved anyway. Maybe $pool->save($item) should fail, if false === $item->isHit()?

Hello everyone,

First, thanks for your work on those different bundles!

I'm currently monitoring my app with newrelic and i can see a 180 ms latency (this an average but it never goes down even after a week) only attributed to memcached which is unlikely to be normal.
Are you aware of any performance issue concerning the memcached bundle ?

The cache key must support spaces (which is not a part of the spec). It must also support the exclamation mark "!".

This should be documented.

Is that still true? Why should it support spaces?

Make sure the examples in the docs does not have any spaces and verify that spaces aren't needed.

It uses invalid characters in the cache key.

Make sure to generate a better key,
https://github.com/php-cache/cache-bundle/blob/master/src/Routing/CachingRouter.php#L128
https://github.com/php-cache/cache-bundle/blob/master/src/Routing/CachingRouter.php#L145

This is a blocker for tagging version 0.3

I get the following notice thrown in

Cache\CacheBundle\Factory\DoctrineBridgeFactory

But I’m not sure where to put this in the config to fix it.

We should explain that the repo is for development only. If they need an adapter they should install the adapter directly. We should also refer users to the website.

Just above the "framework integration" there should be installation instructions. It should just say that you need to use composer to install an adapter and the reference to the packet's github page

This repo isn't deprecated now that cache/cache exists?

While caching is often an important part of application performance, it should never be a critical part of application functionality. Thus, an error in a cache system SHOULD NOT result in application failure. For that reason Implementing Libraries MUST NOT throw exceptions other than those defined by the interface, and SHOULD trap any errors or exceptions triggered by an underlying data store and not allow them to bubble.

An Implementing Library SHOULD log such errors or otherwise report them to an administrator as appropriate.

Let's make the AbstractPool implement Psr\Log\LoggerAwareInterface and add the setLogger() function. Then do a try catch to fetch every \Exception we can. We should only re-throw our InvalidArgumentException.

Hi !

Why not create a meta project that requires all adapaters? This project will contain only a composer.json that would look like this:

{
    "name": "cache/cache",
    "require": {
        "cache/apc-adapter": "^0.1",
        "cache/apcu-adapter": "^0.1",
        "cache/array-adapter": "^0.1",
        "..."
    }
}

Like that, lazy developers (as me) just have to do composer require cache/cache.

https://github.com/snc/SncRedisBundle/blob/master/Session/Storage/Handler/RedisSessionHandler.php

https://www.reddit.com/r/PHP/comments/43og98/using_redis_for_php_session_storage/czjs3w9

We need to update all our adapters to use cache/adapter-common: ^0.2 and cache/taggable-cache:^0.3. These updates will not be backwards compatible.

• Apc
• Apcu
• Array
• Doctrine
• Filesystem
• Memcache
• Memcaced
• MongoDB
• Predis
• Redis
• Void
• Chain
• CacheBundle
• Hierarchical cache

These are the adapters that we should support.

• Apc https://github.com/php-cache/apc-adapter
• Apcu https://github.com/php-cache/apcu-adapter
• Array https://github.com/php-cache/array-adapter
• Chain https://github.com/php-cache/chain-adapter
• Couchbase ?
• Doctrine Cache
• File
• Filesystem (flysystem) https://github.com/php-cache/filesystem-adapter
• Memcache https://github.com/php-cache/memcache-adapter
• Memcached https://github.com/php-cache/memcached-adapter
• Mongodb https://github.com/php-cache/mongodb-adapter
• PHPFile ?
• Predis https://github.com/php-cache/predis-adapter
• Redis https://github.com/php-cache/redis-adapter
• Riak ?
• SQLite
• Void https://github.com/php-cache/void-adapter
• WinCache ?
• Xcache ?
• ZendData

Does it give anything that other tests does not have?

So that we sure that it works on windows too.

The Session handler is reusable. I think it should be moved to a separate package. I do also want to revisit my decision of calling it the SessionHandlerBridge.

From @andrerom

By design this has a high risk of filling up memory, and causing stale data and corresponding issues in applications (the longer application is running the higher risk that some other request/server/process has invalidated items cached here).

Same issue exists in Stash, where similar driver in lib is documented for unit test, but Bundle is exposing option for end users to enable "InMemory" cache without really documenting the issues it can cause.

Possible solutions

A. Clearly documented that this is for testing only, and never use in any doc or examples or bundles or bridges, other then for internal tests.

B. Aim to support in prod where people want performance at own risk, then somewhat fix the issues:

• Add limit (100?) to avoid filling up memory, clear in first in first out order to recycle items over time
• Add flag to getItem(s) where application developer can specify that stale data is allowed on a use case by use case basis, pass this argument to adapter and only return items from this array cache if application accepts stale data.

With B this adapter can be somewhat reliably be used with Chain adapter in front of adapter serving the real shared cache pool, at least assuming the developer using api knows his domain well enough to know when he can safely use stale data.

Consider this:

// T = 0
$item = $pool->getItem('key');
$item->set('value');
$item->expiresAfter(3600);
$pool->save($item);

// T = 600
$item = $pool->getItem('key');
$item->set('foobar');
$pool->save($item);

For how long is the item still in cache? The way I see it there are three options:

• A) Until T=3600
• B) Until T=4200
• C) Forever since $item->expiresAfter() was never called.

I believe that option A is preferable.

This question has a lot of impact on performance. If C is the right way we do not have to read to the cache before doing a store.

EDIT Correct answer is C

From @Nyholm: The correct answer was C according to the mailing list. We need to write a test for that.

I get the following error when using doctrine caching and I’m not sure how to change the name of the cache key.

Invalid key: ”MyNameSpace\CoreBundle\Entity\User$CLASSMETADATA”. The key contains one or more characters reserved for future extension: {}()/@:

The TaggablePoolInterface extends CacheItemPoolInterface. It will also makes sense when you typehint for a HierarchicalPoolInterface

We should abandon this bundle because its functionality is replaced by AdapterBundle.

Make sure to:

• Update the readme
• Update the readme at CacheBundle
• Update the readme at DoctrineAdapter
• Abandon the package on packagist.

We need a new logo!

Suggestions are open, and welcome!

@kynx wrote on php-cache/integration-tests#43 (comment)

I think collecting all the places where PSR-6 is a bit vague into one place (maybe a wiki on this repo?) would be a valuable thing. Ultimately it would be for FIG to issue an addendum or whatever - which may take years ;) - but in the meantime it would be a valuable resource for implementers.

I believe this is a good idea. We should document points to explain our interpretation of the methods and functionality in PSR-6.

As I see it we can put the documentation in four places:

1. A wiki the integration tests.
2. A wiki on the main repository
3. As documentation (normal markdown files) on the integration tests repo
4. A new section on PHP-cache.com

Any thoughts?

We should fix a version number instead.

When I try to clear the cache using app/console cache:flush all I get the following warning:

Warning: call_user_func_array() expects parameter 1 to be a valid callback, class 'Cache\Adapter\Memcached\MemcachedCachePool' does not have a method 'clearTags'

In the following method in Cache\CacheBundle\CommandCacheFlushCommand

private function clearTypedCacheFromService($type, $serviceId)
    {
        /** @type \Psr\Cache\CacheItemPoolInterface $service */
        $service = $this->getContainer()->get($serviceId);
        if ($service instanceof TaggablePoolInterface) {
            return $service->clearTags([$type]);
        } else {
            return $service->clear();
        }
    }

Cache\Adapter\Memcached\MemcachedCachePool does not implement TaggablePoolInterface. However, in Cache\CacheBundle\Command\CacheFlushCommand -> clearTypedCacheFromService() the service IS an instance of TaggablePoolInterface although when I dump $service here I get

Cache\CacheBundle\Cache\RecordingCachePool {#450
-calls: []
-cachePool: Cache\Adapter\Memcached\MemcachedCachePool {#446
-cache: Memcached {#449}
#deferred: []
-keyCache: []
}
}

If I temporarily return $service->clear(); then I can get the cache to clear. However, after returning the code to it's original state and then running my web application, I then get the following:

Argument 1 passed to Cache\CacheBundle\Cache\FixedTaggingCachePool::__construct() must be an instance of Cache\Taggable\TaggablePoolInterface, instance of Cache\Adapter\Memcached\MemcachedCachePool given, called in /***********/vendor/cache/cache-bundle/src/Factory/DoctrineBridgeFactory.php on line 33 and defined', '/*******/vendor/cache/cache-bundle/src/Cache/FixedTaggingCachePool.php', '41', array()) in FixedTaggingCachePool.php line 41

I'm not sure why tagging is a requirement anyway, as I am not using tagging myself.

From @maidmaid

Currently, there are one autoload for each adapter. I watched how Symfony did made them subtree splits and I note that source code is at first level folder, without src folder (for example, console component in symfony project and subtree split console). By this way, autoload section in composer.json is very simple.

The documentation needs to be improved. It is poor English and many of the headings needs to be rewritten to be more clear.

@Nyholm

There is an issue here, I don't know if we should bother or not. But what if we start request A and lots of tags will get into the temporary storage. At the same time request B will invalidate all these tags. A will not see that the tags are invalid.

This is only an issue with long running requests. If A takes less than 200 ms as it should, this would not be a problem.

Any thoughts?

@aequasi

A warning should be made about that i think.

This is big problem in my project, because for every request router loads routes from files and that takes one second.
Is there any reason for not caching route collection?
I cannot use php-cache untill route collection is cached. I'm actually using Aequasi cache-bundle which was replaced by php-cache. https://github.com/aequasi/cache-bundle#this-bundle-is-deprecated

Issue: Right now, when we run that script, SessionHandler and Bridge both have the integration-tests requirement, when they don't need it

Also, we could do this:

Update all the libraries so they just require * version or something, and then during the travis build, update them.

Nevertheless it should never be used in theory, symfony IDE plugins might resolve it and complain about not implementing the interface. It should probably implement it and throw logic exceptions in every method.