Api improvement for isSearch. #164
Conversation
Codecov Report
@@ Coverage Diff @@
## master #164 +/- ##
============================================
- Coverage 99.41% 99.41% -0.01%
+ Complexity 151 146 -5
============================================
Files 11 11
Lines 344 340 -4
============================================
- Hits 342 338 -4
Misses 2 2
Continue to review full report at Codecov.
|
| continue; | ||
| } | ||
| $result = $filter->process(); | ||
| if ($result !== false) { |
There was a problem hiding this comment.
Explicit check for false will cause problems with existing custom filters people have which don't return anything.
There was a problem hiding this comment.
I did this on purpose. All current would Return null and behave Like true.
There was a problem hiding this comment.
This is already the current behavior. So it only Got better now, Not worse.
There was a problem hiding this comment.
Ah right it checks for not false. This is why i dislike negations in general, easy to misread :) Valid use here.
|
I rebased and resolved the conflict. |
|
I suggest the following release notes for the tag 3.0.1: |
|
Why all the test removals? |
|
Because they are obsolete with this improvement. You don't have to check for the skip being called. |
|
I rebased it - should be clear now. |
|
Not bc as it changes the signature of a method. I think this is okay to go into 3.1.0 though. |
|
The idea was to release a 3.0.1 for it all after the release of 3.0.0 was just the other day. |
|
But both is fine. |
|
@josegonzalez Strictly speaking it is BC, the method signature only changes on the semantic level. |
src/Model/Filter/Value.php
Outdated
| if (!empty($expressions)) { | ||
| $this->getQuery()->andWhere([$this->config('mode') => $expressions]); | ||
| } | ||
| $this->getQuery()->andWhere([$this->config('mode') => $expressions]); |
There was a problem hiding this comment.
Did I rebase wrongly?
There was a problem hiding this comment.
We removed the empty check in favor of validation expressions.
|
Strictly speaking, |
|
We both know what strictly means in a PHP world pre-7.1 - where we do not typehint anything scalar right now. |
|
And just to be clear: A change to bool (or anything non-void) is always BC from void - not the other way around. |
|
Sorry, I think we'll agree to disagree. In any case, this is good to go. |
After be just bumped the major, I think this should also land in that new version.
It is BC - and improves the API by clearly returning a bool result on process() to indicated if a search filter has been actually applied.
It also simplifies the code since skip() doesnt need to be invoked twice.