Thanks for all the feedback!
On 26/11/2016 10:08, Nicolas Grekas wrote:
On CacheInterface:
- doesn't say what happens when $key is invalid => the same exception as
PSR-6?
I guess yes we have to take that over for compatibility. Sounds reasonable.
- the fact that `getMultiple` returns *all* keys, even cache misses,
makes it impossible to (efficiently) implement a PSR-16 to PSR-6 bridge,
because it makes it impossible to detect cache misses. When given an
array, `apcu_fetch` for example has this other behavior of returning
only the "hit", so it doesn't suffer from this. Could this be worth
considering?
IMO this kinda goes in the simplicity category, which is why I still
think enforcing an array return value vs a traversable would also be
simpler.
Using getMultiple() basically can be either:
if ($result['lala'] === null) { ... }
or having to iterate over results to check if it was in it, or having to
use iterator_to_array + if (!isset($result['lala']) { ... }
I find the former much easier, it's less smart (i.e. less surprising)
and possibly a tiny bit less performant if the cache implementation has
to check for missing keys and initialize them to null, but we're talking
really minor differences here in terms of perf.
- accepting `Traversable` for `*Multiple` methods is not consistent with
PSR-6 which only takes arrays as arguments
Agreed I think we can remove this..
- returning `array` for `getMultiple` is not consistent with PSR-6
`getItems` which returns `array|Traversable`
See 2 points above. I think array only would be best. If it's wrapping a
PSR-6 pool it can just normalize that with iterator_to_array()
- some methods return `void` when they could return `bool` => this is
both inconsistent with some other methods, and with PSR-6
Thanks, makes sense.
On CounterInterface:
- the draft doesn't say what happens when $key is invalid => the same
exception as PSR-6?
I'd say so.
- nor does it say what happens when $step is not an integer => return
false? throw something?
I'd say InvalidArgumentException there as well as it's misuse and not a
cache error.
- what should happen when $key already exists in the storage but is not
"incrementable/integer"? (Redis INCR fails, I didn't check apcu) =>
return false? throw? erase and store $step? other?
That's a backend error which is usually suppressed as if there was a
cache miss, but here people are going to expect a number back so not
sure if returning false is great (c.f strpos-style common mistakes..).
It is a kind of misuse so maybe throwing here is better?
- atomicity misses a normative MUST or SHOULD.
I'd say MUST otherwise it's kind of a pointless feature.
About exceptions:
- if the PSR is going to document when exceptions should be thrown, then
it should either define new exception classes or reuse those from PSR-6
- reusing exceptions defined in PSR-6 would look the most sensible to me
- yet, they are currently not in the same namespace. But: couldn't all
these new interfaces move in the Psr\Cache namespace (thus releasing
them as psr/cache 1.1 on packagist?)
That's an interesting idea, but also kinda confusing for users because
Psr\Cache would suddenly contain a Psr\Cache\Cache which sounds like the
thing to use, but also the Pool.. So maybe best to just duplicate the
InvalidArgumentException/CacheException into SimpleCache namespace.
Cheers
--
Jordi Boggiano
@seldaek - http://seld.be
--
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+unsubscr...@googlegroups.com.
To post to this group, send email to php-fig@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/php-fig/508314f0-6a69-dfdb-6379-19974caaddce%40seld.be.
For more options, visit https://groups.google.com/d/optout.
