Array valued 'extra' values generate invalid SQL in Views joins

+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
@@ -333,7 +331,13 @@ public function buildJoin($select_query, $table, $view_query) {
+            $extras[] = "$join_table_field $operator $placeholder";          ¶

extra whitespace.

Also, can you please post a "test only" patch so that we can see the failure (with the new assert) for review purposes?

Reviewed and tested this as part of the patch in #2706495-16: add Views reverse relationships from entities to Group Content.

Looks good, does what it promises and has green tests. RTBC.

error: core/modules/views/src/Tests/Plugin/JoinTest.php: does not exist in index

Looks like the test has been moved.

Straight reroll, so if tests go green this goes back to RTBC.

The test moved into a Kernel subfolder is all.

The tests were moved as part of #2553655: Convert ViewKernelTestBase to use KernelTestBaseTNG

Green so back to RTBC.

This looks great for me!

1. +++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
   @@ -303,19 +303,17 @@ public function buildJoin($select_query, $table, $view_query) {
                $local_arguments = array();
   ...
   +            $local_arguments[$placeholder] = $info['value'];
   ...
                $arguments += $local_arguments;
   

   I think we can do away with the $local_arguments completely. Ie I think we can just do
   $arguments[$placeholder] = $info['value'];.

2. +++ b/core/modules/views/tests/src/Kernel/Plugin/JoinTest.php
   @@ -172,7 +172,7 @@ public function testBasePlugin() {
   +    $this->assertTrue(strpos($join_info['condition'], "users4.name IN ( :views_join_condition_3[] )") !== FALSE, 'The IN condition for the join is properly formed.');	 ¶
   
   @@ -197,8 +197,8 @@ public function testBasePlugin() {
   +    $this->assertTrue(strpos($join_info['condition'], "views_test_data.status = :views_join_condition_5") !== FALSE, 'Make sure the second extra join condition appears in the query.');		    ¶
   

   Space at the end of the line.

I think we can do away with the $local_arguments completely. Ie I think we can just do
$arguments[$placeholder] = $info['value'];.

I think so, $placeholder is unique.

Both good points. Looking at the code it is indeed now possible to set the value directly.

Nice!

+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
@@ -303,19 +303,15 @@ public function buildJoin($select_query, $table, $view_query) {
+          $use_parenthesis = FALSE;
...
+            $placeholder = ':views_join_condition_' . $select_query->nextPlaceholder() . '[]';
...
-            $placeholder = '( ' . implode(', ', array_keys($local_arguments)) . ' )';
...
+            $use_parenthesis = TRUE;

@@ -340,7 +336,13 @@ public function buildJoin($select_query, $table, $view_query) {
+          if ($use_parenthesis) {
+            $extras[] = "$join_table_field $operator ( $placeholder )";
+          }
+          else {
+            $extras[] = "$join_table_field $operator $placeholder";
+          }

Is there any reason we don't do as before and add the parenthesis into the placeholder? I can't see one and this would mean there is less logic in the method. As the whole if ($use_parenthesis) { change would be unnecessary.

Sorry I should have asked this in earlier reviews.

Nah, good point. I left the comment in there, as it doesn't hurt to have some in such a bloated function :)

If you prefer the old IN(...) SQL syntax, there is probably a very short fix for the problem.

@@ -331,7 +331,7 @@ class JoinPluginBase extends PluginBase
             if (isset($info['left_field'])) {
               $placeholder = "$left[alias].$info[left_field]";
             }
-            else {
+            elseif (!is_array($info['value'])) {
               $arguments[$placeholder] = $info['value'];
             }
           }

If you prefer the IN(...) SQL syntax, there is probably a very short fix for the problem.

@@ -331,7 +331,7 @@ class JoinPluginBase extends PluginBase
             if (isset($info['left_field'])) {
               $placeholder = "$left[alias].$info[left_field]";
             }
-            else {
+            elseif (!is_array($info['value'])) {
               $arguments[$placeholder] = $info['value'];
             }
           }

I'm not sure I know what you're getting at dku. The issue is about the joins not working at all because the plugin never incorporated the logic from the security advisory and the latest patch fixes that.

@dku
This really sounds like a different problem, maybe reporting this in its own issue would be great!

Nice find @alexpott, this simplifies the code a bit!

Actually, the parenthesis change lead to this patch breaking when used because the arguments array now has a key containing parentheses whereas Drupal looks for the placeholder without parentheses.

Reverting to the old patch from #20 and leaving as RTBC again because that one was properly reviewed and worked in #2706495: add Views reverse relationships from entities to Group Content, whereas the latest one doesn't.

Actually, back to needs work :(

It throws notices in this part of views_query_views_alter():

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

$value is an array so invalid for use as an array key in the isset() call. Any suggestions on how to address that one? It's the final piece of this puzzle for it to be completed :)

How about a much simpler approach?

We refactor views_query_views_alter() to account for $value potentially being an array and then loop over that. That way, a join condition of WHERE foo IN ( :bar[] ) with :bar expanding to ***CURRENT_TIME*** and ***CURRENT_VERSION*** would also work as expected.

Something like this:

// 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];
        }
      }
    }
  }
}

The funny part is: This is broken for all of Views, not just the functionality in this patch. Looking at the original code, there is no way an IN-query of any kind would have ever properly replaced its substitution keys with the actual values. So perhaps we should ignore it here and file a separate issue for it?

So, as per #29 we can go back to RTBC for the patch in #20. As per #34, a follow-up should be created for views_query_views_alter() because it (should throw/throws) notices for more than just this issue.

+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
@@ -303,19 +303,15 @@ public function buildJoin($select_query, $table, $view_query) {
+            $placeholder = ':views_join_condition_' . $select_query->nextPlaceholder() . '[]';
             $operator = !empty($info['operator']) ? $info['operator'] : 'IN';
-            $placeholder = '( ' . implode(', ', array_keys($local_arguments)) . ' )';
-            $arguments += $local_arguments;
+            $arguments[$placeholder] = $info['value'];
+            $use_parenthesis = TRUE;

This could still be written as

  $placeholder = ':views_join_condition_' . $select_query->nextPlaceholder() . '[]';
  $operator = !empty($info['operator']) ? $info['operator'] : 'IN';
  $arguments[$placeholder] = $info['value'];
  $placeholder = '( ' . $placeholder . ' )';

Which would rid us of the conditional and $use_parenthesis.

We refactor views_query_views_alter() to account for $value potentially being an array and then loop over that. That way, a join condition of WHERE foo IN ( :bar[] ) with :bar expanding to ***CURRENT_TIME*** and ***CURRENT_VERSION*** would also work as expected.

That sounds sensible

So the $placeholder variable was both used for the array key in $arguments and the SQL string all the way at the end. Because there was some extra code later on, we can't simply encapsulate $placeholder in parentheses because we run the risk of the arguments array having the wrong key again.

The attached patch solves this by introducing a second $placeholder_sql variable which is only used in the generated SQL, which means we can safely keep using $placeholder without parentheses. I also added a test which checks for the arguments key to be a correct placeholder.

Edit: will post a patch saying $placeholder_sql instead of $place_holder_sql after this one goes green.

Tests are currently failing on 8.1.x due to a B/C break in #2735045: Convert StandardTest to BrowserTestBase by introducing a AssertLegacyTrait and new assertions on the WebAssert so flagging this issue as 8.1.2 and uploading the patch from #38 without the typo and with a slightly cleaner variable assignment.

Also created a spin-off for the warnings in the logs at #2744069: views_query_views_alter() does not handle IN queries

JoinTest actually passed in that test, so the tests we want to go green went green.

This looks perfect for me now. The changes are fairly minimal, we have some test.

Committed 4bab1f7 and pushed to 8.1.x and 8.2.x. Thanks!