Views allows magic query arguments to be used and translates them when the query is run so you can cache the original query, but keep the functionality of adding in the current user's ID for example.

The problem is it does not account for IN queries who have an array of values in their arguments entry. This causes views_query_views_alter() to try and use an array as an array key in the following piece of code:

// Replaces substitutions in tables.
foreach ($tables as $table_name => $table_metadata) {
  foreach ($table_metadata['arguments'] as $replacement_key => $value) {
    if (isset($substitutions[$value])) { // Value is an array for IN-queries!
      $tables[$table_name]['arguments'][$replacement_key] = $substitutions[$value];
    }
  }
}

Proposed fix:

// Replaces substitutions in tables.
foreach ($tables as $table_name => $table_metadata) {
  foreach ($table_metadata['arguments'] as $replacement_key => $value) {
    if (!is_array($value)) {
      if (isset($substitutions[$value])) {
        $tables[$table_name]['arguments'][$replacement_key] = $substitutions[$value];
      }
    }
    else {
      foreach ($value as $sub_key => $sub_value) {
        if (isset($substitutions[$sub_value])) {
          $tables[$table_name]['arguments'][$replacement_key][$sub_key] = $substitutions[$sub_value];
        }
      }
    }
  }
}

Although the patch should obviously also contain a test (or amended test) to show that this breaks on any view that is tagged with 'views' and is using an IN condition.

CommentFileSizeAuthor
#22 drupal-2744069-22.patch5.81 KBkristiaanvandeneynde
#22 interdiff-18-22.txt543 byteskristiaanvandeneynde
#18 drupal-2744069-18.patch5.8 KBkristiaanvandeneynde
#18 drupal-2744069-18-TEST_ONLY.patch4.63 KBkristiaanvandeneynde
#5 2744069-5-views-in-queries.patch1 KBachton
#3 2744069-3-views-in-queries.patch950 bytesachton
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
Related issues: +#2658438: Array valued 'extra' values generate invalid SQL in Views joins

Spin-off from #2658438-34: Array valued 'extra' values generate invalid SQL in Views joins

achton’s picture

achton
Location Denmark
CreditAttribution: achton at Reload commented
Issue tags: +Needs tests
FileSize
2744069-3-views-in-queries.patch950 bytes

I hope this is not totally unwelcome, but here is a patch containing that exact change, minus tests.

I had the exact same problem with Views and Group over in #2779801: Php Warning on group/{groupid}/nodes when multiple content plugins are installed and found the same solution as here, so I am adding this patch to start with, in order to ease up local testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Active » Needs work
achton’s picture

achton
Location Denmark
CreditAttribution: achton at Reload commented
FileSize
2744069-5-views-in-queries.patch1 KB

The patch in #3 does not apply cleanly, this one does.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

I've just seen this on production, really annoying :)

Feel free to ping me on any communication way, when you need some way of help with those tests.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Perhaps this could be taken care of by a few lines of code in /core/modules/views/tests/src/Kernel/Plugin/JoinTest.php. That's the test covering the issue where this bug was uncovered.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

I would be fine with that :)

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Noctilaris commented
Status: Needs work » Needs review

Just kicking the testbot into motion for #5

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Needs work

Needs work for writing some tests :)

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.

johnreytanquinco’s picture

johnreytanquinco CreditAttribution: johnreytanquinco commented

I have the same problem when viewing the groups. Patch #5 works for me. Thanks! :D

AdamPS’s picture

AdamPS CreditAttribution: AdamPS commented
Issue tags: +Contributed project blocker

(Edited to fix linked issue, thanks to @kristiaanvandeneynde #15 for pointing out.)

This bug is blocking Administer Users by Role, see #2853289: Warning: Illegal offset type in isset or empty in views_query_views_alter(). Patch #5 solves.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

I think AdamPS meant to link to #2853289: Warning: Illegal offset type in isset or empty in views_query_views_alter().

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Well this just broke the Group module tests in a funny way: #2867710-6: Move storage related group content functionality to the GroupContent storage

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

@dawehner: Searching the code base for 'views_substitutions' it seems there is zero test coverage for argument substitution right now. Where would this best be added? My previous suggestion of JoinTest is definitely wrong in this case.

Edit: Or maybe not, I mean I could easily break said test in a way that the attached patch fixes it. But that wouldn't be a pure test.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
Status: Needs work » Needs review
FileSize
drupal-2744069-18-TEST_ONLY.patch4.63 KB
drupal-2744069-18.patch5.8 KB

Attached is a patch which fixes the problem as proposed in the issue summary and also adds a test that tests the basic relationship handler when an IN-condition is added to the join.

The last submitted patch, 18: drupal-2744069-18-TEST_ONLY.patch, failed testing.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Awesome, starting to get the hang of this :)

dawehner’s picture

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

Great test coverage!

  1. +++ b/core/modules/views/tests/src/Kernel/Plugin/RelationshipJoinInTest.php
@@ -0,0 +1,129 @@
+class RelationshipJoinInTest extends RelationshipJoinTestBase {
+  use UserCreationTrait;

    Nitpick of the month: I believe there should be a new line here :P 

  2. +++ b/core/modules/views/tests/src/Kernel/Plugin/RelationshipJoinInTest.php
@@ -0,0 +1,129 @@
+  /**
+   * Tests the query result of a view with a relationship with an IN condition.
+   */
+  public function testRelationshipInQuery() {

    I think its not ideal to have the test coverage in here, but its a fair compromise for now. We also add some test coverage for other real world problems.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
Issue tags: -Needs tests
FileSize
interdiff-18-22.txt543 bytes
drupal-2744069-22.patch5.81 KB

Added the space. Also agree about the test coverage. At least it makes sure we can't have a regression in relationship handlers.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Issue credit should also include ohthehugemanatee, see #2871712: Warning: Illegal offset type in isset or empty in views_query_views_alter()

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

At least it makes sure we can't have a regression in relationship handlers.

Good point!

  • catch committed 963f01f on 8.4.x 
    Issue #2744069 by kristiaanvandeneynde, achton, dawehner,...

catch credited ohthehugemanatee.

catch credited ohthehugemanatee.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

  • catch committed 555c167 on 8.3.x 
    Issue #2744069 by kristiaanvandeneynde, achton, dawehner,...
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Sweet, thanks! Going to see far fewer bug reports about this in the Group module issue queue now :)

Status: Fixed » Closed (fixed)

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