views_embed_view() cannot handle arguments

How do you "embed" a view on your site?

Ensure that you at least provide the php code you use and other stuff.

Just imagine that we don't know what you are doing.

Thank you for sharing so many information: the view itself, the php code and thank you for not using the git version.

I noticed in #2208893: Remove unused functions from Views that views_embed_view() is never used in core, and doesn't currently have any test coverage, so it's not really surprising if it's accidentally been broken at some point.

@ivanjaros. I wonder whether the same should be applied on a lower level, like on ViewExecutable::setArguments. Do you have any opinion about that?

.

It would be great to have some dedicated test coverage here as well.

@ivanjaros
Can you please explain me why views_embed_view for itself works pretty fine? I asked you multiple ****** times to show the code.

Well, at least we now have views_embed_view test coverage.

I asked you multiple ****** times to show the code.

Yes quick solution is, in views_embed_view()

- return $view->preview($display_id, $args);
+ return $view->preview($display_id, array_values($args));

I can also confirm that the patch fixes the problem.

I hope this get committed soon, this bug prevent my new D8 module to work properly.

See: http://cgit.drupalcode.org/views_field_formatter/tree/src/Plugin/Field/F...

Module: https://www.drupal.org/project/views_field_formatter

...

Updated patch for d8 head.

If you want to see the problem, insert a print_r($args); in the setArguments method then run the test with drush: drush test-run 'Drupal\views\Tests\ModuleTest' --full.

The patch applies correctly on my instance...

pol@x230 ~/git/drupal8 $ git checkout 8.0.x
Switched to branch '8.0.x'
Your branch is up-to-date with 'origin/8.0.x'.
pol@x230 ~/git/drupal8 $ git pull
Already up-to-date.
pol@x230 ~/git/drupal8 $ git apply 220881-fixed-views-with-arguments-stopped-working.patch 
pol@x230 ~/git/drupal8 $

Any idea ?

New patch.

First attempt to create a .FAIL patch.

And the patch.

+++ b/core/modules/views/src/Tests/ModuleTest.php
--- a/core/modules/views/src/Tests/ViewUnitTestBase.php
+++ b/core/modules/views/src/Tests/ViewUnitTestBase.php

