[REVIEW] PSR-6 Caching

[Paul Dragoonis]

Hello,

I am now putting PSR-6 forward for review. 

I’m exhausted but also really happy to be able be able to put this up for review as we’re at a point where we feel this PSR is complete + viable enough to pave the way for a solid level of caching consistency in the php world.

If we decide to move forward with an acceptance vote, we can do so as early as Tuesday, 27th October 2015.

FROM BYLAW:
The goal is also not necessarily to have every PHP-FIG member agree with the approach chosen by the proposal. The goal however is to have all PHP-FIG members agree on the completeness and objectivity of the meta document.

For the current version see:
 * https://github.com/php-fig/fig-standards/blob/master/proposed/cache.md
 * https://github.com/php-fig/fig-standards/blob/master/proposed/cache-meta.md

Many thanks,
Paul

[will.g...@couchbase.com]

Hi,

New to posting on PHP-FIG, apologies if I've got procedure wrong. I have a few niggles with the proposal, but on the whole it looks very good.

My first concern is with the inconsistency between storage, deletion and retrieval of multiple items. For retrieval and deletion there is a multiple items at once strategy, for storage there is a pipelining strategy. I feel that it would make more sense to have just one strategy, either staging CacheItems for retrieval/deletion (And performing the retrieval itself through commit or otherwise) or replacing deferred saving with a saveItems -esque approach where a whole list is persisted at once. Either the Pool should be responsible for maintaining a list of pending operations or the user - not a mix.

I'm personally a fan of pipelining with a combined use 'commit' as some caching platforms support mixed style operations arriving in a single batch.

My second concern is that there is no way to cancel deferred saving, once you have deferred an item you must either commit it or stop using the pool. It is also not clear what the behaviour should be if a an item is saved with save() whilst there are deferred saves, should you save all of them or just the one?

I'm also extremely concerned with returning false on failure. I often joke at work that returning false/null on failure is php's way of doing things, mostly however I understand it as a relic of compatibility in the standard library. It's 2015 we should be using exceptions for this. Therefore if I commit and one of the items fails rather than just getting false I *could* be receiving an exception which will give me useful information regarding what the hell happened. In addition we seem to have inconsistency again, behaviour on a commit failure is defined, behaviour on save failure is undefined.

There is also undefined behaviour for the CacheItemInterface, if I set expiresAt beyond what my caching system can handle should that fallback to 'as long as possible' or should it error (Returning false or exception?). And if it's possible to set indefinite caching for expiresAt, then why not expiresAfter. And if we can support an integer delta with expiresAfter then why not an integer absolute (epoch) with expiresAt? It's also unclear whether the translation between the two should be handled at item level or the system level. For instance memcached (At a protocol level) can support expiry deltas (Upto 30 days) and absolute expiry - should the library be translating it at the PHP level or letting memcached sort it out?

I'm also not a fan of the 'isHit' method, I feel a better solution to race conditions would be for 'get' to throw an exception on cache miss. This way we can allow an item to be regarded as 'expired' in a more consistent manner by allowing expiry after retrieval.

Best Regards,

Will

[Dracony]

commit() returning false definitely seems out of place. Thanks for pointing that out actually, since I somehow always missed it. Is there any reason for such behavior?

[will.g...@couchbase.com]

(Apologies to those who already got a bit of this message, fat fingered my keyboard.)

It's not just commit(), clear() has the same issue.

--

Just realised I was a little vague about why I don't like isHit() so I'm going to expand on it a bit and a bit more on expiry in general.

In it's current form this PSR allows for two broad models of the CacheItem. In one model the CacheItem is just a packaged result and the value for the key has actually already been retrieved (Or at least an attempt made), in the other the CacheItem is a lazy object and will attempt retrieval of the value at the first get/isHit. 

Now the issue being that this is an interface, the end-user should not care about this implementation difference which means the semantics of both model are slightly constrained. What this means is that the internal miss/hit state *must* be determined on the first get/isHit - otherwise there would be a significant difference in behaviour if the CacheItem is created but not accessed for a length of time (Say five minutes). In the former model this could succeed and in the latter fail (Or with concurrency concerns, the reverse). Therefore there is additional logic which must go into the first model, an isHit could look like this:

private $fake_hitness;
private $actual_hitness
public function isHit() {
    if($this->actual_hitness) {
        if($this->fake_hitness === null) {
            $this->fake_hitness = time() <= $this->expiry;
        }
        return $this->fake_hitness;
    }
    return false;
}

Suddenly the CacheItem has gone from being a fairly lightweight wrapper to a value to having an internal state. It would be more sensible if the CacheItem always checked the expiry and never gave up the value if the expiry was surpassed (In which case we must resolve the race condition, in my opinion an exception makes the most sense), either that or it should never check the expiry itself in which case we should disallow the lazy model and say that once a CacheItem exists its state of cache hit is determined and its value is determined. The latter has the added bonus of the CacheItem not needing the expiry.

[Larry Garfield]

Greetings!  Responses inline.  (Paul, including some questions for you.)

On 10/17/2015 01:02 PM, will.g...@couchbase.com wrote:

Hi,

New to posting on PHP-FIG, apologies if I've got procedure wrong. I have a few niggles with the proposal, but on the whole it looks very good.

Thanks.

My first concern is with the inconsistency between storage, deletion and retrieval of multiple items. For retrieval and deletion there is a multiple items at once strategy, for storage there is a pipelining strategy. I feel that it would make more sense to have just one strategy, either staging CacheItems for retrieval/deletion (And performing the retrieval itself through commit or otherwise) or replacing deferred saving with a saveItems -esque approach where a whole list is persisted at once. Either the Pool should be responsible for maintaining a list of pending operations or the user - not a mix.

I'm personally a fan of pipelining with a combined use 'commit' as some caching platforms support mixed style operations arriving in a single batch.

True, it's a bit different.  In *most* cases, we assume that take-action-immediately is the preferred behavior.  In the majority case, that's safer than deferring action to some undetermined time.

The saveDeferred()/commit() approach is in practice only really meaningful for cache warming.  In that case, you would do a whole lot of cache generation work, then save it all for later usage.  Usually you'd do this out-of-band (not in a user request), or similar.  In that case, you don't want the overhead of many separate IO calls.

However, support for multi-operations varies depending on the backend data store, as do things like max payload size.  The deferred/commit approach explicitly allows the Pool to decide when to actually save an item, but no later than commit().  That is, the intelligence about how large of a batch to save at once can live in the pool, closer to where the distinction would be made.  As a calling library, I don't know (deliberately) whether the underlying data store can handle 1 MB of data at once, 200 KB of data, if it supports multi-write operations at all, if it's capped at a certain number, etc.  The pool does (or can).

So in general, you SHOULD be using the atomic actions.  Multi-save is a special case, so we allow it a different handling.

My second concern is that there is no way to cancel deferred saving, once you have deferred an item you must either commit it or stop using the pool. It is also not clear what the behaviour should be if a an item is saved with save() whilst there are deferred saves, should you save all of them or just the one?

True, there isn't a way to cancel a deferred item.  We didn't see a use case for that, and it would complicate the interface.

The behavior of save() when there are deferred items is left up to the implementer.  Committing everything pending at once probably makes sense, and that's listed as one of the known possible options, but that's not a requirement.  Getting that prescriptive feels like it gets into dictating implementation more than we want.

