+++ 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 "="

CommentFileSizeAuthor
#30 interdiff-21-30.txt838 bytesAnonymous (not verified)
#30 2808321-30.patch4.93 KBAnonymous (not verified)
#25 interdiff-21-25.txt1.92 KBjofitz
#25 2808321-25.patch3.64 KBjofitz
#21 interdiff-19-21.txt805 bytesAnonymous (not verified)
#21 2808321-21.patch4.42 KBAnonymous (not verified)
#19 interdiff-17-19.txt1.74 KBAnonymous (not verified)
#19 2808321-19.patch4.3 KBAnonymous (not verified)
#17 2808321-17.patch3.21 KBAnonymous (not verified)
#15 interdiff-10-15.txt2.4 KBAnonymous (not verified)
#15 2808321-15.patch3.22 KBAnonymous (not verified)
#10 2808321-10.patch3.52 KBAnonymous (not verified)
#10 2808321-10-test-only.patch2.81 KBAnonymous (not verified)
#4 2808321-4.patch746 bytesAnonymous (not verified)
#2 2808321-2.patch757 bytesAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

vaplas created an issue. See original summary.

Anonymous’s picture

Status: Active » Needs review
FileSize
757 bytes
Chi’s picture

Could this be self::EQUAL?

Anonymous’s picture

@Chi, thanks!

I'm like the copy-paste and documentation ability of BooleanOperator::EQUAL, but self makes sense, too. And Drupal core have 22 cases with self:: and only 1 with SelfClass:::

class Cache {
  public static function mergeMaxAges($a = Cache::PERMANENT, $b = Cache::PERMANENT) {

But in contrast to the Cache::PERMANENT, the BooleanOperator::EQUAL is not used anywhere outside (unfortunately).

In the end, self shorter.

Anonymous’s picture

And what about doc? Example

-   *   (optional) Either static::EQUAL or static::NOT_EQUAL. Defaults to
-   *   static::EQUAL.
+   *   (optional) Either EQUAL or NOT_EQUAL. Defaults to EQUAL.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

benqwerty’s picture

@vaplas - thanks! Patch4 fixed an issue that showed up in the Devel module - #2811547 - PHP notice when using kint()

Anonymous’s picture

@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:

- protected function queryOpBoolean($field) {
+ protected function queryOpBoolean($field, $query_operator = EQUAL) {

This artefact came in #19, and then in #25 was suggested make method without default value (via BC layout).

+++ b/core/modules/views/src/Plugin/views/filter/BooleanOperator.php
@@ -226,15 +226,29 @@ public function query() {
-  protected function queryOpBoolean($field, $query_operator = EQUAL) {
+  protected function queryOperatorBoolean($field, $query_operator) {
...
   protected function queryOpBoolean($field) {
+    $this->queryOperatorBoolean($field, static::EQUAL);
   }
Lendude’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Fix looks good, but needs a test.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
2.81 KB
3.52 KB

Test is based on FilterBooleanOperatorStringTest + first case of FilterBooleanOperatorTest::testFilterBooleanOperator().

The last submitted patch, 10: 2808321-10-test-only.patch, failed testing.

Lendude’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Looks good! Little bit of nitpicking:

  1. +++ b/core/modules/views/tests/modules/views_test_data/src/Plugin/views/filter/FilterBooleanOperatorDefaultTest.php
    @@ -0,0 +1,26 @@
    + * Filter for test the BooleanOperator::queryOpBoolean() with default
    + * $query_operator parameter.
    

    Needs to be one line.

    Maybe something like :
    Filter to test queryOpBoolean() with default operator.

  2. +++ b/core/modules/views/tests/src/Kernel/Handler/FilterBooleanOperatorDefaultTest.php
    @@ -0,0 +1,69 @@
    +  /**
    +   * The modules to enable for this test.
    +   *
    +   * @var array
    +   */
    

    {@inheritdoc} would do here

  3. +++ b/core/modules/views/tests/src/Kernel/Handler/FilterBooleanOperatorDefaultTest.php
    @@ -0,0 +1,69 @@
    +  /**
    +   * Views used by this test.
    +   *
    +   * @var array
    +   */
    

    {@inheritdoc} would do here

  4. +++ b/core/modules/views/tests/src/Kernel/Handler/FilterBooleanOperatorDefaultTest.php
    @@ -0,0 +1,69 @@
    +  /**
    +   * Map column names.
    +   *
    +   * @var array
    +   */
    +  protected $columnMap = array(
    +    'views_test_data_id' => 'id',
    +  );
    

    Not needed, usually only used in assertIdenticalResultset()

  5. +++ b/core/modules/views/tests/src/Kernel/Handler/FilterBooleanOperatorDefaultTest.php
    @@ -0,0 +1,69 @@
    + * @see \Drupal\views\Plugin\views\filter\BooleanOperatorString
    

    should be \Drupal\views\Plugin\views\filter\BooleanOperator

Anonymous’s picture

Thank you, @Lendude! Done.

Filter to test queryOpBoolean() with default operator.

Looks pretty! Maybe use it for other cases:

+++ b/core/modules/views/tests/src/Kernel/Handler/FilterBooleanOperatorDefaultTest.php
@@ -0,0 +1,69 @@
+ * Tests the BooleanOperator::queryOpBoolean() with default $query_operator.
...
+   * Tests the BooleanOperator::queryOpBoolean() with default $query_operator.

And is it correct name?

+++ b/core/modules/views/tests/src/Kernel/Handler/FilterBooleanOperatorDefaultTest.php
@@ -0,0 +1,69 @@
+  public function testFilterBooleanOperator() {

Because we testing BooleanOperator, but we FilterBooleanOperatorDefault. Maybe testFilterBooleanOperatorDefault?

What better 'boolean_default' or 'boolean_default_test'?

Lendude’s picture

Yeah 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 :)

Anonymous’s picture

Status: Needs work » Needs review
FileSize
3.22 KB
2.4 KB
Lendude’s picture

Status: Needs review » Reviewed & tested by the community

@vaplas nice work.

We have a fix, we have a test.

Anonymous’s picture

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Good catch!

+++ b/core/modules/views/src/Plugin/views/filter/BooleanOperator.php
@@ -234,7 +234,7 @@ public function query() {
+  protected function queryOpBoolean($field, $query_operator = self::EQUAL) {
     if (empty($this->value)) {
       if ($this->accept_null) {
         if ($query_operator == static::EQUAL) {

The one thing I noticed is that the declaration uses self::EQUAL for the default value but the method checks static::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.

Anonymous’s picture

Thank you, @xjm! Done. + replace static:: to self:: in doc:

+++ b/core/modules/views/src/Plugin/views/filter/BooleanOperator.php
@@ -234,7 +234,7 @@ public function query() {
    *   (optional) Either static::EQUAL or static::NOT_EQUAL. Defaults to
    *   static::EQUAL.

Do we need to use static::EQUAL and static::NOT_EQUAL instead of '=' and '<>' in this comment?
// Forces an '=' operator instead of a '<>' for performance reasons.

Lendude’s picture

Do we need to use static::EQUAL and static::NOT_EQUAL instead of '=' and '<>' in this comment?

Yeah that would make sense to me, should we ever change the constants, that comment makes no sense.

Anonymous’s picture

@Lendude, thank you!

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

All feedback has been addressed, back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Why was self:: chosen over static::. I think we should always prefer static:: over self:: unless we have good reason. Using self:: makes classes less flexible. Also changing from self to static at this point is BC change. So I think we should just use static.

alexpott’s picture

And if self is right - and I don't think it is... you would also need to update \Drupal\views\Plugin\views\filter\BooleanOperator::operators()

jofitz’s picture

Status: Needs work » Needs review
FileSize
3.64 KB
1.92 KB

Replaced self:: with static::.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/filter/BooleanOperator.php
@@ -237,7 +237,7 @@ public function query() {
   protected function queryOpBoolean($field, $query_operator = EQUAL) {

Need to fix the original bug too :)

jofitz’s picture

Sorry, my mistake, I didn't quite understand the issue. How should it be fixed? Apparently "static:: is not allowed in compile-time constants".

Lendude’s picture

Apparently "static:: is not allowed in compile-time constants".

Which answers @alexpotts question:

Why was self:: chosen over static::

for that reason. And then self was used throughout the method for consistency

alexpott’s picture

Ah ok - my mistake. So this is an interesting thing cause in some ways the switch to self:: is a BC break.

+++ b/core/modules/views/src/Plugin/views/filter/BooleanOperator.php
@@ -255,12 +255,13 @@ protected function queryOpBoolean($field, $query_operator = EQUAL) {
+          $this->query->addWhere($this->options['group'], $field, 1, self::EQUAL);

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().

Anonymous’s picture

The last submitted patch, 25: 2808321-25.patch, failed testing.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @alexpott for the feedback and explanation.

All feedback has been addressed, back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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.

  • alexpott committed cda222e on 8.4.x
    Issue #2808321 by vaplas, Jo Fitzgerald, Lendude, alexpott, xjm, Chi,...

  • alexpott committed dd3de11 on 8.3.x
    Issue #2808321 by vaplas, Jo Fitzgerald, Lendude, alexpott, xjm, Chi,...
Anonymous’s picture

Thanks! 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?

  1. Fixed
  2. Closed (fixed)
  3. Closed (duplicate)
  4. Closed (cannot reproduce)
  5. don't close, but add post with info about fix it for >= 8.3.x ?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.