+++ b/core/modules/views/src/Tests/ViewUnitTestBase.php
@@ -224,7 +224,7 @@ protected function orderResultSet($result_set, $column, $reverse = FALSE) {
-    $view->preExecute($args);
+    $view->preExecute((array) $args);

That's not necessary, we convert the array in setArguments() anyway.

It's up to @dawehner now...

Seems to be a crosspost

Yeah there is no reason to stop the patch based upon that.

Committed c55ac96 and pushed to 8.0.x. Thanks!

+++ b/core/modules/views/src/Tests/ViewUnitTestBase.php
@@ -224,7 +224,7 @@ protected function orderResultSet($result_set, $column, $reverse = FALSE) {
   protected function executeView($view, $args = array()) {
     $view->setDisplay();
-    $view->preExecute($args);
+    $view->preExecute((array) $args);
     $view->execute();
     $verbose_message = '<pre>Executed view: ' . ((string) $view->build_info['query']). '</pre>';
     if ($view->build_info['query'] instanceof SelectInterface) {

I removed this unnecessary change on commit.

Lol well doing that broke HEAD.

This patch should also fix up the expectations of executeView by doing something like:

diff --git a/core/modules/entity_reference/src/Tests/Views/EntityReferenceRelationshipTest.php b/core/modules/entity_reference/src/Tests/Views/EntityReferenceRelationshipTest.php
index da87544..1d5d9ee 100644
--- a/core/modules/entity_reference/src/Tests/Views/EntityReferenceRelationshipTest.php
+++ b/core/modules/entity_reference/src/Tests/Views/EntityReferenceRelationshipTest.php
@@ -137,7 +137,7 @@ public function testRelationship() {
     }
 
     $view->destroy();
-    $this->executeView($view, 'embed_1');
+    $this->executeView($view, array('embed_1'));
 
     foreach (array_keys($view->result) as $index) {
       $this->assertEqual($view->result[$index]->id, $this->entities[$index + 1]->id());
diff --git a/core/modules/views/src/Tests/ViewUnitTestBase.php b/core/modules/views/src/Tests/ViewUnitTestBase.php
index 0d07484..ba65df4 100644
--- a/core/modules/views/src/Tests/ViewUnitTestBase.php
+++ b/core/modules/views/src/Tests/ViewUnitTestBase.php
@@ -222,7 +222,7 @@ protected function orderResultSet($result_set, $column, $reverse = FALSE) {
    * @param array $args
    *   (optional) An array of the view arguments to use for the view.
    */
-  protected function executeView($view, $args = array()) {
+  protected function executeView($view, array $args = array()) {
     $view->setDisplay();
     $view->preExecute($args);
     $view->execute();

Damn, sorry Alex.

Here's the new patch including your recommandations.

Here's the interdiff.

Thanks for addressing the concerns outlined in #48. This looked good before it was rtbc and it does again now.

+++ b/core/modules/entity_reference/src/Tests/Views/EntityReferenceRelationshipTest.php
@@ -137,7 +137,7 @@ public function testRelationship() {
     $view->destroy();
-    $this->executeView($view, 'embed_1');
+    $this->executeView($view, array('embed_1'));
 

This doesn't look like an argument to me but a display id, which is not supposed to be passed in like this?

That should be done with $view->setDisplay('embed_1') first, and then no argument to executeView()

Also, issue title and summary could really use an update to describe the actual bug, which is only documented in one of the comments above.

That should be done with $view->setDisplay('embed_1') first, and then no argument to executeView()

True, this never worked and also the fix can't work properly :)

working on a re-roll

I talked with @Pol about this on IRC and tried digging in myself, but it's hopeless. I can't figure out the best way to debug the test View because it assumes that the views_test_data table has data in it. But this is only populated in the context of the test. Not sure how to debug the View via the UI. Seems to have properties that aren't defined in the configuration schema, and the testing system doesn't like this.

The entity reference test is fixed though, so this is what I have so far. Not including interdiff because it's the same other than not having the EntityReference test changes.

.

Here's the updated patch with the view fixed.

Updated patch.

Sorry wrong patch, here's the good one.

Incomplete patch, forget it.

Thank you!

Sorry for the previous comment, the patch is incomplete. This patch is ok.

Re-RTBC

On IRC Alexpott raised that the \Drupal\views\ViewExecutable:preview() method is not tested and asked me to rephrase the comment in the \Drupal\views\ViewExecutable:setArguments() method.

Let's follow the workflow of views_embed_view() function:

$my_view = views_embed_view('my_view', 'default', 'arg1', 'arg2', 'arg3');

As the arguments number is variable, views_embed_view() will use func_get_args() to extract them, it will also remove the name and display_id, the two first arguments of the function.

Before calling the $view->preview() method, if we do a print_r on the $args variable, we get this:

Array
(
    [2] => arg1
    [3] => arg2
    [4] => arg3
)

Then we call $view->preview().
There, the $args variable is untouched and passed to ViewExecutable->preview(), let's see what happens there:

if ($args) {
  $this->setArguments($args);
}

The ViewExecutable->setArguments() method is:

public function setArguments($args) {
  $this->args = $args;
}

My patch change these line in:

public function setArguments(array $args) {
  $this->args = array_values($args);
}

So, we are sure that the resulting array has the correct keys, like this:

Array
(
    [0] => arg1
    [1] => arg2
    [2] => arg3
)

The method $view->preview() method is also fixed using this patch.

As comment for setArguments(), I'm proposing this comment:

public function setArguments(array $args) {
  // The array keys of the arguments will be incorrect if set by views_embed_view() or preview().
  // This will reindex the array to match expectations.
  $this->args = array_values($args);
}

Finally,

Here's a much better version of the patch with updated tests.

Let me know what you think.

Here's a new patch with an updated comment in the \Drupal\views\ViewExecutable:setArguments() method.

I also added a patch containing only the tests.

Add method namespace in comment.

Looks great for me!

The patch has failed because of the test bot.

Putting this patch back to RTBC.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 40d6c14 and pushed to 8.0.x. Thanks!