I'm also extremely concerned with returning false on failure. I often joke at work that returning false/null on failure is php's way of doing things, mostly however I understand it as a relic of compatibility in the standard library. It's 2015 we should be using exceptions for this. Therefore if I commit and one of the items fails rather than just getting false I *could* be receiving an exception which will give me useful information regarding what the hell happened. In addition we seem to have inconsistency again, behaviour on a commit failure is defined, behaviour on save failure is undefined.

There are a few methods that return false on failure; however, their only other return value is true.  That is, they return "yay, it worked!" or "crap, didn't work".  I am *completely and vehemently in agreement* that returning "the value you wanted or false" is an anti-pattern that PHP needs to stop using.  See also:

http://www.garfieldtech.com/blog/empty-return-values

However, as I note there it's not so much returning false on error as returning "yep/nope" on "did you do what I told you to do?"  That's still type safe, so a reasonable use of a boolean.

(If there are any places where the spec is still returning "the value you wanted or false", let us know because I thought we exterminated all of those.)

There is also undefined behaviour for the CacheItemInterface, if I set expiresAt beyond what my caching system can handle should that fallback to 'as long as possible' or should it error (Returning false or exception?). And if it's possible to set indefinite caching for expiresAt, then why not expiresAfter. And if we can support an integer delta with expiresAfter then why not an integer absolute (epoch) with expiresAt? It's also unclear whether the translation between the two should be handled at item level or the system level. For instance memcached (At a protocol level) can support expiry deltas (Upto 30 days) and absolute expiry - should the library be translating it at the PHP level or letting memcached sort it out?

Hm, a couple of points here.  Let me try to break them out.

1) expiresAfter()/expiresAt() past the underlying system's capability. - Since an implementation is permitted to expire an item before the set expiration time, just not after, I would say in this case an implementation should cache for "as long as possible".  I believe that's already covered in the Expiration definition:

" Implementing Libraries MAY expire an item before its requested Expiration Time, but MUST treat an item as expired once its Expiration Time is reached."

That is, the requested expiration time is technically "cache for no longer than".  The same logic applies to caching "forever", if the underlying system can't handle "forever" (say, it's memory-resident only).  Do you think that needs to be clarified, perhaps a parenthetical in the part I quoted above?  I believe that would be a Review-safe change, as it's not changing behavior, just clarifying.  Or possibly a note in the metadoc.  Paul, your thoughts?

2) What happens if expiresAt() and expiresAfter() are both never called? - I would argue that the current description is still valid.  That is, if the calling library never specifies a an expiration, cache forever.  Perhaps that can be worded a bit better.  In fact, now that there's 2 methods, it may make sense to move that rule from the interface to the description of expiration further up.  And then yes, calling either method with null should set it back to forever.  I believe that is a Review-safe change, since it's not changing any behavior, just clarifying.  Paul, do you agree?  I can file a PR if so.

While reading the spec again while answering this part, I also noted that Item::set() still has mention of TTL in its docblock.  That seems vestigial since set() no longer has a $ttl parameter.  I believe it's safe to just remove that block, as it's redundant with the docblock of expiresAt() now (or whatever we refactor that to).  Again, that's not changing behavior so I believe it's Review-safe.

3) expiresAt(int) - In what timezone?  DateTime always carries a timezone, so the underlying system can do any timezone translation necessary.  An epoc timestamp doesn't.  That's why, in practice, epoc timestamp ints are unreliable and should pretty much never be used ever.  If the timezones do not match, the intended time of expiration and what actually gets recorded could be off by as much as a day, resulting in the value never being cached (time is in the past, in that timezone) or cached for too long.  All kinds of bad things happen when you don't know your timezone. I've been bitten by about a third of them at some point, and am crippled for life as a result. :-)

4) Translation between expiration time and duration - This is left up to the implementation to figure out.  As long as it happens it doesn't matter to the caller if it's PHP or Memcache that is doing "time = now + $duration" (or vice versa).  That will likely vary depending on the details of the backend.  Do whatever makes the most sense for your backend.

I'm also not a fan of the 'isHit' method, I feel a better solution to race conditions would be for 'get' to throw an exception on cache miss. This way we can allow an item to be regarded as 'expired' in a more consistent manner by allowing expiry after retrieval.

The problem there is that exceptions are expensive.  They are also for, well, exceptional cases.  Ie, errors, "something weird went wrong", etc.  A requested cache item not being present is quite normal, so an exception is inappropriate.

In later messages you note that it puts more logic into isHit().  I don't think that's really a problem.  As you note, this spec allows for CacheItem to be "smart" (and contain active logic) or "dumb" (and just be a holder for data given to it by the Pool).  That's by design, as different implementations may need different trade-offs (eg, a single-load could lazy load, but multi-load front-load).  Even a "dumb" item can still have logic in it.

For a non-lazy-loading implementation, see this (somewhat dated but still valid for this part of the spec) partial implementation:

https://github.com/Crell/Cache-Util/blob/master/BasicCacheItemTrait.php

That's really quite simple.  For a lazy-loading implementation, both isHit() and get() can sub-call to a private method that talks back to the pool to actually load the data when either one is called.  That seems fine to me, and not a burden on the implementer.

Thanks for your feedback!

--Larry Garfield

[will.g...@couchbase.com]

There are a few methods that return false on failure; however, their only other return value is true.  That is, they return "yay, it worked!" or "crap, didn't work".  I am *completely and vehemently in agreement* that returning "the value you wanted or false" is an anti-pattern that PHP needs to stop using.  See also:

http://www.garfieldtech.com/blog/empty-return-values

However, as I note there it's not so much returning false on error as returning "yep/nope" on "did you do what I told you to do?"  That's still type safe, so a reasonable use of a boolean.

(If there are any places where the spec is still returning "the value you wanted or false", let us know because I thought we exterminated all of those.)

My concern was not one of type safety, my concern was:

a) Returning false gives no indication of why it was not successful

b) It is inconsistent, if commit() and clear() return a boolean then why not save() or delete()? If a save() or deleteItems() does not complete then the only way of signalling that (And it absolutely should in the case of delete) would be to throw an exception, and if save() or deleteItems() throw exceptions then it seems weird that commit() and clear() wouldn't.

[Dracony]

It is inconsistent, if commit() and clear() return a boolean then why not save() or delete()? If a save() or deleteItems() does not complete then the only way of signalling that (And it absolutely should in the case of delete) would be to throw an exception, and if save() or deleteItems() throw exceptions then it seems weird that commit() and clear() wouldn't.

I agree, it shifts the burden to the user of actually checking whether it worked or not. To me, if I call ->commit() and it failed, that is an exceptional case, that should rase an exception and be treated like en error. There are numerous examples where this can shoot the user in the foot, e.g. the cache layer is not working properly and items are regenrated on every request drastically increasing the load. The user will only notice this after the server runs out of processing power and that causes exceptions elsewhere. Just because we can return typehinted boolean, doesn't mean e should. What would you guys say if Doctrine flush() method would silently return false when the dtabase is down, and report an error.

And there is another problem: what kind of error is 'false' anyway? No message, no reason given, just false, how do you debug that?

Memcached not running ? "false"

Wrong credentials? "false"

Wrong permissions on cache folder? "false"

On Wednesday, October 14, 2015 at 12:10:19 AM UTC+2, Paul Dragoonis wrote:

[Larry Garfield]

On 10/18/2015 10:51 AM, Larry Garfield wrote:

