Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
+++ b/core/modules/views/src/Plugin/views/filter/BooleanOperator.php
@@ -234,7 +234,7 @@ class BooleanOperator extends FilterPluginBase {
protected function queryOpBoolean($field, $query_operator = EQUAL) {
Default result "EQUAL" instead "="
Comment | File | Size | Author |
---|---|---|---|
#30 | interdiff-21-30.txt | 838 bytes | Anonymous (not verified) |
#30 | 2808321-30.patch | 4.93 KB | Anonymous (not verified) |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedvaplas created an issue. See original summary.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #3
Chi CreditAttribution: Chi commentedCould this be self::EQUAL?
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commented@Chi, thanks!
I'm like the copy-paste and documentation ability of
BooleanOperator::EQUAL
, butself
makes sense, too. And Drupal core have 22 cases withself::
and only 1 withSelfClass::
:But in contrast to the
Cache::PERMANENT
, theBooleanOperator::EQUAL
is not used anywhere outside (unfortunately).In the end,
self
shorter.Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedAnd what about doc? Example
Comment #7
benqwerty CreditAttribution: benqwerty as a volunteer commented@vaplas - thanks! Patch4 fixed an issue that showed up in the Devel module - #2811547 - PHP notice when using kint()
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commented@benqwerty, good catch. Thanks!
Background. It appeared here #2595169: Operator 'Is not equal' of BooleanOperator doesn't work. Method
queryOpBoolean()
hasn't default value before, and has incorrect default value after:This artefact came in #19, and then in #25 was suggested make method without default value (via BC layout).
Comment #9
LendudeFix looks good, but needs a test.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedTest is based on
FilterBooleanOperatorStringTest
+ first case ofFilterBooleanOperatorTest::testFilterBooleanOperator()
.Comment #12
LendudeLooks good! Little bit of nitpicking:
Needs to be one line.
Maybe something like :
Filter to test queryOpBoolean() with default operator.
{@inheritdoc} would do here
{@inheritdoc} would do here
Not needed, usually only used in assertIdenticalResultset()
should be \Drupal\views\Plugin\views\filter\BooleanOperator
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedThank you, @Lendude! Done.
Looks pretty! Maybe use it for other cases:
And is it correct name?
Because we testing
BooleanOperator
, but weFilterBooleanOperatorDefault
. MaybetestFilterBooleanOperatorDefault
?What better
'boolean_default'
or'boolean_default_test'
?Comment #14
LendudeYeah I'd go with
testFilterBooleanOperatorDefault
, but both are good enough in my opinion.'boolean_default_operator_test'
?I like my naming verbose, but that might just be me :)
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #16
Lendude@vaplas nice work.
We have a fix, we have a test.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedreroll by instruction.
Comment #18
xjmGood catch!
The one thing I noticed is that the declaration uses
self::EQUAL
for the default value but the method checksstatic::EQUAL
. That seems like the kind of thing that could lead to really weird bugs, if one is inheritance-aware and the other isn't.Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedThank you, @xjm! Done. + replace
static::
toself::
in doc:Do we need to use
static::EQUAL
andstatic::NOT_EQUAL
instead of'='
and'<>'
in this comment?// Forces an '=' operator instead of a '<>' for performance reasons.
Comment #20
LendudeYeah that would make sense to me, should we ever change the constants, that comment makes no sense.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commented@Lendude, thank you!
Comment #22
LendudeAll feedback has been addressed, back to RTBC
Comment #23
alexpottWhy was
self::
chosen overstatic::
. I think we should always preferstatic::
overself::
unless we have good reason. Usingself::
makes classes less flexible. Also changing from self to static at this point is BC change. So I think we should just use static.Comment #24
alexpottAnd if self is right - and I don't think it is... you would also need to update \Drupal\views\Plugin\views\filter\BooleanOperator::operators()
Comment #25
jofitz CreditAttribution: jofitz at ComputerMinds commentedReplaced
self::
withstatic::
.Comment #26
alexpottNeed to fix the original bug too :)
Comment #27
jofitz CreditAttribution: jofitz at ComputerMinds commentedSorry, my mistake, I didn't quite understand the issue. How should it be fixed? Apparently "static:: is not allowed in compile-time constants".
Comment #28
LendudeWhich answers @alexpotts question:
for that reason. And then self was used throughout the method for consistency
Comment #29
alexpottAh ok - my mistake. So this is an interesting thing cause in some ways the switch to
self::
is a BC break.But this is theoretical because no one is going to use a different operator than = here so maybe
self::
is okay. But also this method is never called with an operator so the default is never actually used.Let's just change all instances to self:: and be done - so that's #21 plus fixing
\Drupal\views\Plugin\views\filter\BooleanOperator::operators()
.Comment #30
Anonymous (not verified) CreditAttribution: Anonymous commented#29 done, thanks!
Comment #32
LendudeThanks @alexpott for the feedback and explanation.
All feedback has been addressed, back to RTBC.
Comment #33
alexpottCommitted and pushed cda222e to 8.4.x and dd3de11 to 8.3.x. Thanks!
Backported to 8.3.x as this is a bugfix that does not make any BC changes and fixes a PHP notice.
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks! I understand that this is a nit improvement (especially against the background of the latest megabyte cleanup patches). But this improvement is also justified. At least fewer distracting messages when analyzing the code.
#7 points to #2811547: PHP notice when using kint() with PHP notice about it. Can anyone provide how to correctly close it?