Skip to content
New issue

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

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

Already on GitHub? # to your account

Null object pattern #84

Merged
merged 2 commits into from
Jul 10, 2017
Merged

Null object pattern #84

merged 2 commits into from
Jul 10, 2017

Conversation

bpolaszek
Copy link
Contributor

Hi there,

I've implemented the Null Object Pattern to allow dynamic strategy assignment (i.e. when your CacheStrategy object comes from dependency injection).

This is simply a NullCacheStrategy object that implements CacheStrategyInterface without actually caching anything. This will help in looking for a valid CacheStrategyInterface without really enabling the cache (for debug purposes, for instance, but there are more use cases).

This pattern has been used in several popular projects, like in PSR-3, symfony/cache, ...

No BC break expected, it's just a new class.

*/
public function cache(RequestInterface $request, ResponseInterface $response)
{
return true;
Copy link
Owner

Choose a reason for hiding this comment

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

Should it not be false? And same for update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, when I read PHPdoc's return type hint for CacheStrategyInterface::cache() and CacheStrategyInterface::update(), it's:

/**
* @return bool true if success
*/

So I assumed returning false would mean a failure. However, to my eyes, nothing failed: the NullCacheStrategy accomplished its job with success (i.e. doing nothing).

I feared a further implementation would do this:

if (false === $strategy->cache($request, $response) {
    throw new \RuntimeException("There was an error when trying to cache the request/response");
}

Then, the NullCacheStrategy would systematically fail.

What's your opinion?

Copy link
Owner

Choose a reason for hiding this comment

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

However, to my eyes, nothing failed: the NullCacheStrategy accomplished its job with success (i.e. doing nothing).

Yes, you're right. I'm not familiar with this pattern, but I understand it. Thanks for your point of view!

@Kevinrob Kevinrob mentioned this pull request Jul 9, 2017
@Kevinrob Kevinrob self-assigned this Jul 9, 2017
@Kevinrob Kevinrob merged commit 8ec8d1b into Kevinrob:master Jul 10, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants