-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix notcontains search option operator #18003
base: 10.0/bugfixes
Are you sure you want to change the base?
fix notcontains search option operator #18003
Conversation
008851d
to
5bb30bc
Compare
|
||
// Potential negation will be handled by the subquery operator | ||
$SEARCH = self::makeTextSearch($val, false); | ||
$subquery_operator = $nott ? "IN" : "NOT IN"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is inverted compared to "contains" counterpart a few lines below
5bb30bc
to
d9f2d67
Compare
@@ -4744,8 +4744,52 @@ public static function addWhere($link, $nott, $itemtype, $ID, $searchtype, $val, | |||
|
|||
switch ($searchtype) { | |||
case "notcontains": | |||
$nott = !$nott; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted when we reach
https://github.com/glpi-project/glpi/blob/10.0/bugfixes/src/Search.php#L5405
8e5cf01
to
d9f2d67
Compare
d9f2d67
to
6920525
Compare
6920525
to
4e89aa8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Search has always been opaque to my eyes, but added test seems OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of duplicating the whole case code, maybe it was possible to just adapt a few lines.
For instance:
if ($searchtype === 'contains') {
$subquery_operator = $nott ? "NOT IN" : "IN";
} else {
$subquery_operator = $nott ? "IN" : "NOT IN";
}
and
$SEARCH = self::makeTextSearch($val, $searchtype === 'contains' ? $nott : !$nott);
Yes, it would make sense. But I found there is a real mess with contains / notcontains on one hand and equals / not equals in the other hand. For numeric search options one or the other pair is used in function of the column being a ID or FK. So I adopted the less risky solution, by reproducing the code pattern matching equals / not equals (which as far as I know is bug free). |
Quality Gate failedFailed conditions |
SQL requests are wrong when using a search criteria with the operator notcontains.
It seems there is no unit tests for this, then I added them.
To reproduce
Need to find a search option having datatype = number without min nor max boundaries. Printer current count of page is an easy to test candidate.
Checklist before requesting a review
Please delete options that are not relevant.
Description
The variable
$nott
is used later in Search::addWhere() leading to a double negation. I de-factorized the code to fix the issue to stick in the same design as equals / notequals operators implementation.Screenshots (if appropriate):