Interesting, like the simplistic approach, and especially that there is only one backend lookup for getItem call. But missing use of multi get / set / (...) to reduce backend roundtrips further on getItems(array) and batch deferred updates. And calls to hasItem and getItem(and opposite) will result in two lookups here, but last point is maybe to be expected given the spec.

I know@Nyholm,@aequasi and others have beenworking on this topic, and hope to be able to contribute from eZ as well.

Could be an opportunity to join forces.

Side: On design side would perhaps opt for following approach:

• 2 layers, the service (api) psr6 layer, and the adapter/driver layer with more low level interfaces, to avoid conflicts with future PSR's
• Aim to support Tagging for reliable cache expiry on entity changes in most real world applications

Thank you for the ping.

I ran this implementation on ourintegration tests and found some issues. Like you do not protect for all the reserved characters and you have forgotten to clear the deferred items on$pool->clear().

Thephp-cache supports tagging, hierarchal keys and have doctrine bridges in both directions. We are currently outperforming other PSR-6 caches with a factor of 2 and we have ideas for even more performance improvements.

We would definitely like to join forces somehow. We are open for a discussion about that.

Comments addressed, thank you@Nyholm for the link to the integration test, this will definitely help.
As I tried to state in the README, I target a strict implementation of PSR-6 here. The goal is not to replace any existing caching libraries (php-cache, stash, doctrine/cache, etc.) but only provide the features that have be standardized, so that we can build interoperability on top. This means that adding more interfaces or features (tagging) is out of scope, unless a new PSR standardize them.

I added testing with cache/integration-tests, I'll look at the failures asap. I already fixed the missing: reserved char.

I target a strict implementation of PSR-6 here. The goal is not to replace any existing caching libraries (php-cache, stash, doctrine/cache, etc.) but only provide the features that have be standardized, so that we can build interoperability on top.

If the goal is not to replace any existing cache libraries, why build another cache library? Wouldn't it be better if all Symfony components using cache relied on the PSR6 interface and then required the virtual packagepsr/cache-implementation: ~1.0?

That way we use the benefit of PSRs and we can let the user decide what cache library to use. If they like to use feature X they install a library with feature X. If they want a strict implementation they would choose a strict implementation.

@Nyholm ... and this component is a strict implementation that users can use if they want to, like any other ones. The goal of PSRs (if I get this right) is to allow several implementations with different philosophies, not to standardize on only one.

@Nyholm ... and this component is a strict implementation that users can use if they want to, like any other ones. The goal of PSRs (if I get this right) is to allow several implementations with different philosophies, not to standardize on only one.

You are correct. That is the goal of PSRs. But it was this sentence I reacted to:

The goal is not to replace any existing caching libraries

If the goalis to provide a strict implementation of PSR-6 thatwill replace (or be an option to) any existing library, then I agree that we need this component. Otherwise I do not see the benefits at all.

Understood, we definitely want to provide a strict implementation thatcan replace existing libraries :)

Okey, awesome.

Im happy to help.@nicolas-grekas do you want PR's with more adapters now or once this is merged?

@Nyholm thank you that's much appreciated. Your test suite is already an amazing help. I think it would be easier to add more adapters after this one has been merge.
For now I have the following failures:

1. Symfony\Component\Cache\Tests\CacheTest::testSaveWithNoData
   When saving an object with no data, we should generate an error.

I didn't read anything about this in PSR-6, did I miss something?

For the other ones, I'll check tomorrow hopefully:

2. Symfony\Component\Cache\Tests\CacheTest::testDeferredSaveWithoutCommit
   A deferred item should automatically be committed on CachePool::__destruct().

3. Symfony\Component\Cache\Tests\CacheTest::testExpiration
   Failed asserting that true is false.

4. Symfony\Component\Cache\Tests\CacheTest::testIsHit
   Failed asserting that false is true.

5. Symfony\Component\Cache\Tests\CacheTest::testIsHitDeferred
   Failed asserting that false is true.

Using a standardized interface for cache in Symfony will be very helpful!

What do you think about using the PSR-6 interface instead of the Doctrine Cache one in#16838 and#16917?