There is also undefined behaviour for the CacheItemInterface, if I set expiresAt beyond what my caching system can handle should that fallback to 'as long as possible' or should it error (Returning false or exception?). And if it's possible to set indefinite caching for expiresAt, then why not expiresAfter. And if we can support an integer delta with expiresAfter then why not an integer absolute (epoch) with expiresAt? It's also unclear whether the translation between the two should be handled at item level or the system level. For instance memcached (At a protocol level) can support expiry deltas (Upto 30 days) and absolute expiry - should the library be translating it at the PHP level or letting memcached sort it out?

Hm, a couple of points here.  Let me try to break them out.

1) expiresAfter()/expiresAt() past the underlying system's capability. - Since an implementation is permitted to expire an item before the set expiration time, just not after, I would say in this case an implementation should cache for "as long as possible".  I believe that's already covered in the Expiration definition:

" Implementing Libraries MAY expire an item before its requested Expiration Time, but MUST treat an item as expired once its Expiration Time is reached."

That is, the requested expiration time is technically "cache for no longer than".  The same logic applies to caching "forever", if the underlying system can't handle "forever" (say, it's memory-resident only).  Do you think that needs to be clarified, perhaps a parenthetical in the part I quoted above?  I believe that would be a Review-safe change, as it's not changing behavior, just clarifying.  Or possibly a note in the metadoc.  Paul, your thoughts?

2) What happens if expiresAt() and expiresAfter() are both never called? - I would argue that the current description is still valid.  That is, if the calling library never specifies a an expiration, cache forever.  Perhaps that can be worded a bit better.  In fact, now that there's 2 methods, it may make sense to move that rule from the interface to the description of expiration further up.  And then yes, calling either method with null should set it back to forever.  I believe that is a Review-safe change, since it's not changing any behavior, just clarifying.  Paul, do you agree?  I can file a PR if so.

While reading the spec again while answering this part, I also noted that Item::set() still has mention of TTL in its docblock.  That seems vestigial since set() no longer has a $ttl parameter.  I believe it's safe to just remove that block, as it's redundant with the docblock of expiresAt() now (or whatever we refactor that to).  Again, that's not changing behavior so I believe it's Review-safe.

Update: I've added a PR that cleans up the wording as described above:

https://github.com/php-fig/fig-standards/pull/650

Ping Paul Dragoonis...

--Larry Garfield

[Paul Dragoonis]

Inline.

Since the meta document states: "after which the item MUST be considered expired” then I feel this is enough. I’m happy for further clarification if someone provides an elaborated copy.

 

2) What happens if expiresAt() and expiresAfter() are both never called? - I would argue that the current description is still valid.  That is, if the calling library never specifies a an expiration, cache forever.  Perhaps that can be worded a bit better.  In fact, now that there's 2 methods, it may make sense to move that rule from the interface to the description of expiration further up.  And then yes, calling either method with null should set it back to forever.  I believe that is a Review-safe change, since it's not changing any behavior, just clarifying.  Paul, do you agree?  I can file a PR if so.

Yes, it is important to specify what should happen when no TTL is specified and that the TTL is implicitly forever. PR definitely needed.

 

While reading the spec again while answering this part, I also noted that Item::set() still has mention of TTL in its docblock.  That seems vestigial since set() no longer has a $ttl parameter.  I believe it's safe to just remove that block, as it's redundant with the docblock of expiresAt() now (or whatever we refactor that to).  Again, that's not changing behavior so I believe it's Review-safe.

Yes this is a review-safe change.

 

3) expiresAt(int) - In what timezone?  DateTime always carries a timezone, so the underlying system can do any timezone translation necessary.  An epoc timestamp doesn't.  That's why, in practice, epoc timestamp ints are unreliable and should pretty much never be used ever.  If the timezones do not match, the intended time of expiration and what actually gets recorded could be off by as much as a day, resulting in the value never being cached (time is in the past, in that timezone) or cached for too long.  All kinds of bad things happen when you don't know your timezone. I've been bitten by about a third of them at some point, and am crippled for life as a result. :-)

Yes, timezones are important here, and keep us safe.

For expiresAfter(X) is safe for an integer because it means expireAfter(current date and timezone + X seconds).

 

4) Translation between expiration time and duration - This is left up to the implementation to figure out.  As long as it happens it doesn't matter to the caller if it's PHP or Memcache that is doing "time = now + $duration" (or vice versa).  That will likely vary depending on the details of the backend.  Do whatever makes the most sense for your backend.

I'm also not a fan of the 'isHit' method, I feel a better solution to race conditions would be for 'get' to throw an exception on cache miss. This way we can allow an item to be regarded as 'expired' in a more consistent manner by allowing expiry after retrieval.

The problem there is that exceptions are expensive.  They are also for, well, exceptional cases.  Ie, errors, "something weird went wrong", etc.  A requested cache item not being present is quite normal, so an exception is inappropriate.

In later messages you note that it puts more logic into isHit().  I don't think that's really a problem.  As you note, this spec allows for CacheItem to be "smart" (and contain active logic) or "dumb" (and just be a holder for data given to it by the Pool).  That's by design, as different implementations may need different trade-offs (eg, a single-load could lazy load, but multi-load front-load).  Even a "dumb" item can still have logic in it.

For a non-lazy-loading implementation, see this (somewhat dated but still valid for this part of the spec) partial implementation:

https://github.com/Crell/Cache-Util/blob/master/BasicCacheItemTrait.php

That's really quite simple.  For a lazy-loading implementation, both isHit() and get() can sub-call to a private method that talks back to the pool to actually load the data when either one is called.  That seems fine to me, and not a burden on the implementer.

Thanks for your feedback!

--Larry Garfield

--
You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/5623BFFF.5010602%40garfieldtech.com.

For more options, visit https://groups.google.com/d/optout.

[Jonathan Eskew]

Epoch timestamps are defined as being in UTC (see http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_15). Timezones are not a concern when dealing with UNIX time, only when converting it from epoch to a timezone-aware representation (like an instance of \DateTime).

[will.g...@couchbase.com]

Responding to relevant Zendcon notes here to keep it consolidated:

All present felt that the expiration cleanup PR that was at the time outstanding was Review safe, and it has since been merged. Yay.
We also discussed the other concern, that some methods return true/false for "Worked/Didn't Work" rather than throwing an exception.  Most of the discussion centered on the save() routines.
The general consensus was that while changing that would not be a Review-safe change, we shouldn't change it anyway for a number of reasons.
1) Cache backends are frequently unreliable by nature, so a save not working is not necessarily exceptional.
2) Given that, wrapping so many operations in a try-catch would be a DX fail.
3) Even with that aside, no one present really felt strongly enough about it to want to restart Review over it.
Thus, the recommendation is to not change those returns and continue as-is, with PSR-6 being ready for a vote really-soon-now-for-reals!

This is extremely frustrating for the reasons I put out before. I (almost) don't care if they all throw exceptions or if they all use true/false. What I DO care about is that some of the damn methods use true/false and some you would be forced to throw an exception. I'm also VERY frustrated, Larry, that despite the fact that I put these concerns back out there some time ago you have not responded to them in any and still intend on pressing ahead with a vote.

Case study time!:

1. I'm inserting one cache item into my cache (And it fails)

$myCachePool->save($myCacheItem); // Must throw exception as the caching layer has no defined behaviour on cache save failure

2. I'm bootstrapping and inserting multiple cache items into my cache (And it fails)

$myCachePool->saveDeferred($myFirstCacheItem);
$myCachePool->saveDeferred($myFirstCacheItem);

if(!$myCachePool->commit()) {
    // Returned false, not very helpful but at least it's defined
}

We now have the worst of every world, an inconsistent interface, a defined failure behaviour that tells you exactly nothing about why the error occurred and a method throwing exceptions (Despite what you have now described as non-exceptional behaviour).

