If view display have header / footer with other this view displays, calculateDependencies() method adds to dependencies this view.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Good catch!!! Do you think we should write a test here?

devpreview’s picture

I tried to write the tests but I don't understand how to write them.

Sorry.

Status: Needs review » Needs work

The last submitted patch, 2: recursion_calculate_dependencies_tests.patch, failed testing.

dawehner’s picture

So what you would do is to write some code which calls out to

$this->viewHandler->calculateDependencies();

This itself might try to load the view itself, see $this->viewStorage->load($view_id)

In order for that to work, you have to mock the behaviour of the entity storage:

$view0 = $this->getMock('Drupal\views\ViewEntityInterface');
$view1 = $this->getMock('Drupal\views\ViewEntityInterface');

$this->entityStorage->expects($this->any())
  ->method('load')
  ->willReturnMap([
    ['id_0', $view0],
    ['id_1', $view1],
  ])

Feel free to ping me in IRC.

devpreview’s picture

Patch with tests.

Thanks for help from IRC!

devpreview’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +VDC

Awesome work

+++ b/core/modules/views/src/Plugin/views/area/View.php
@@ -158,7 +158,7 @@ public function calculateDependencies() {
-    if ($view_id) {
+    if ($view_id && $this->view->storage->id() != $view_id) {

It would be great to add a one-line documentation explaining why we need this check here.

devpreview’s picture

Sorry, I do not speak English.
This comment will be correct?

Check if exist view to insert and this view is not current view

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs beta evaluation

Maybe try something like: // Ensure to not call the current view, as it would result into a infinite recursion.

devpreview’s picture

Then

Ensure to exists insert view id and not call the current view, as it would result into a infinite recursion.

"exists insert view id" for old "if ($view_id)" condition.

devpreview’s picture

FileSize
3.48 KB

Updated one-line documentation

dawehner’s picture

Sorry for the endless nitpicking here ... i think its pointless to describe what the code is doing but rather we should describe why (which is doing the second part of the sentence).

+++ b/core/modules/views/src/Plugin/views/area/View.php
@@ -158,7 +158,9 @@ public function calculateDependencies() {
+    // Ensure to exists insert view id and not call the current view,
+    // as it would result into a infinite recursion.

I would vote for: "Don't call the current view, as it would result into an infinite recursion."

devpreview’s picture

FileSize
3.44 KB

Thank you for your help!
I don't think it's nitpicking since it makes the code better :)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Great catch and nice patch. 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 0943b23 and pushed to 8.0.x. Thanks!

  • alexpott committed 0943b23 on 8.0.x
    Issue #2442221 by devpreview: Recursion calculate dependencies in area...

Status: Fixed » Closed (fixed)

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