Return Promise for set and remove

[maciejmrozinski]

Things done:

• Added more IDE friendly comments
• Return FulfilledPromise for ArrayCache implementation
• Modify tests with Promise checks

[jsor]

I think all doc blocks can be removed from ArrayCache as they are inherited from CacheInterface.

[maciejmrozinski]

You're totally right.

[jsor]

The test should probably be like

$mock = $this->createCallableMock();
$mock
    ->expects($this->once())
    ->method('__invoke');
    ->with($this->identicalTo(true))
;
$setPromise->then($mock);

Same for removeShouldRemoveKey().

[maciejmrozinski]

Done

[kelunik]

The allowed types for $key and $value should probably be added.

[jsor]

The allowed types for $key and $value should probably be added.

Strictly speaking, defining types for key and value are out of the scope of this PR. IMHO, the defintions should stay like this or removed at all.

[kelunik]

If they don't exist, they're out of scope, yes. But in that case, those @params are just noise and can be deleted.

[maciejmrozinski]

I removed them.

[jsor]

👍

[WyriHaximus]

Maybe we could add here what was expect and/or allow to be resolved through the promise. Is it just a simple bool or do we allow implementer specific information to be returned?

[jsor]

This is a good question and i though about that myself lately. Since the returned promise is just a placeholder for a value, i'd be great if the types of the actual value the placeholder will resolve to could be defined.

Maybe ask the authors of the PSR PHPDoc Standard?

For now, this should probably just stated in the description, eg.

@return PromiseInterface A promise which resolves to true on success.

[maciejmrozinski]

I'm not sure if true (or bool more generally) is a good idea. In Redis cache, the returning values are responses directly from Redis server (int, string, etc.). In example, for successful save operation "OK" string is returned as Redis returns it.

This is a general problem with Promises, that function should be described with PromiseInterface (to make IDE life easier and ours too) but we don't have any knowledge then about the value returned by this Promise itself when fulfilled. I saw in few libraries that declared return value was: PromiseInterface|int|string. We can simplify this to PromiseInterface|mixed. This will help code completition ans also give some tip about value retrieved from Promise itself. Implementing libraries can then narrow the return value to PromiseInterface|string or PromiseInterface|MyGreatClass.

Just a suggestion.

[jsor]

Sounds like a good compromise 👍

[kelunik]

@maciejmrozinski It doesn't help code completion, as mixed etc. isn't returned, it's the resolution value. The proper way is either using generics like Promise<int> or using @return int and an additional @promise or @async. That way, also @throws could be used. While this is a general problem in PHP, promises are that often in async programming that I think it's fine to introduce new PHPDoc tags for them.

[WyriHaximus]

I like the Promise<int> notation but that is just personal 😄 . What is more important that we define here what the developer can be expected to get from the promise.

[jsor]

This might not be the best place to discuss this topic. This probably more suited for a meta discussion. For now, i'd propose to simply define the the possible resolution values in the description.

/**
 * @return PromiseInterface A promise which resolves to TRUE if the entry was successfully stored in the cache, FALSE otherwise.
 */
public function set($key, $value);

/**
 * @return PromiseInterface A promise which resolves to TRUE if the cache entry was successfully removed, FALSE otherwise.
 */
public function remove($key);

[kelunik]

Shouldn't it just resolve to null and failure to store it in the cache be an exception?

[jsor]

This is debatable, but psr/simple-cache as well as doctrine/cache return true/false for set and delete/remove.

https://github.com/php-fig/simple-cache/blob/master/src/CacheInterface.php#L29
https://github.com/php-fig/simple-cache/blob/master/src/CacheInterface.php#L41

https://github.com/doctrine/cache/blob/master/lib/Doctrine/Common/Cache/Cache.php#L77
https://github.com/doctrine/cache/blob/master/lib/Doctrine/Common/Cache/Cache.php#L86

[kelunik]

"There was an error, but I won't tell you where or why it failed."

The reason for PSR-16 not to use an exception was that a failing cache shouldn't interrupt your code and keep things working. In case of promises you can just not wait for the promise or subscribe to it's resolution at all if you don't care.

[WyriHaximus]

@maciejmrozinski Could you make the changes about what the expected behavior is as discussed? Like to move forward with this issue and react/cache v0.5.0 in general

[WyriHaximus]

@maciejmrozinski I've opened a new PR based on this one, thanks for your work so far 👍