[Anthony Ferrara]

All,

I just took a look at the revised PRS-6 proposal and have a few comments:

I would *strongly* suggest not throwing exceptions. A cache system
being down is not an exceptional event (and if it is, it's not being
used as a cache system). This is one of those cases where a failure
should be upconverted at the application level, since only the
application knows if it is exceptional or not (to the cache subsystem
it's not). In the general case it is not.

Secondly, you define Psr\Cache\InvalidArgumentException. This directly
conflicts with PHP7, which would expect TypeError in most of those
cases.

Third, you define public function expiresAt($expiration); which
accepts a DateTime object, but you don't actually use the type
declaration for DateTime.

I still feel that this entire proposal is way over complicated for the
90% use-case, but obviously enough people disagree with me there to
keep it pushing forward. I just want to state my unofficial objection
once more.

Thanks

Anthony

> --
> You received this message because you are subscribed to the Google Groups
> "PHP Framework Interoperability Group" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to php-fig+u...@googlegroups.com.
> To post to this group, send email to php...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/php-fig/9178333d-754e-4a5f-9dae-0fdc31b63f88%40googlegroups.com.

[Robert Hafner]

I’ve stayed out of the exception argument since people seem to have it covered, but I figured when Anthony and I agree on something (as we apparently do with regards to *not* throwing exceptions) I figure I should at least step in and point it out.

Caching systems fail, but that shouldn’t be considered a fail for the application. If memcached is unavailable the apps should run slower, not fail to run at all.

As a final note, PSR-3 exists. Presumably most caching libraries are going to want to support it. If a caching system fails it should be logged- with appropriate urgency- so it gets pushed to the system admins in charge of things. This is much better than alerting the users to a problem they don’t ever have to be aware of.

Rob

> To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/CAAyV7nGZJdAgHkHAHgJCE9%3DELoWCP4%2BAF8F2025_dFOYTMfgxQ%40mail.gmail.com.

[Dracony]

If the developer wants to avoid stopping execution when memcached is down he can always wrap the call within a try/catch block. With the 'silent fail' approach a cautios developer will have to still write extra code that makes sure cache worked, but he won't get the reason for the fail, just that fail happened. And to me saying 'lets use logger instead of exceptions' is a really thin argument. If the developer want to continue when the error happens it is his choice, but I don't believe it should be the default setting.

I looked into some "industry standard" Java caching libraries and all of them throw exceptions in these cases. Why in PHP we should go with the 'silent fail approach again, if the PHP api is already ridiculed for doing this exact thing in a bunch of places.

On Wednesday, October 14, 2015 at 12:10:19 AM UTC+2, Paul Dragoonis wrote:

[Woody Gilk]

Couldn't this whole argument be resolved by having get() always throw an exception, and adding getSilent($id) that would return null if an exception happened? Personally I don't see any value in having an isHit() method because it implies an object with transient state and will not work properly in an async world which is quickly becoming reality in PHP.

Regards,

--
Woody Gilk
http://about.me/shadowhand

--

You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/d1f39afe-57ff-49a8-8e0a-bf6d81fc9b2b%40googlegroups.com.

[Larry Garfield]

As has been noted, a cache value not being present is not at all
exceptional. It's rather normal. And exceptions are also way more
expensive in PHP than returning a boolean, since they need to generate
and store a stack trace.

isHit() and get() don't imply a transient state object. They imply an
object that you can inspect in 2 different ways. Calling one vs the
other does not affect the state of the object as far as the calling
library is concerned. (Either one may trigger a lazy load from the
backend if the implementing library wrote it that way, but that's
incidental to the PSR.)

> <mailto:php-fig+u...@googlegroups.com>.

> To post to this group, send email to php...@googlegroups.com

> <mailto:php...@googlegroups.com>.

> To view this discussion on the web visit
> https://groups.google.com/d/msgid/php-fig/d1f39afe-57ff-49a8-8e0a-bf6d81fc9b2b%40googlegroups.com

> <https://groups.google.com/d/msgid/php-fig/d1f39afe-57ff-49a8-8e0a-bf6d81fc9b2b%40googlegroups.com?utm_medium=email&utm_source=footer>.

> For more options, visit https://groups.google.com/d/optout.
>
>

> --
> You received this message because you are subscribed to the Google
> Groups "PHP Framework Interoperability Group" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to php-fig+u...@googlegroups.com

> <mailto:php-fig+u...@googlegroups.com>.

> To post to this group, send email to php...@googlegroups.com

> <mailto:php...@googlegroups.com>.

> To view this discussion on the web visit

> https://groups.google.com/d/msgid/php-fig/CAGOJM6K_Ddjr5i6LmiKQ%2BYMdw61EJwvF0gjbbPK908M%3DHkyHpg%40mail.gmail.com
> <https://groups.google.com/d/msgid/php-fig/CAGOJM6K_Ddjr5i6LmiKQ%2BYMdw61EJwvF0gjbbPK908M%3DHkyHpg%40mail.gmail.com?utm_medium=email&utm_source=footer>.

> For more options, visit https://groups.google.com/d/optout.

--
--Larry Garfield

[Roman Tsjupa]

There is a huge difference between a cache value not being present and a whole cache subsystem being down imho.

You received this message because you are subscribed to a topic in the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/php-fig/8DOioT3eSYg/unsubscribe.
To unsubscribe from this group and all its topics, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/562FC3DE.9070203%40garfieldtech.com.

[Woody Gilk]

On Tue, Oct 27, 2015 at 1:35 PM, Larry Garfield <la...@garfieldtech.com> wrote:

isHit() and get() don't imply a transient state object. 

I disagree. In an async world, the following is entirely possible:

$value = $cache->get('foo');

$promise->then(function () use ($cache) {

    if ($cache->isHit()) {

        // this is not longer guaranteed to be the hit for "foo"!

    }

});

Anything that sets object state based on a separate method invocation is absolutely transient!

[Chris Tankersley]

On Tue, Oct 27, 2015 at 2:40 PM, Roman Tsjupa <draco...@gmail.com> wrote:

There is a huge difference between a cache value not being present and a whole cache subsystem being down imho.

There is a big difference, but should my site completely crumble and refuse to work because the caching layer isn't working properly? Caching is a means to store off computed data, but the cache system not responding, not actually saving, or having a problem getting the cache shouldn't grind my site to a halt. It should just return a known value (false, because it couldn't get the requested info), and move on. 

Will my site be less performant? Yes, but it will still be up. We can fix the cache while still letting users use the site. If you rely on a cache being available 100% of the time, it's not a cache. It's a datastore.

 

To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.

To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/CANamvv3oCW_ztvk3fhpUky%3DztYp_CdR%3Dmomua1SLhwsuVr3rHg%40mail.gmail.com.

[Larry Garfield]

From the spec (docblock of isHit()):

Note: This method MUST NOT have a race condition between calling isHit()
and calling get().


So the above code would be fine, as the hit status of the object is not
allowed to change between isHit() and get(). It could change from
calling save(), but that is what you'd expect to happen.

That said, the sample you have is backward. Why you'd call isHit() long
after calling get() I have no idea. That seems highly contrived. :-)

PHP is not a fully multi-threaded environment, so any async interrupts
only happen at known points. It's not going to change from one line to
another. If you're spreading your isHit() and get() calls out that far
from each other, I'd consider that unsupported behavior.

--
--Larry Garfield

[will.g...@couchbase.com]

If you don't want it to grind to a halt then just catch the exception. I don't see what's so difficult about catching an exception. 

Regardless most of the methods don't return false anyway and in the current situation you'd either have that whole 'grind to a halt' or completely silent errors.

[Woody Gilk]

On Tue, Oct 27, 2015 at 2:01 PM, Larry Garfield <la...@garfieldtech.com> wrote:

Why you'd call isHit() long after calling get() I have no idea.

It really doesn't matter what you _think_ is a good idea, the interface should protect against this kind of transient state. Anything else is just sloppy. There is no valid reason for isHit() to exist, imho. If someone wants to verify that something definitely exists, their code should look like:

if ($cache->exists('foo')) {

    $value = $cache->get('foo');

}

I do not support throwing exceptions on cache hit failures. The interface needs to be sane by default, and right now it is only _mostly_ sane.

[Woody Gilk]

Actually I'm completely misreading the interface... isHit exists on the CacheItem not CachePool. My apologies for going down a dead end.

--
Woody Gilk
http://about.me/shadowhand

[Robert Hafner]

Your claim about the errors being completely silent is false. As I’ve pointed out already in this thread, we have a logging standard. Caching systems can use this to log errors, which admins can then act on. There is no silent failure.

What you are failing to realize is that the absence of a caching backend is not a failure or an exceptional cache. Systems that use caches operate under the assumption that the data will be missing, and for those systems there isn’t much of a difference between a miss because the data isn’t in the caching backend or the caching backend simply isn’t there.

Every system that is using a cache is capable of running without the data in it. Exceptions are designed to deal with situations that change the normal flow of a program- situations where the program can not continue along it’s normal paths- and that is irrelevant in a situation where the flow of the program does not have to change in anywhere due to the “error”.

So then it comes down to a question- why else should we throw an exception? This caching standard is meant to be injected in all sorts of calling libraries- should each of them be required to understand and deal with issues such as memcached not being available? Should they all fail when that happens? If, in the majority of caches, they shouldn’t fail- then it makes sense not to force them into writing a bunch of boilerplate code. This still doesn’t prevent them from throwing their own exceptions if they decide this is a failure condition (if caching library returns false, throw exception with message that caching system is unavailable), so there is nothing being prevented from happening by this decision.

Rob

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/f28061fa-dc71-4cbd-9db4-a7c10f084197%40googlegroups.com.

[Roman Tsjupa]

You cant make the argument that the cache library "obviously" has to log something. If that is the case the PSR should explicitly say that it at least SHOULD do that. Right now saying that the cache library uses PSR-3 to log stuff is "just saying". What if my app doesn't have a logger ? Is PSR-6 not usable by me then. You simply cant make assumptions like these.

Yo decide such things you have to look at real world libraries. All of the popular ones I took a look at (including non PHP ones) treat the cache service being down as an exception, and have been doing for years. In fact I cant think of a single library that does silent fails. So why come up with these arbitrary rules, when real life cases exist?

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/291E73D3-233A-4AFD-81A5-526C8B79FB91%40tedivm.com.

[will.g...@couchbase.com]

I see nothing in this Cache interface which states it has to implement the logging interface - although that's not a bad idea.

But yes, calling libraries should be forced to decide how to respond to a failure in the caching layer, if you can deal with the cache layer not being there then fine, catch the exception and ignore it but the important thing is the interface in that situation forced you to make that decision. I work for a company which supplies a high performance KV store frequently used for caching - I know from interacting with customers that when their cache goes down that it is a VERY BIG DEAL as the performance hit is significant. If your caching layer is unreliable then it's pretty pointless cache. The idea that your cache going walkabouts is unexceptional is pretty laughable.

[Evert Pot]

On 2015-10-27 6:36 PM, Roman Tsjupa wrote:
> You cant make the argument that the cache library "obviously" has to log
> something. If that is the case the PSR should explicitly say that it at
> least SHOULD do that. Right now saying that the cache library uses PSR-3
> to log stuff is "just saying". What if my app doesn't have a logger ? Is
> PSR-6 not usable by me then. You simply cant make assumptions like these.
>
> Yo decide such things you have to look at real world libraries. All of
> the popular ones I took a look at (including non PHP ones) treat the
> cache service being down as an exception, and have been doing for years.
> In fact I cant think of a single library that does silent fails. So why
> come up with these arbitrary rules, when real life cases exist?

Actually, Memcache, for the most part, silently fails if a single
instance goes down.

Internally the memcache extension will mark the instance as failed
(presumably in shared memory), and by default only try to hit it every
15 seconds to see if it's back up.

Failures can be acted upon using a callback.

http://php.net/manual/en/memcache.addserver.php

In a large memcache cluster, the expected situation is that servers go
down, and that it shouldn't matter for the application.

This is a common use-case for typical large-scale memcache installations.

I would add to that though that I can also see different scenarios where
you *do* need your key-value store to be reliable. I just don't think
this is an either/or type of thing.

Some key-value stores *are* meant to be reliable data-stores, and
sometimes it's important to get feedback of failure.

Now you can argue that this PSR is not meant to address those specific
use-cases. I don't really have an opinion about that, but that's what
this discussion should be about.

Evert

[Dracony]

Actually the Memcached library does not throw any exceptions ever at all, which does not strike me as a particularly good design. So its prrobably not the best of examples

[Evert Pot]

On 2015-10-27 6:36 PM, Roman Tsjupa wrote:

> You cant make the argument that the cache library "obviously" has to log
> something. If that is the case the PSR should explicitly say that it at
> least SHOULD do that. Right now saying that the cache library uses PSR-3
> to log stuff is "just saying". What if my app doesn't have a logger ? Is
> PSR-6 not usable by me then. You simply cant make assumptions like these.
>
> Yo decide such things you have to look at real world libraries. All of
> the popular ones I took a look at (including non PHP ones) treat the
> cache service being down as an exception, and have been doing for years.
> In fact I cant think of a single library that does silent fails. So why
> come up with these arbitrary rules, when real life cases exist?

[Robert Hafner]

There is also nothing in the caching proposal that requires people to follow the autoloading or coding standards PSRs, but we still encourage that to occur. In any case where logging is valuable a library should use the logging standard. If someone made a caching library that doesn’t follow PSR-3 then it is significantly less useful and less likely to be used by applications that want logging. 

With regards to Roman's idea that no caching library works like this- that’s just not true. Evert pointed out one counter example, and there’s at least one caching library in FIG that operates this way as well. Anyone who claims that there are “none” has proven that they need to do more research. Even without that the implication that the people who have spent almost four years on this project haven’t looked at other caching libraries along the way is ridiculous.

Will is right, the calling libraries should be able to respond to failure. I just don’t think the standard prevents that, as those that want to can by paying attention to return values. People trying to diagnose issues can use logs, which are likely to be a lot clearer than random stack traces or whatever they’re generating from exceptions.

Can someone provide a counter example to my assertion that this standard doesn’t prevent people from reacting to failure if they chose to?

Rob

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/14aa5826-5441-47d2-aca1-08a8276ac349%40googlegroups.com.

[will.g...@couchbase.com]

To avoid going round in circles, my main concern is and always has been that we are inconsistent in this matter. Some of the methods return false on failure but most of them do nothing. They just silently fail (other than *maybe* log). If I do a saveItem() I have no way to know if that particular save failed or succeeded. If I were implementing this interface I would probably end up throwing an exception in that situation just to communicate what failed and when - irrespective of why (Although Exception's do have that upside). I'm not sure why more people aren't more concerned about this inconsistency since it would enforce the worst of both worlds.

[Roman Tsjupa]

@Robert, yes, to properly react and debug a failure you need some information about it. Exceptions provide messages that can actually tell you what went wrong. Why returning false only tells you "something" went wrong. There is then no way to know that something without digging into the library. A message like "cannot reach host foo" is way more helpful than "false", especially in the case where you have multiple cache servers running and getting proper error message can lead to a quicker fix

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/395b8906-2bd8-4609-b0f4-88202c8739e3%40googlegroups.com.

[Paul Dragoonis]

Hi All,

Thanks for your feedback. It's good to see people are still passionate this proposal.

To clarify on implementations that this proposal is targetted at:

Memcached: returns false on a failed set(), and gives you getResultCode() to pull out the error.

Redis: return false on a failed set().

This proposal is meant to be inclusive and in-line with popular caching implementations already existing our there.

If you want to throw exceptions in your implementing system, you can totally do this. If you want to add logging in your implementing system you can totally do this. These are all actually implementation details and correctly pointed out above, you can choose what best suits your system based on a bool(false) response from a failed set().

For those wanting exceptions, where your cache is a critical part of your system:

if(your save fails) { throw MyDomainException; }

For those wanting logging, where your cache is a enhanced part of your system:

if(your save fails) { log the error, and carry on }

I hope that clarifies why the interface is flexible enough for you to make your own domain decisions around this.

Many thanks,

Paul

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/CANamvv3RpYQEUUuyHbDbo0V5UWzXS05x%3DV12DwLR2UdmS8rptw%40mail.gmail.com.

[Robert Hafner]

So log the message “caching error: cannot reach host foo” with a critical priority so you get an alert about the issue. 

You keep ignoring what I’m saying, and that isn’t helping at all.

Rob

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/CANamvv3RpYQEUUuyHbDbo0V5UWzXS05x%3DV12DwLR2UdmS8rptw%40mail.gmail.com.

[Roman Tsjupa]

There should at least be an getLastError method or similar. Like Paul mentioned memcached has. Even json_decode() provides a way to get an error message. If such method is present I agree the user can construct the exception himself.

@Robert, since this PSR does not mention such logging you cannot just implicitly "expect it to be done". Either it is in the standard or not.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/19C20B18-2843-46C2-9E4B-661D6A164C0D%40tedivm.com.

[will.g...@couchbase.com]

But Paul, my point is YOU CANT EVEN DO THIS. save() does not return false on failure! If the save fails there's no way to tell:

    /**
     * Persists a cache item immediately.
     *
     * @param CacheItemInterface $item
     *   The cache item to save.
     *
     * @return static
     *   The invoked object.
     */
    public function save(CacheItemInterface $item);

Doesn't return false on failure, either the implementation throws an exception or it does nothing at all. But the whole point of an interface is to allow interoperability - if some implementations throw exceptions and others do nothing then you've achieved didly squat in allowing this stuff to be portable. It's also incredibly weird that commit() would return false on failure and be a black sheep on the reporting front.

[Robert Hafner]

I thought the returning of “self” was a bit silly for a lot of things. If you want to make a list of all the functions that should be returning failure conditions and propose something off of that I think you’ll get better results than trying to push everything towards throwing exceptions.

Rob

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/60e7b3a5-ab70-490d-8acd-8e1f178b1d7f%40googlegroups.com.

[Dracony]

Can we actually have a quick vote on the whole exceptions vs silent fail approach? It would set the direction and then we would all stick with it. Without the vote it's just going to keep coming up.

What do you think? We could start a vote thread now, and give it 3 days of lifetime. I am sure all the members interested in it going either way will manage to vote in that period?

[Paul Dragoonis]

On Wed, Oct 28, 2015 at 4:48 PM, Dracony <draco...@gmail.com> wrote:

Can we actually have a quick vote on the whole exceptions vs silent fail approach? It would set the direction and then we would all stick with it. Without the vote it's just going to keep coming up.

What do you think? We could start a vote thread now, and give it 3 days of lifetime. I am sure all the members interested in it going either way will manage to vote in that period?

The PSR-6 standard telling libraries to throw exceptions isn't going to happen, but we'll put more focus on ensuring that it's possible for implementors to throw them if they want to. 

 

The first step would be to make $pool->save() return a boolean instead of $self. As per what Will Gardner pointed out.

Any other methods you guys feel should be returning booleans for success/fail instead of $self? Sorry if you've pointed out already I just need a condensed list of things to look at.

Thanks!

On Wednesday, October 14, 2015 at 12:10:19 AM UTC+2, Paul Dragoonis wrote:

Hello,

I am now putting PSR-6 forward for review. 

I’m exhausted but also really happy to be able be able to put this up for review as we’re at a point where we feel this PSR is complete + viable enough to pave the way for a solid level of caching consistency in the php world.

If we decide to move forward with an acceptance vote, we can do so as early as Tuesday, 27th October 2015.

FROM BYLAW:
The goal is also not necessarily to have every PHP-FIG member agree with the approach chosen by the proposal. The goal however is to have all PHP-FIG members agree on the completeness and objectivity of the meta document.

For the current version see:
 * https://github.com/php-fig/fig-standards/blob/master/proposed/cache.md
 * https://github.com/php-fig/fig-standards/blob/master/proposed/cache-meta.md

Many thanks,
Paul

--

You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/f373689b-6c93-4b84-bd18-66cc78e7ad07%40googlegroups.com.

[Paul Dragoonis]

On Wed, Oct 28, 2015 at 5:14 PM, Paul Dragoonis <drag...@gmail.com> wrote:

On Wed, Oct 28, 2015 at 4:48 PM, Dracony <draco...@gmail.com> wrote:

Can we actually have a quick vote on the whole exceptions vs silent fail approach? It would set the direction and then we would all stick with it. Without the vote it's just going to keep coming up.

What do you think? We could start a vote thread now, and give it 3 days of lifetime. I am sure all the members interested in it going either way will manage to vote in that period?

The PSR-6 standard telling libraries to throw exceptions isn't going to happen, but we'll put more focus on ensuring that it's possible for implementors to throw them if they want to. 

 

The first step would be to make $pool->save() return a boolean instead of $self. As per what Will Gardner pointed out.

Any other methods you guys feel should be returning booleans for success/fail instead of $self? Sorry if you've pointed out already I just need a condensed list of things to look at.

Thanks!

We don't have a $pool->deleteItem() which would be easy and simple to have, with a boolean result. Easy win there. Why was it removed? It once was $pool->delete($key) before we introduced "Items".

$pool->deleteItems($keys) doesn't have a bool result, it's also $self. This needs to be improved.

It gets complicated on what you return when you're deleting multiple items. 

Redis returns you the number of keys deleted (you could make this a bool in your library if count($args) == $ret)

Memcached returns boolean

I would be inclined here to return a boolean for deleteItems() too. This is in line with popular libraries and I'm happy to keep consistency here.

Anyone object to me changing $self to $boolean for $pool->set(), $pool->deleteItems($keys), and adding $pool->delete($key) which will return a boolean?

[will.g...@couchbase.com]

At minimum

save()

saveDeferred() (Since there's no requirement to *actually* defer the saving)

deleteItems() (This is especially important because there's real danger if a delete fails)

We're in a slightly weird territory with the getItem, getItems and hasItem methods because they have legitimate return types. If hasItem fails because of a caching issue there would be no way to indicate that since it already returns false on cache miss. We could say that a failure in general should be false but you'd not be able to detect if there was a systematic issue or just a missing item.

[Paul Dragoonis]

On Wed, Oct 28, 2015 at 5:29 PM, <will.g...@couchbase.com> wrote:

At minimum

save()

saveDeferred() (Since there's no requirement to *actually* defer the saving)

How can we boolean up safeDeferred() if it doesn't do anything? it's commit() that will fail.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/e116fd56-38a7-4526-b112-db66752ebe1d%40googlegroups.com.

[will.g...@couchbase.com]

The way saveDeferred() behaves depends on the implementation, it can save immediately, save them in chunks, or wait until commit(). commit() is only a guarantee that they've actually been saved, it doesn't actually require they were saved at commit (At least based on what Larry said near the start of this discussion) - this is why we have no way to cancel a commit and why the two methods couldn't be replaced with saveItems(). In theory you don't have to call commit() and the implementation should automatically make sure all deferred saves are committed in its destructor (In theory after the request has been flushed).

[will.g...@couchbase.com]

We don't have a $pool->deleteItem() which would be easy and simple to have, with a boolean result. Easy win there. Why was it removed? It once was $pool->delete($key) before we introduced "Items".

We don't need a deleteItem() as you can just specify a single element array to deleteItems(). 

[Robert Hafner]

I'm opposed to this. People have had almost four years to express their opinions on this, and we've already had a lot of conversations around this point. I don't think now is the time to change the strategy we've selected.

That being said, making sure that strategy actually works is a reason to make modifications. For this reason I am not opposed to a refactoring/cleanup of the return values.

Rob

--

[Paul Dragoonis]

On Wed, Oct 28, 2015 at 5:43 PM, Robert Hafner <ted...@tedivm.com> wrote:

I'm opposed to this. People have had almost four years to express their opinions on this, and we've already had a lot of conversations around this point. I don't think now is the time to change the strategy we've selected.

Because you top posted it's difficult to see what "this" implies as a few topics were still being discussed.

 

That being said, making sure that strategy actually works is a reason to make modifications. For this reason I am not opposed to a refactoring/cleanup of the return values.

I have made a PR for Larry to merge if he's happy with it. https://github.com/php-fig/fig-standards/pull/659

 

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/12E0388D-AD88-4854-AACF-92BFA4822045%40tedivm.com.

[Dracony]

The PSR-6 standard telling libraries to throw exceptions isn't going to happen, but we'll put more focus on ensuring that it's possible for implementors to throw them if they want to. 

Can we really do that? If I understand correctly it would mean a library can decide for itself whether it want to return false or throw an exception. Wouldnt that kind of be agains the whole interpolability? I would think that telling it to return false means that the library MUST suppress all exceptions and never throw anything, isnt it right? 

[will.g...@couchbase.com]

I think Paul is suggesting that the library *using* the cache implementation can choose to throw an exception.

[Robert Hafner]

Just to clarify, he’s saying implementers of calling libraries would decide to throw exceptions. He’s not saying that difference implementations of the caching standard may do one thing or another. All of the PSR-6 behavior will remain consistent, but libraries that decide to use a PSR-6 library can chose how they handle errors on their own. 

Rob

-- 
You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/c4609330-f12c-41e2-9fe5-ea887f89ceb2%40googlegroups.com.

[will.g...@couchbase.com]

Which (I forgot to add before) is fairly ridiculous since the calling library doesn't have any information about why it failed. If the Cache implementation threw an exception then at least that way the exceptions are actually useful and have meaningful backtraces.

[Roman Tsjupa]

Yes, that is exactly what I said yesterday, and it appears we have gone full circle. Now I should say: "for that the implementing library must provide some getLastError() method like json_decode does. This way you would get info on failure"

You received this message because you are subscribed to a topic in the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/php-fig/8DOioT3eSYg/unsubscribe.
To unsubscribe from this group and all its topics, send an email to php-fig+u...@googlegroups.com.

To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/4fa2ade2-5be3-49c0-9e5c-81c5145af5eb%40googlegroups.com.

[Robert Hafner]

Sorry- for clarity, I’m opposed to the concept of starting yet another poll to reopen the exception conversation, and think we should just clean up the return values and move on.

I’m heading into work now and will review emails/pull requests this evening.

Rob

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/CAKxcST-GzX06PjZnnDY9iVCFaP6mJoLLyer42WStumFXT_jNRw%40mail.gmail.com.

[Larry Garfield]

Let’s take a step back and try to get a bird’s eye view. I went through
the current spec and cataloged all the methods and their current return
values. In general, any method could return:

- A meaningful value that makes sense in the domain model and API, of
some type.
- self, i.e., the called object. Usually this is to make it chainable.
- success/failure (i.e., boolean that specifically represents “it works”
or “nope, didn’t work”)
- null (which none of these methods do)

It of course could throw an exception.

Now, here’s where PSR-6 stands today:

CacheItem:
- getKey: Meaningful value (string)
- get: Meaningful value (mixed)
- set: self (chainable)
- isHit: Meaningful value (boolean)
- expiresAt: self (chainable)
- expiresAfter: self (chainable)

CacheItemPool:
- getItem: Meaningful value (CacheItem), throws InvalidArgumentException
- getItems: Meaningful value (array|\Traversable)
- hasItem: Meaningful value (boolean)
- clear: Success/failure (boolean)
- deleteItems: self (chainable)
- save: self (chainable)
- saveDeferred: self (chainable)
- commit: Success/failure (boolean)


# Regarding exceptions:

The first thing to note is that there is one and only one exception
throw in the entire API. It is specifically on calling $pool->getItem()
with an invalid key. Not a missing key, a key that is illegal according
to the spec. That is, it does not happen on a failure of the
Implementing Library (the pool) but an error by the Calling Library, who
is passing in a key that is nonsensical. The only exception is if the
Calling Library screws up. In fact, in PHP 7 I’d almost argue that it
should be an Error, not an Exception, as such an error can pretty much
only happen on developer error, not user or environmental error. Since
we need to be PHP 5 compatible, though, that’s not an option.

I would argue that

1) The Calling Library passing in a garbage key is an exceptional case.
2) Throwing an exception is therefore preferred to returning something
other than a CacheItem.

The alternative here would be to silently treat “invalid key” as a cache
miss and return a CacheItem with isHit() returning false instead, with
the Implementing Library possibly logging that fact if it were so
inclined; however, $item->getKey() would then by necessity have to
return either NULL (nope) or a known-invalid value. That seems a worse
state than now.

To summarize: In the current spec, a failure of the cache backend *never
produces an exception*. That is very consistent.


# Regarding CacheItem:

Let’s look specifically at CacheItem then. It contains 3 accessor
methods (get(), getKey(), and isHit()), all of which return a defined
meaningful value. It also contains three modifier methods (set(),
expiresAt(), and expiresAfter()), all of which return self. That allows
modifiers to be chained for convenience.

Now, chainable interfaces are somewhat controversial, as we all know.
The main problem with them is that they can easily get in the way of
wrapping an object in another object of the same interface. It’s
possible but becomes much more difficult to do. However, that is not
relevant in this case as there is no reason to ever wrap CacheItem. The
design of this API is such that CacheItem is only ever created by a
Pool, and is passed back into the pool moments later to be persisted.
The design already effectively precludes wrapping CacheItems in another
CacheItemInterface externally to the Pool, so making the modifiers
chainable doesn’t actually hurt anything but does improve DX. While I
am aware some disagree with that fundamental design, it is the
fundamental design of this PSR.

Also, for those modifiers there’s really nothing to fail. They are not
yet touching the underlying datastore, just modifying a value object in
memory. Thus, there’s no real error condition here that would need to
return false.


# Regarding CacheItemPool:

Let’s turn our attention to the Pool, which is a bit more interesting.
It has two accessors (getItem() and getItems()), both of which return a
meaningful domain value (CacheItem or a collection of CacheItems). It
also has one accessor that returns a boolean (hasItem()), which also
makes sense.

Then there’s clear(), which should modify data in the backend. It
returns success/failure (boolean). Unless we want it to throw an
exception (I don’t think we do), that should be fine. If you want to
know what the error was, I agree with those that say “that’s what
logging is for”. No, that’s not required by the spec but it doesn’t
need to be. Cache libraries that make it impossible to debug what
happened will be replaced by those that do. (Or they’ll get PRs.)

The same logic applies to commit as well.

That then leaves deleteItems(), save(), and saveDeferred(). All three
currently return self. I suspect that is, in part, vestigial from when
CacheItem had save() instead of the Pool. For deleteItems() and save(),
I would agree that returning self offers little value as there’s no use
case for wanting to chain such calls. saveDeferred() might, but only if
you’re chaining multiple saveDeferred() calls. Which... seems rather weird.

It is true that returning self means that there is no indication if the
action worked or not. However, it’s also true that the application
should continue the same way regardless, so the impact here is minor.
The Implementing Library can still log whatever it feels relevant to log.


# What if the backend is completely offline?

Currently that’s not defined explicitly. One could argue that the spec
implicitly states that such a condition may not throw an exception,
since one is not defined. I would be open to making that explicit,
however, and I don’t believe that would be an API change so it’s
Review-safe to do.

By and large I agree with those who have said that a cache backend being
offline is not exceptional enough to break an application. It should be
treated as a cache miss by the business logic. The app may slow down,
and it may fall over from too much traffic, but the business logic
itself shouldn’t need to change. So I would be against adding more
exceptions to the interface beyond the one we have now, which is NOT
triggered by a backend error or Implementing Library error, but by a
Calling Library error.


# Conclusion

In conclusion then, I would consider the following to be the only
changes to consider:

1) Clarify that the Implementing Library SHOULD log backend failures but
otherwise catch errors and treat them as a cache miss. I believe this
would be a Review-safe change.

2) Switch save, saveDeferred, and deleteItems to returning
success/failure (boolean). Although minor, I fear this would not be
Review-safe so I’m reluctant to do so as I really want this damned thing
passed. :-) However, if we need to reset Review anyway for the change
in Sponsor then I am open to this change.



--
--Larry Garfield

[Woody Gilk]

Larry,

This is a really great summary.

In regards to #2, I do think it is important that write operations return a boolean because if the item is already persisted and we are attempting to refresh it, we will end up with a stale cache with no way to know that. With a boolean return, we gain the ability to do:

if (!$pool->save($item)) {

    $pool->deleteItems([$item->getKey()]);

}

PS: If there the multi method deleteItems, why is there no singular deleteItem?

I don't want to derail the process (more than I already have) but has anyone thought about how this will work with extension? Right now it looks like CacheItemPool is going be creating CacheItem via getItem(), but there is no definition of a factory in the current spec. This means that a custom CacheItem cannot be fetched without modifying CacheItemPool to an unknown degree, which ends up being a bit of a mess.

Regards,

--
Woody Gilk
http://about.me/shadowhand

--
--Larry Garfield

--
You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/563125D4.3090607%40garfieldtech.com.

[Larry Garfield]

On 10/28/15 3:10 PM, Woody Gilk wrote:
> Larry,
>

> This is a really great summary.
>
> In regards to #2, I do think it is important that write operations
> return a boolean because if the item is already persisted and we are
> attempting to refresh it, we will end up with a stale cache with no
> way to know that. With a boolean return, we gain the ability to do:
>
> if (!$pool->save($item)) {
> $pool->deleteItems([$item->getKey()]);
> }
>
> PS: If there the multi method deleteItems, why is there no singular
> deleteItem?

