Problem/Motivation

If you embed a view using the #type => 'view' exposed forms as skipped.

Proposed resolution

Don't alter the root level render array, but rather add one below it, just like the non embed case does.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 2549719-8-fail.patch3.01 KBdawehner
#7 interdiff.txt3.01 KBdawehner
#7 2549719-7.patch3.68 KBdawehner
#3 2549719-1.patch684 bytesdawehner

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
Issue tags: +Needs issue summary update

There we go,

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2549719-1.patch684 bytes

This time with a patch.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Normal » Major
Issue tags: +VDC, +D8 cacheability 
dawehner: WimLeers: so it lost the cache metadata of an exposed filter and boom, things got cached all the time
damiankloip’s picture

damiankloip commented

This change looks good to me, I spoke to Daniel and I can see how merging this array directly is causing issues. Just needs some sort of test coverage.

dawehner’s picture

dawehner
Primary language German
commented

Working on this now., meh

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
Issue tags: -Needs tests, -Needs issue summary update
StatusFileSize
new 2549719-7.patch3.68 KB
new interdiff.txt3.01 KB

Here is a test.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2549719-8-fail.patch3.01 KB

Let's fail it as well.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
+++ b/core/modules/views/src/Tests/ViewElementTest.php
@@ -127,7 +127,7 @@ public function testViewElementEmbed() {
-    $render['#embed'] = TRUE;
+    $render['view']['#embed'] = TRUE;

Why does this need to be moved a level down?

dawehner’s picture

dawehner
Primary language German
commented

Why does this need to be moved a level down?

Because it was wrong previously, see 

    $this->render = array(
      'view' => array(
        '#type' => 'view',
        '#name' => 'test_view_embed',
        '#display_id' => 'default',
        '#arguments' => array(25),
        '#embed' => FALSE,
      ),
    );
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Ah! That makes sense.

It's a bit concerning then that A) the tests were passing, B) no changes were necessary to the existing tests. But I guess they happened to be asserting things that were true for both embedded and non-embedded views. So that makes sense.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2549719-8-fail.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

#7 is RTBC, #8 is the test-only/FAIL patch.

damiankloip’s picture

damiankloip commented

RTBC for me too, good to see the ViewElementTest getting some more love!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Category: Task » Bug report
Status: Reviewed & tested by the community » Fixed

This is a bug. Committed 57fb699 and pushed to 8.0.x. Thanks!

  • alexpott committed 57fb699 on 8.0.x 
    Issue #2549719 by dawehner: View::preRenderViewElement should use $...

Status: Fixed » Closed (fixed)

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