Return Promise for set and remove by WyriHaximus · Pull Request #23 · reactphp/cache

WyriHaximus · 2018-02-18T21:36:18Z

This PR builds on #16 by resolving merge conflicts and adding more documentation in the cache interface plus adding documentation to the readme.

Supersedes / closes #16

jsor · 2018-02-20T16:41:44Z

src/ArrayCache.php

    public function set($key, $value)
    {
        $this->data[$key] = $value;
+        return new Promise\FulfilledPromise(true);


I'd prefer Promise\resolve(true) here.

jsor · 2018-02-20T16:41:53Z

src/ArrayCache.php

    public function remove($key)
    {
        unset($this->data[$key]);
+        return new Promise\FulfilledPromise(true);


I'd prefer Promise\resolve(true) here.

jsor · 2018-02-20T16:43:18Z

src/CacheInterface.php

     * @param string $key
     * @param mixed $value
-     * @return void
+     * @return PromiseInterface<bool|Exception>


As discussed (ot sure where atm), only fulfillment values should be included.

PromiseInterface<bool>

jsor · 2018-02-20T16:43:30Z

src/CacheInterface.php

     *
     * @param string $key
-     * @return void
+     * @return PromiseInterface<bool|Exception>


PromiseInterface<bool> (see above)

…jsor

…jsor at reactphp#23 (comment) and reactphp#23 (comment)

@jsor

…nstance via @jsor at reactphp#23 (comment) and reactphp#23 (comment)

WyriHaximus · 2018-02-20T20:52:35Z

@jsor addressed your points 👍

jsor · 2018-02-21T07:03:30Z

src/CacheInterface.php

+/**
+ * All methods only reject with an exception when an error occurs on the underlying storage
+ * layer, for example the connection with Redis dropped and cannot be recovered, or the
+ * directory on the filesystem used to store cache has the wrong permissions of is full.


of is full -> or is full

WyriHaximus · 2018-02-21T08:09:46Z

@jsor fixed 👍

clue

No objections, but one question:

What is an "error" and how is this related to #22? It's my understanding that we aim for a Promise-based alternative interface similar to PSR-16, which uses the following return type for set():

@return bool True on success and false on failure.
@throws \Psr\SimpleCache\InvalidArgumentException MUST be thrown if the $key string is not a legal value.

jsor · 2018-02-26T21:50:51Z

Good point, there was also a discussion here: https://github.com/reactphp/cache/pull/16/files#r104752102

WyriHaximus · 2018-02-27T21:39:12Z

@jsor @clue updated PR with the points discussed during our call

jsor · 2018-02-28T07:57:13Z

Regarding set() now returning null: That differs now from PSR-16. As @clued noted in #23 (review):

@return bool True on success and false on failure.

But since i've not attended the call, i'm probably missing something.

WyriHaximus · 2018-02-28T08:54:26Z

@jsor right so the thing @clue and I discussed is that when it always resolves true on success or rejects with an error we might as well resolve with null because true doesn't add any value.

jsor · 2018-02-28T09:28:48Z

The point is, that according to PSR-16, set() never throws, except for an invalid key. That means, our promise only rejects for an invalid key and resolves to true/false otherwise.

Refs:

WyriHaximus · 2018-02-28T21:55:06Z

@jsor that makes sense so we should always resolve with true on success false on failure. That will mean the use won't get any exceptions but the cache implementation should handle/log those them self. Makes perfect sense to me. What do you think @clue ?

clue · 2018-03-13T17:14:23Z

We've discussed this only briefly in our last hangouts. My point is: I can see some valid arguments for both directions as rightfully pointed out by @jsor. I personally don't have a use case that requires any specific behavior, I only require consistent behavior.

For now, I would vote to keep this in line with PSR-16 and simply use a promise with a boolean resolution value and basically never reject.

I would rather get this feature out and we can still discuss this for a future version

WyriHaximus · 2018-03-24T21:38:37Z

Ping @jsor && @clue updated the PR

clue · 2018-03-25T09:50:13Z

src/CacheInterface.php

     * Retrieve an item from the cache, resolves with its value on
-     * success or null when no item can be found.
+     * success or null when no item can be found. It will reject with an exception
+     * on error. (Note that a cache miss isn't an error.)


Rejecting would be the async equivalent of an Exception and I don't see this in PSR-16?

clue · 2018-03-25T09:51:17Z

src/CacheInterface.php

+ * All methods only reject with an exception when an error occurs on the underlying storage
+ * layer, for example the connection with Redis dropped and cannot be recovered, or the
+ * directory on the filesystem used to store cache has the wrong permissions or is full.
+ */


Not sure if this is a leftover, but this seems to contradict the below definitions?

clue · 2018-03-25T09:53:45Z

src/CacheInterface.php

     * @param string $key
     * @param mixed $value
-     * @return void
+     * @return PromiseInterface<bool>


I don't see any notation for this in any of the major projects, so I would suggest using the more verbose form @return PromiseInterface Returns a promise which resolves to true on success of false on error and possibly update the description above? (See also below)

WyriHaximus · 2018-03-25T10:23:53Z

@clue updated the cache interface 👍

clue · 2018-03-25T11:12:57Z

README.md


-If the key `foo` does not exist, the promise will be fulfilled with `null` as value.
+If the key `foo` does not exist, the promise will be fulfilled with `null` as value. On 
+any error it will reject with an exception.


WyriHaximus · 2018-03-25T16:09:00Z

@clue whoops updated

clue

LGTM, please squash this to a reasonable number of commits before merging

WyriHaximus · 2018-03-25T17:19:58Z

When @jsor also doesn't find any issues and approves I'll squash everything 👍

jsor · 2018-03-26T06:51:28Z

README.md

-already exists, it is overridden. No guarantees are made as to when the cache
-value is set. If the cache implementation has to go over the network to store
+already exists, it is overridden. To provide guarantees as to when the cache
+value is set a promise is returned. Which will fulfill with `true` on success or 


This should either be one sentence ("...a promise is returned which will fulfill...") or better separated ("...a promise is returned. The promise will fulfill...").

WyriHaximus · 2018-03-26T09:42:09Z

@jsor done

WyriHaximus · 2018-03-26T12:17:57Z

Squashed the commits, the whole history without the squash is available here: https://github.com/WyriHaximus-labs/cache/tree/return_promises_history

WyriHaximus added new feature maintenance help wanted labels Feb 18, 2018

WyriHaximus added this to the v0.5.0 milestone Feb 18, 2018

WyriHaximus requested review from clue and jsor February 18, 2018 21:36

jsor reviewed Feb 20, 2018

View reviewed changes

WyriHaximus added a commit to WyriHaximus-labs/cache that referenced this pull request Feb 20, 2018

@return PromiseInterface<*> should only show fulfillment values via @…

8859423

…jsor at reactphp#23 (comment) and reactphp#23 (comment)

WyriHaximus added a commit to WyriHaximus-labs/cache that referenced this pull request Feb 20, 2018

Use the resolve function instead of creating a new FulfilledPromise i…

fc8dd1a

…nstance via @jsor at reactphp#23 (comment) and reactphp#23 (comment)

jsor reviewed Feb 21, 2018

View reviewed changes

WyriHaximus added a commit to WyriHaximus-labs/cache that referenced this pull request Feb 21, 2018

of => or via @jsor at reactphp#23 (comment)

0f7a224

clue reviewed Feb 26, 2018

View reviewed changes

clue requested changes Mar 25, 2018

View reviewed changes

clue reviewed Mar 25, 2018

View reviewed changes

clue approved these changes Mar 25, 2018

View reviewed changes

clue added BC break and removed help wanted labels Mar 25, 2018

jsor reviewed Mar 26, 2018

View reviewed changes

WyriHaximus added a commit to WyriHaximus-labs/cache that referenced this pull request Mar 26, 2018

Fixed readme issue as suggested by @jsor reactphp#23 (comment)

a814de4

jsor approved these changes Mar 26, 2018

View reviewed changes

Maciej Mroziński and others added 2 commits March 26, 2018 14:15

Return Promise for set and remove

0b2326a

Describe current behavior on the cache interface

584e3c2

WyriHaximus force-pushed the return_promises branch from a814de4 to 584e3c2 Compare March 26, 2018 12:15

WyriHaximus merged commit 9478319 into reactphp:master Mar 26, 2018

WyriHaximus mentioned this pull request Mar 26, 2018

Return values for set and remove #15

Closed

Uh oh!

Conversation

WyriHaximus commented Feb 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Feb 21, 2018

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

jsor commented Feb 26, 2018

Uh oh!

WyriHaximus commented Feb 27, 2018

Uh oh!

jsor commented Feb 28, 2018

Uh oh!

WyriHaximus commented Feb 28, 2018

Uh oh!

jsor commented Feb 28, 2018

Uh oh!

WyriHaximus commented Feb 28, 2018

Uh oh!

clue commented Mar 13, 2018

Uh oh!

WyriHaximus commented Mar 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Mar 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Mar 25, 2018

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Mar 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Mar 26, 2018

Uh oh!

WyriHaximus commented Mar 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants