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.

Comments

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde’s picture

achton’s picture

Issue tags: +Needs tests
FileSize
950 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

Status: Active » Needs work
achton’s picture

FileSize
1 KB

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

dawehner’s picture

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

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

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

Status: Needs work » Needs review

Just kicking the testbot into motion for #5

dawehner’s picture

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

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

AdamPS’s picture

(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’s picture

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

@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

Status: Needs work » Needs review
FileSize
4.63 KB
5.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

Awesome, starting to get the hang of this :)

dawehner’s picture

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

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

dawehner’s picture

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

Good point!