Problem/Motivation

This is the code for InOperator::validate():

    elseif (!empty($this->value) && ($this->operator == 'in' || $this->operator == 'not in')) {
      $errors[] = $this->t('The value @value is not an array for @operator on filter: @filter', ['@value' => var_export($this->value), '@operator' => $this->operator, '@filter' => $this->adminLabel(TRUE)]);
    }

It want use var_export to get the parsable string for value, but forgot the second parameter, so it will outputs the string and break the JSON output format for AJAX call.

Proposed resolution

Fix it.

CommentFileSizeAuthor
#16 interdiff_11-16.txt562 bytesravi.shankar
#16 3074595-16.patch2.28 KBravi.shankar
#11 3074595-11.patch2.28 KBLendude
#11 3074595-11-TEST_ONLY.patch1.39 KBLendude
#7 issue-3074595-7.patch812 bytessime
#2 3074595-var_export-2.patch912 bytesjian he
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jian he created an issue. See original summary.

jian he’s picture

jian he CreditAttribution: jian he commented
Status: Active » Needs review
FileSize
3074595-var_export-2.patch912 bytes

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.9 was released on November 6 and is the final full bugfix release for the Drupal 8.7.x series. Drupal 8.7.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.8.0 on December 4, 2019. (Drupal 8.8.0-beta1 is available for testing.)

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

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

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

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 for Hook 42 commented
Version: 8.9.x-dev » 9.1.x-dev
Issue tags: +Bug Smash Initiative

Patch still applies to 9.1.x.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 for Hook 42 commented

Thanks for the issue and patch.

1) Checked all of core and the only place not using TRUEis InOperator.php.

2) Kicking off tests for 9.1.x.

3) Is there a way to test this manually?

sime’s picture

sime
any/any
Location Melbourne
CreditAttribution: sime at Lil Engine commented
FileSize
issue-3074595-7.patch812 bytes

I agree the code won't work as expected but I assume the filter UI (eg checkboxes) ensures this value is always an array. So I can't think of a way to reproduce this through the UI.

I also can't find any tests that cover this piece of code. I removed the whole clause and I ran a few of the coverage tests, and nothing failed.

modules/views/tests/src/Kernel/Handler/FilterInOperatorTest.php
modules/views/tests/src/Functional/Plugin/FilterTest.php

Posting a patch that removes the entire clause just to see if any tests fail.

sime’s picture

sime
any/any
Location Melbourne
CreditAttribution: sime at Lil Engine commented
Assigned: Unassigned » sime

Assigned myself for follow up. Find me in #bugsmash if I'm tardy.

sime’s picture

sime
any/any
Location Melbourne
CreditAttribution: sime at Lil Engine commented
Status: Needs review » Needs work

So the tests pass if we remove that piece of code. I think at the very least it needs a test. Although I'm of the opinion the code never gets triggered unless a filter plugin has been badly coded.

sime’s picture

sime
any/any
Location Melbourne
CreditAttribution: sime at Lil Engine commented
Assigned: sime » Unassigned

Looking for some feedback here, not sure how to functional/kernel test this if all the related methods are setting the ->value correctly as an array. So maybe a unit test? But I don't see any existing unit tests for this code... i don't think. Perhaps the patch is fine as is.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented
Status: Needs work » Needs review
FileSize
3074595-11-TEST_ONLY.patch1.39 KB
3074595-11.patch2.28 KB

Let's add a unit test for this.

I would say the best way forward here is #2. While I like removing things, I think this is a valid check. We would lose some developer feedback if this would just silently pass validation even when the value is invalid (even if this can only be achieved with bad code).

The last submitted patch, 11: 3074595-11-TEST_ONLY.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

sime’s picture

sime
any/any
Location Melbourne
CreditAttribution: sime at Lil Engine commented

Thanks @lendude, I've learned from this.

Tested the patch locally and it looks RTBC.

init90’s picture

init90
Location Ukraine
CreditAttribution: init90 at ZANZARRA Drupal Agency, Drupal Ukraine Community commented
Status: Needs review » Reviewed & tested by the community
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/views/tests/src/Unit/Plugin/filter/InOperatorTest.php
@@ -0,0 +1,39 @@
+  static public function validate_options_callback() {

Nit: static public -> public static

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
3074595-16.patch2.28 KB
interdiff_11-16.txt562 bytes

Fixed comment #15.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Thanks, back to RTBC :)

  • larowlan committed 98856bb on 9.1.x 
    Issue #3074595 by Lendude, ravi.shankar, sime, jian he, Kristen Pol,...

  • larowlan committed 264a36a on 9.0.x 
    Issue #3074595 by Lendude, ravi.shankar, sime, jian he, Kristen Pol,...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Title: var_export only returns if the second parameter set to TRUE » [backport] var_export only returns if the second parameter set to TRUE
Version: 9.1.x-dev » 8.9.x-dev

Committed 98856bb and pushed to 9.1.x. Thanks!

C/p to 9.0

Kicking a test-run off for 8.9.x for possible backport

  • larowlan committed 23da768 on 8.9.x 
    Issue #3074595 by Lendude, ravi.shankar, sime, jian he, Kristen Pol,...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Title: [backport] var_export only returns if the second parameter set to TRUE » var_export only returns if the second parameter set to TRUE
Status: Reviewed & tested by the community » Fixed

c/p to 8.9.x - thanks

Status: Fixed » Closed (fixed)

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