1. When saving an object with no data, we should generate an error.

We have given some thoughts in to this. The PSR doc block onsave says "True if the item was successfully persisted. False if there was an error.". I claim it is an error becausenull is valid data.

Consider this:

$item =$pool->getItem('key');$pool->save($item);// Assume this is okey$pool->hasItem('key');// Should return true, yes? Or else, what is the point of saving?/** * Doc block for get(): * If isHit() returns false, this method MUST return null. Note that null * is a legitimate cached value, so the isHit() method SHOULD be used to * differentiate between "null value was found" and "no value was found." * * Doc block for isHit(): * Confirms if the cache item lookup resulted in a cache hit. */$pool->getItem('key')->isHit();// Must be false because "no value was found."// So ...$pool->hasItem('key') !==$pool->getItem('key')->isHit();// This makes no sense.

The pool should not have an item that never can be a hit. That would be a stale item and should be removed.
If you really want to store just the key you should just do$item->set(null);

@Nyholm I don't get your example, to me, assuming "key" is not already in the cache:

$pool->hasItem('key');// false$item =$pool->getItem('key');$item->get();// null$item->isHit();// false$pool->save($item);// true$item =$pool->getItem('key');$item->get();// null$item->isHit();// true -> a value has been found, we saved it before!

You have not saved any value. Since: "Note that null is a legitimate cached value, so the isHit() method SHOULD be used to differentiate between "null value was found" and "no value was found.""

The last two rows in you example:

$item->get();// null$item->isHit();// true -> a value has been found, we saved it before!

If$item->isHit() === true and$item->get() returnsnull you assume that someone has done a$item->set(null) which is not the case.

The point is,null should not be treated any special.

If $item->isHit() === true and $item->get() returns null you assume that someone has done a $item->set(null) which is not the case.

My example above clearly allows to differentiate between a null a "found" vs a null as "not found". Nothing in the spec says that$item->set(null) is the only way to get a "data" null. This still looks like a non standard requirement to me.

@nicolas-grekas Thank you for working on this - it is awesome! We should merge this.

+1 to merge it now. It will allow to start migrating metadata systems and other places in Symfony using Doctrine Cache internally to PSR-6.

Thank you@nicolas-grekas.

IMO namespaces handling should be a separate decorator and not built into the adapter directly. Seedoctrine/cache#96

I also want to express my objections against the recent fast merges of new components. This was merged in less than two days which does not give enough time to review and discuss a full new component at all. Also it's against our own rules which says

A pull request can be merged if: Enough time was given for peer reviews (at least 2 days for "regular" pull requests, and 4 days for pull requests with "a significant impact");
http://symfony.com/doc/current/contributing/code/core_team.html

I hope it's not going to be of the same quality as the LDAP component.

IMO namespaces handling should be a separate decorator and not built into the adapter directly. Seedoctrine/cache#96

True, it probably should be, but also a bit of a orange to apple comparison. In doctrine this is a namespace kept in cache and that needs to be checked on every cache lookup, that is not the case here(luckily, the extra cache lookup adds overhead, use case can be covered by tagging instead to avoid it).

@Tobion sorry if you feel uncomfortable, we merged quickly that's true... I invite you to open a PR to move the namespace concept in it's own decorator if you think it's better (taking@andrerom into account). The component will see more enhancements until 3.1 is released, but at least we don't have to discuss about its interfaces, which makes the situation totally different from the Ldap component hopefully.

My suggestion for namespace would be to let Adaptor generate key with a namespace. For e.g. most Redis tools groups keys if they have: separators. If Adaptor for e.g. doesn't support some characters or uses own rules with namespaces it would be better I think in this way. Like I said for Redis e.g. to replace\ with: it would allow better viewing for redis tools...

@GCDS namespace (or prefix really) are already managed by adapters (they must deal with it in their constructor). Which means a Redis adapter could do whatever you'd need there. To me, the simple prefixing logic is rightfully in the abstract, because adding a prefix is really something common. Doing namespaces àla doctrine/cache is for a decorator, or at least not for the abstract, I agree.

Ahh yeah, sounds correct to me :)