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.

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

[5.7] Add `missing()` method to the Cache #26351

Merged
merged 2 commits into from Nov 2, 2018

Conversation

Projects
None yet
4 participants
@dmitry-ivanov
Contributor

dmitry-ivanov commented Nov 1, 2018

The missing() method would be useful to avoid the negative conditions.

For example, I have an array of keys. Some of them are present in cache, some not.

If I want to get only those which are not (for the further caching), I have to use ! has() condition.

@dmitry-ivanov dmitry-ivanov changed the title from [5.7] Added `missing()` method to the Cache to [5.7] Add `missing()` method to the Cache Nov 1, 2018

@X-Coder264

This comment has been minimized.

Contributor

X-Coder264 commented Nov 1, 2018

This is a breaking change since you are adding a method to an interface which means that this can't go to 5.7. Besides I don't think this is useful enough to be added to the interface and force all implementations to implement that as the has method is already on the interface and there isn't anything new that you can achieve with this method that you can't with has.

@dmitry-ivanov

This comment has been minimized.

Contributor

dmitry-ivanov commented Nov 2, 2018

@X-Coder264 Hm.. Yes, you're right. This would be the breaking change, but only for those projects, who has their own implementation of the Cache\Repository contract. In the framework there is only one implementation, and I've fixed & tested it.

But please note, that we have two different contracts - one for the Repository and one for the Store:
https://github.com/laravel/framework/blob/5.7/src/Illuminate/Contracts/Cache/Repository.php
https://github.com/laravel/framework/blob/5.7/src/Illuminate/Contracts/Cache/Store.php

Only the Repository contract has the missing() method. So, neither the framework Stores nor the custom ones would be affected by this change. And I think that typically nobody needs to override the framework's Cache\Repository (it is also uses Macroable trait which reduces that risk even more).

I disagree with you, I think that such method is useful to avoid the negative conditions in the code.

Anyways, now I'm not sure that 5.7 is the correct branch. Probably it should be the master if the PR would be accepted.

@X-Coder264

This comment has been minimized.

Contributor

X-Coder264 commented Nov 2, 2018

It doesn't matter if you've added the method to the framework's only implementation of the contract. Adding a method to an interface is a breaking change and as such it must target the master branch. You'd break every app out there which has its own implementation of the interface. I don't know why there is even a need for explaining such an obvious breaking change 😄

As for the method itself, I still stand by my opinion that this doesn't need to be added to the interface. Each method on the interface provides an unique feature that the implementation must provide. This method brings nothing new to the table, it's just the negation of an already existing method on the interface. Like for example the Validator interface has only the fails method while the concrete implementation has also the opposite method called passes which is not on the interface.

@dmitry-ivanov

This comment has been minimized.

Contributor

dmitry-ivanov commented Nov 2, 2018

@X-Coder264 Makes sense. Great example, thank you.

I've removed the missing() method from the Contract, and now it's okay for 5.7 :)

@dmitry-ivanov

This comment has been minimized.

Contributor

dmitry-ivanov commented Nov 2, 2018

PS: @X-Coder264 I was wonder if it is a strict rule for any interface not to have the opposite methods... and I've found two cases where it doesn't - the Pagination\Paginator and the Support\MessageBag contracts both implement isEmpty() / isNotEmpty().

So, in general - it depends... not a strict rule to follow, I think...

@jmarcher

This comment has been minimized.

Contributor

jmarcher commented Nov 2, 2018

What is wrong with ! $cache->has('key') ?

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Nov 2, 2018

See above comment.

@taylorotwell taylorotwell reopened this Nov 2, 2018

@taylorotwell taylorotwell merged commit a481780 into laravel:5.7 Nov 2, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/styleci/pr The analysis has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment