Problem/Motivation

Attempt to override query settings on a views display (e.g. attachment) throws the following error:

TypeError: explode(): Argument #2 ($string) must be of type string, array given in explode() (line 329 of /.../core/modules/views/src/Plugin/views/query/Sql.php)

Steps to reproduce

  1. Create a view or edit an existing view
  2. Add a new page display
  3. Expand the Advanced fieldset and click Query settings: Settings
  4. Set the "For" dropdown to "This page (override)"
  5. Click "Apply (this display)"

Proposed resolution

Prevent Drupal\views\Plugin\views\query\Sql::submitOptionsForm() from processing the values twice.

CommentFileSizeAuthor
#11 3231861-11.patch3.46 KBLendude
#11 interdiff-3231861-8-11.txt790 bytesLendude
#8 3231861-8.patch4.09 KBLendude
#8 interdiff-3231861-3-8.txt967 bytesLendude
#4 3231861-3.patch3.86 KBLendude
#4 3231861-3-TEST_ONLY.patch2.37 KBLendude
#3 3231861-3.patch3.86 KBLendude
#3 interdiff-3231861-2-3.txt817 bytesLendude
#2 3231861-2.patch4.06 KBLendude
#2 3231861-2-TEST_ONLY.patch2.57 KBLendude
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marksmith created an issue. See original summary.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented
Title: Disable SQL rewriting only works if applied to all views » PHP errors when overriding the query settings
Version: 9.2.x-dev » 9.3.x-dev
Status: Active » Needs review
FileSize
3231861-2-TEST_ONLY.patch2.57 KB
3231861-2.patch4.06 KB

@marksmith thanks for reporting this, it is very easy to reproduce. But as far as I can tell it has nothing to do with SQL rewrite. You can just set the display to overridden and not change any settings, and errors will be shown.

As far as I can tell this is because of the handling of the query_tags field, which falls back to an empty array when overriding while all the logic expects a string. Here is a fix and a test for this.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented
FileSize
interdiff-3231861-2-3.txt817 bytes
3231861-3.patch3.86 KB

Bit of a clean up for the test which had some copy/pasta in there.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented
FileSize
3231861-3-TEST_ONLY.patch2.37 KB
3231861-3.patch3.86 KB

Now with a test only patch that works

The last submitted patch, 4: 3231861-3-TEST_ONLY.patch, failed testing. View results

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Status: Needs review » Reviewed & tested by the community

- Failing test fails with the error described in the IS
- Patch comes back green
- Added code (except for the test) checks if options['query_tags'] is an array, if so, it uses explode/implode on options['query_tags'], otherwise it uses options['query_tags'] itself.

RTBC for me.

GuyPaddock’s picture

GuyPaddock CreditAttribution: GuyPaddock at Inveniem commented
Status: Reviewed & tested by the community » Needs work

Why doesn't the method simply return early if it detects that the value is an array? There's no need to run it back through filtering and setting again in the form state.

I proposed a slight variation on this for #3232745: Manually-added Query Tags in "Query Settings" are Lost when Changing Override Status of Query Options for any Display. Would be good if you could add a comment as to why it might be an array, for future devs to understand what's happening.

For reference, my changes for the duplicate issue:
https://git.drupalcode.org/project/drupal/-/merge_requests/1190/diffs

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented
Status: Needs work » Needs review
FileSize
interdiff-3231861-3-8.txt967 bytes
3231861-8.patch4.09 KB

@GuyPaddock thanks for the review! Took your comment and fix from #3232745: Manually-added Query Tags in "Query Settings" are Lost when Changing Override Status of Query Options for any Display and modified slightly.

mstrelan’s picture

mstrelan CreditAttribution: mstrelan at PreviousNext commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I've reviewed the patch, tested locally and updated the issue summary. Updating status to RTBC.

GuyPaddock’s picture

GuyPaddock CreditAttribution: GuyPaddock at Inveniem commented
Status: Reviewed & tested by the community » Needs work

Thanks, @lendude!

Question, though -- why is the change to buildOptionsForm() necessary? I can't envision any reason why $this->options['query_tags'] should be something other than an array since the submit handler now guarantees it.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented
Status: Needs work » Needs review
FileSize
interdiff-3231861-8-11.txt790 bytes
3231861-11.patch3.46 KB

@GuyPaddock you are right, that is no longer needed, took it out and now the test stays green.

GuyPaddock’s picture

GuyPaddock CreditAttribution: GuyPaddock at Inveniem commented

LGTM!

GuyPaddock’s picture

GuyPaddock CreditAttribution: GuyPaddock at Inveniem commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 32eb238f75 to 9.3.x and 156c4c17e5 to 9.2.x. Thanks!

  • alexpott committed 32eb238 on 9.3.x 
    Issue #3231861 by Lendude, GuyPaddock: PHP errors when overriding the...

  • alexpott committed 156c4c1 on 9.2.x 
    Issue #3231861 by Lendude, GuyPaddock: PHP errors when overriding the...

Status: Fixed » Closed (fixed)

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