Skip to content

Return Promise for set and remove #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
maciejmrozinski wants to merge 5 commits into from
Closed

Return Promise for set and remove #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
456b243
Return Promise for set and remove
Mar 3, 2017
5dc49c1
Fix tests for PHP 5.3
Mar 3, 2017
214113b
Remove unnecessary doc blocks
Mar 4, 2017
92f2645
Remove params doc blocks
Mar 6, 2017
ca6a8f9
Changes in tests
Mar 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/ArrayCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@ public function get($key)
public function set($key, $value)
{
$this->data[$key] = $value;
return new Promise\FulfilledPromise(true);
}

public function remove($key)
{
unset($this->data[$key]);
return new Promise\FulfilledPromise(true);
}
}
12 changes: 11 additions & 1 deletion src/CacheInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,22 @@

namespace React\Cache;

use React\Promise\PromiseInterface;

interface CacheInterface
{
// @return React\Promise\PromiseInterface
/**
* @return PromiseInterface
*/
public function get($key);

/**
* @return PromiseInterface
*/
public function set($key, $value);

/**
* @return PromiseInterface
Copy link
Member

@WyriHaximus WyriHaximus Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@jsor jsor Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@maciejmrozinski maciejmrozinski Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jsor jsor Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good compromise 👍

Copy link

@kelunik kelunik Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@WyriHaximus WyriHaximus Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jsor jsor Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link

@kelunik kelunik Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jsor jsor Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@kelunik kelunik Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

*/
public function remove($key);
}
23 changes: 21 additions & 2 deletions tests/ArrayCacheTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

class ArrayCacheTest extends TestCase
{
/**
* @var ArrayCache
*/
private $cache;

public function setUp()
Expand All @@ -27,9 +30,17 @@ public function getShouldRejectPromiseForNonExistentKey()
/** @test */
public function setShouldSetKey()
{
$this->cache
$setPromise = $this->cache
->set('foo', 'bar');

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

$setPromise->then($mock);

$success = $this->createCallableMock();
$success
->expects($this->once())
Expand All @@ -47,9 +58,17 @@ public function removeShouldRemoveKey()
$this->cache
->set('foo', 'bar');

$this->cache
$removePromise = $this->cache
->remove('foo');

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

$removePromise->then($mock);

$this->cache
->get('foo')
->then(
Expand Down