Because $pool->deleteItems([$key]); is trivial enough for a Calling
Library, and a meaningful deleteItem() implementation would just do that
itself anyway.

> I don't want to derail the process (more than I already have) but has
> anyone thought about how this will work with extension? Right now it
> looks like CacheItemPool is going be creating CacheItem via getItem(),
> but there is no definition of a factory in the current spec. This
> means that a custom CacheItem cannot be fetched without modifying
> CacheItemPool to an unknown degree, which ends up being a bit of a mess.

We've thought about extension. A lot. Like, for months. :-) See the
metadoc for an example of how one might write a cache tag extension, for
instance.

Items and Pools in this model always come as a matched set. Always. If
you ever write $item = new CacheItem() outside of a Pool object, You're
Doing It Wrong(tm). That is, "a custom CacheItem cannot be fetched",
period. That's not a thing.

--
--Larry Garfield

[Woody Gilk]

On Wed, Oct 28, 2015 at 3:23 PM, Larry Garfield <la...@garfieldtech.com> wrote:

a meaningful deleteItem() implementation would just do that itself anyway.

So why not include it in the spec? More often than not, working with a single key would be more common than an array of keys.

[Paul Dragoonis]

On Wed, Oct 28, 2015 at 8:23 PM, Larry Garfield <la...@garfieldtech.com> wrote:

On 10/28/15 3:10 PM, Woody Gilk wrote:

Larry,

This is a really great summary.

In regards to #2, I do think it is important that write operations return a boolean because if the item is already persisted and we are attempting to refresh it, we will end up with a stale cache with no way to know that. With a boolean return, we gain the ability to do:

if (!$pool->save($item)) {
$pool->deleteItems([$item->getKey()]);
}

PS: If there the multi method deleteItems, why is there no singular deleteItem?

Because $pool->deleteItems([$key]); is trivial enough for a Calling Library, and a meaningful deleteItem() implementation would just do that itself anyway.

I'd really like this. All the implementing backends have singular and multiple read/write/delete methods. So we should be allowing people to continue to do that with our Interface.

It is indeed more common to delete a single item, (a Must Have) and having a fancy way to delete multiple items on a single socket call is a Nice To Have. I really want Basic get/set/delete to be here.

 

I don't want to derail the process (more than I already have) but has anyone thought about how this will work with extension? Right now it looks like CacheItemPool is going be creating CacheItem via getItem(), but there is no definition of a factory in the current spec. This means that a custom CacheItem cannot be fetched without modifying CacheItemPool to an unknown degree, which ends up being a bit of a mess.

We've thought about extension.  A lot. Like, for months. :-)  See the metadoc for an example of how one might write a cache tag extension, for instance.

Items and Pools in this model always come as a matched set. Always. If you ever write $item = new CacheItem() outside of a Pool object, You're Doing It Wrong(tm).  That is, "a custom CacheItem cannot be fetched", period. That's not a thing.

Bang on.

 

--
--Larry Garfield

--
You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/56312EA4.4050609%40garfieldtech.com.

[Paul Dragoonis]

Thanks for the much needed summary. Comments inline.

Definitely save() and deleteItem(s)() need boolean. I want this passed as much as you, but I'd like to reset REVIEW to make such necessary changes go through. This is why we have a review process in the first place.

--
--Larry Garfield

--
You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/563125D4.3090607%40garfieldtech.com.

[will.g...@couchbase.com]

Definitely save() and deleteItem(s)() need boolean. I want this passed as much as you, but I'd like to reset REVIEW to make such necessary changes go through. This is why we have a review process in the first place.

100% agreed, I think it's in slightly bad faith to suggest that we should only make a change if we have to reset review anyway.