Problem/Motivation

views_embed_view() is passing arguments to the preview() method with wrong array keys which causes them to be mishandled in setArguments. views_embed_view() is never used in core, and doesn't currently have any tests.

Proposed resolution

In the setArguments use array_values to reset array keys.

Remaining tasks

-Write tests
-Write patch
-Manual testing

User interface changes

-

API changes

-

CommentFileSizeAuthor
#78 Issue_2208811-updated.patch8.03 KBPol
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,216 pass(es). View
#78 interdiff.txt550 bytesPol
#77 tests-Issue_2208811.patch7.36 KBPol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,929 pass(es), 8 fail(s), and 0 exception(s). View
#77 Issue_2208811-new.patch8.01 KBPol
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,941 pass(es). View
#76 Issue_2208811-new.patch8.06 KBPol
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,942 pass(es). View
#71 Issue_2208811.patch4.74 KBPol
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,922 pass(es). View
#69 Issue_2208811.patch2.34 KBPol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,911 pass(es), 2 fail(s), and 2 exception(s). View
#65 Issue_2208811.patch4.63 KBPol
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,637 pass(es). View
#64 Issue_2208811.patch124.29 KBPol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch Issue_2208811_0.patch. Unable to apply patch. See the log in the details link for more information. View
#61 Issue_2208811.patch4.63 KBPol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,412 pass(es), 1 fail(s), and 0 exception(s). View
#58 2208811_fix_views_embed_data_58.patch4.68 KBwizonesolutions
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,325 pass(es), 33 fail(s), and 1 exception(s). View
#50 interdiff-2208811-40-49.txt1.54 KBPol
#49 220881-fixed-views-with-arguments-stopped-working.patch5.37 KBPol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,688 pass(es), 33 fail(s), and 1 exception(s). View
#40 220881-fixed-views-with-arguments-stopped-working.patch4.72 KBPol
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,664 pass(es). View
#34 220881-fixed-views-with-arguments-stopped-working-FAIL.patch1.18 KBPol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,626 pass(es), 2 fail(s), and 3 exception(s). View
#25 220881-fixed-views-with-arguments-stopped-working.patch4.03 KBPol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,876 pass(es), 0 fail(s), and 1 exception(s). View
#18 vdc-2208811-18.patch4.17 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,487 pass(es). View
#15 arguments-unix.patch430 bytesivanjaros
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,461 pass(es). View
#12 arguments.patch442 bytesivanjaros
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid patch format in arguments_1.patch. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner’s picture

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

ivanjaros’s picture

dawehner’s picture

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.

ivanjaros’s picture

There is nothing wrong with the PHP code. It worked in previous alpha releases and stopped working in A9. There were no changes made to the php code or the view itself.

Tomorrow I'll try to create other views and embed them so see what will happen.

dawehner’s picture

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

ivanjaros’s picture

So I have created a completly new View that would display a node. I've added a NID contextual filter and again, it works in the preview mode on the Views form but not when embeded via views_embed_view.

ivanjaros’s picture

This will display "no results" text.

$view = views_embed_view('myview', 'block_1', 1);
$render = render($view);
dsm($render);

This will actually display the view with results.

$view = views_get_view('myview');
$preview = $view->preview('block_1', array(1));
$render = render($preview);
dsm($render);

The views_embed_view function is:

function views_embed_view($name, $display_id = 'default') {
  $args = func_get_args();
  // Remove $name and $display_id from the arguments.
  unset($args[0], $args[1]);

  $view = views_get_view($name);
  if (!$view || !$view->access($display_id)) {
    return;
  }

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

I would tihnk that the issue lies with the access but then the function would return nothing.

longwave’s picture

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

So I have found out that the issue lies with the arguments themselves.
If \Drupal\views\ViewExecutable::preview() receives arguments as array(0 => 1) it will display my view properly, but if it receives arguments like this array(2 => 1) then the "no results" text will show up.

The 1 is the argument I am providing to the view. The array key is 2 because views_embed_view() takes function arguments via func_get_args() and unsets keys 0 and 1 which are View name and display id and hands them over to the preview method.

I havent looked yet but it seems the issue can originate from \Drupal\views\ViewExecutable::_buildArguments.

On the other hand this can be easily fixed by add ing $args = array_values($args); to the views_embed_view function.

function views_embed_view($name, $display_id = 'default') {
  $args = func_get_args();
  // Remove $name and $display_id from the arguments.
  unset($args[0], $args[1]);

  $view = views_get_view($name);
  if (!$view || !$view->access($display_id)) {
    return;
  }
  $args = array_values($args);
  return $view->preview($display_id, $args);
}
dawehner’s picture

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

ivanjaros’s picture

I'm sure _buildArguments() has it's reason why it works with arguments in such way and to answer your question @dawehner yes, I tihnk setArguments would be a good place to fix this but first we'd need to see if any code that uses it does not depend on the arguments array keys(which I don't think so). Testt should quickly find this out though.

ivanjaros’s picture

FileSize
442 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid patch format in arguments_1.patch. View

Lets see if it'll pass through tests.

dawehner’s picture

Status: Active » Needs review

.

Status: Needs review » Needs work

The last submitted patch, 12: arguments.patch, failed testing.

ivanjaros’s picture

FileSize
430 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,461 pass(es). View
ivanjaros’s picture

Status: Needs work » Needs review
dawehner’s picture

Issue tags: +Needs tests

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

dawehner’s picture

FileSize
4.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,487 pass(es). View

@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.

ivanjaros’s picture

It's working on back-end because in Views edit form the \Drupal\views\ViewExecutable::preview() is called directly whereas on the front-end the views_embed_view is used. And that is the place where the argument keys get messed up.

ivanjaros’s picture

As you can see https://api.drupal.org/api/views/views.module/function/views_embed_view/7
previously the array_shift() was used to remove view name and display id which reset the keys. That is the core of the problem.

dawehner’s picture

Status: Needs review » Postponed (maintainer needs more info)

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

matslats’s picture

Yes quick solution is, in views_embed_view()

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

Pol’s picture

Status: Postponed (maintainer needs more info) » Active

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

dawehner’s picture

Issue tags: +VDC

...

Pol’s picture

Status: Active » Needs review
FileSize
4.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,876 pass(es), 0 fail(s), and 1 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 25: 220881-fixed-views-with-arguments-stopped-working.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review

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 ?

Status: Needs review » Needs work

The last submitted patch, 25: 220881-fixed-views-with-arguments-stopped-working.patch, failed testing.

Berdir’s picture

Version: 8.0-alpha9 » 8.0.x-dev

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 25: 220881-fixed-views-with-arguments-stopped-working.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
4.72 KB

New patch.

Pol’s picture

Version: 8.0.x-dev » 8.0-alpha9
FileSize
1.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,626 pass(es), 2 fail(s), and 3 exception(s). View

First attempt to create a .FAIL patch.

Pol’s picture

Version: 8.0-alpha9 » 8.0.x-dev

Status: Needs review » Needs work

The last submitted patch, 34: 220881-fixed-views-with-arguments-stopped-working-FAIL.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 34: 220881-fixed-views-with-arguments-stopped-working-FAIL.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
4.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,664 pass(es). View

And the patch.

webflo’s picture

+++ 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.

Pol’s picture

It's up to @dawehner now...

dawehner’s picture

Status: Needs review » Needs work

Seems to be a crosspost

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed c55ac96 on 8.0.x
    Issue #2208811 by Pol, ivanjaros, dawehner: Fixed Views with arguments...

  • alexpott committed b68e063 on 8.0.x
    Revert "Issue #2208811 by Pol, ivanjaros, dawehner: Fixed Views with...
alexpott’s picture

Status: Fixed » Needs work

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

Status: Needs work » Needs review
FileSize
5.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,688 pass(es), 33 fail(s), and 1 exception(s). View

Damn, sorry Alex.

Here's the new patch including your recommandations.

Pol’s picture

FileSize
1.54 KB

Here's the interdiff.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

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

Berdir’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

dawehner’s picture

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 :)

lauriii’s picture

Title: Views with arguments stopped working on front-end » views_embed_view() cannot handle arguments
Issue summary: View changes

The last submitted patch, 49: 220881-fixed-views-with-arguments-stopped-working.patch, failed testing.

wizonesolutions’s picture

Issue tags: +drupaldevdays

working on a re-roll

wizonesolutions’s picture

FileSize
4.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,325 pass(es), 33 fail(s), and 1 exception(s). View

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.

dawehner’s picture

Status: Needs work » Needs review

.

Status: Needs review » Needs work

The last submitted patch, 58: 2208811_fix_views_embed_data_58.patch, failed testing.

Pol’s picture

FileSize
4.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,412 pass(es), 1 fail(s), and 0 exception(s). View

Here's the updated patch with the view fixed.

lauriii’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 61: Issue_2208811.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
124.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch Issue_2208811_0.patch. Unable to apply patch. See the log in the details link for more information. View

Updated patch.

Pol’s picture

FileSize
4.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,637 pass(es). View

Sorry wrong patch, here's the good one.

The last submitted patch, 64: Issue_2208811.patch, failed testing.

Pol queued 65: Issue_2208811.patch for re-testing.

Pol’s picture

Issue summary: View changes
Pol’s picture

FileSize
2.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,911 pass(es), 2 fail(s), and 2 exception(s). View

Incomplete patch, forget it.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

Pol’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,922 pass(es). View

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

Pol’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Re-RTBC

The last submitted patch, 69: Issue_2208811.patch, failed testing.

Pol’s picture

Status: Reviewed & tested by the community » Needs review

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);
}
Pol’s picture

FileSize
8.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,942 pass(es). View

Finally,

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

Let me know what you think.

Pol’s picture

FileSize
8.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,941 pass(es). View
7.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,929 pass(es), 8 fail(s), and 0 exception(s). View

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.

Pol’s picture

FileSize
550 bytes
8.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,216 pass(es). View

Add method namespace in comment.

The last submitted patch, 77: tests-Issue_2208811.patch, failed testing.

Pol’s picture

Status: Needs review » Reviewed & tested by the community
Pol’s picture

Status: Reviewed & tested by the community » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Looks great for me!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 78: Issue_2208811-updated.patch, failed testing.

Status: Needs work » Needs review

Pol queued 78: Issue_2208811-updated.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 78: Issue_2208811-updated.patch, failed testing.

Status: Needs work » Needs review

Pol queued 78: Issue_2208811-updated.patch for re-testing.

Pol’s picture

Status: Needs review » Reviewed & tested by the community

The patch has failed because of the test bot.

Putting this patch back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed 40d6c14 on 8.0.x
    Issue #2208811 by Pol, ivanjaros, dawehner, wizonesolutions:...
ivanjaros’s picture

Finally.

Status: Fixed » Closed (fixed)

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