Regression: Menu contextual links no longer visible in menu blocks [#2239833]

Updated: Comment #N

Problem/Motivation

In the 'Tools' block (which is enabled by default in the standard installation), there should be a contextual link 'Edit menu' but right now it just shows 'Configure Block'. Actually there's an 'Edit menu' link and we've tests for same also: MenuTest::testBlockContextualLinks

But somehow caching breaks stuff. See related issue: https://drupal.org/node/914382

How it appears now (no 'Edit menu'):
Contextual-links-not -visible.png

How it should appear (screenshot from 7.x):
Contextual-links-visible

Proposed resolution

Contextual links on menu blocks aren't working. The sad reason
for this is render caching (a fucking damn problem, seriously!).

      if ($plugin->isCacheable()) {
        $build[$entity_id]['#pre_render'][] = array($this, 'buildBlock');
        // ...
      }
      else {
        $build[$entity_id] = $this->buildBlock($build[$entity_id]);
      }

      $build[$key]['#block'] = $entity;

      $this->moduleHandler()->alter(array('block_view', "block_view_$base_id"), $build[$entity_id], $plugin);

If the menu block is marked as cacheable (per default caching is enabled) $build[$entity_id]['content'] is
not filled, menu_block_view_system_menu_block_alter() is called
and does not do anything.

Solution: Probably move the #contextual_link handling onto the post_render level instead of the alter stuff

Remaining tasks

Decide how to fix it.

User interface changes

None (apart from the fact that contextual links will actually be visible).

API changes

None.

Comment	File	Size	Author
#19	2239833-19.patch	2.05 KB	Wim Leers
#19
#14	2239833-14.patch	2.04 KB	olli
#14
#14	2239833-fail.patch	1.41 KB	olli
#14
#4	menu_block_contextual_links-2239833-4.patch	1000 bytes	Wim Leers
#4
	Contextual-links-visible.png	10.79 KB	cs_shadow
	Contextual-links-not -visible.png	9.56 KB	cs_shadow

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

cs_shadow CreditAttribution: cs_shadow commented 13 April 2014 at 11:26

Issue summary:

View changes

Comment #2

cs_shadow CreditAttribution: cs_shadow commented 13 April 2014 at 11:26

Issue summary:

View changes

Comment #3

dawehner

German

CreditAttribution: dawehner commented 13 April 2014 at 11:41

Priority:	Normal	» Critical
Issue summary:	View changes

updated the issue summary with my "research"

Comment #4

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 22 April 2014 at 16:31

Title:	Contextual Links not visible in 'Blocks'	» Regression: Menu contextual links no longer visible in menu blocks
Assigned:	Unassigned	» Wim Leers
Status:	Active	» Needs review
Issue tags:		+D8 cacheability, +Performance, +sprint
Related issues:		+#2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags

File	Size
menu_block_contextual_links-2239833-4.patch	1000 bytes

I must've introduced this regression in #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags.

I verified that is the only occurrence of this problem (by grepping for #contextual_link).

Attached is a patch that fixes the regression and simplifies the code. The checking for $build['content'] is completely unnecessary nowadays.

Comment #5

dawehner

German

CreditAttribution: dawehner commented 23 April 2014 at 04:45

Priority:

Critical

» Major

Let's be honest, this is major, compared to other broken stuff. Sadly we probably need a test which actually don't work

Comment #6

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 23 April 2014 at 09:53

#5: Agreed with priority change, and with your assessment: MenuTest::testBlockContextualLinks() does test this:

    $id = 'block:block=' . $block->id() . ':|menu:menu=tools:';
    // @see \Drupal\contextual\Tests\ContextualDynamicContextTest:assertContextualLinkPlaceHolder()
    $this->assertRaw('<div data-contextual-id="'. $id . '"></div>', format_string('Contextual link placeholder with id @id exists.', array('@id' => $id)));

I don't understand why that didn't catch this problem.

Comment #7

dawehner

German

CreditAttribution: dawehner commented 23 April 2014 at 10:42

This one was initially discovered while working on https://drupal.org/comment/8677783#comment-8677783 , so maybe this test failure would be fixed here. Sadly I don't have my tools avaiable right now to remember why it worked before.

Comment #8

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 28 April 2014 at 15:09

What's still needed here?

Comment #9

moshe weitzman CreditAttribution: moshe weitzman commented 28 April 2014 at 16:23

Status:

Needs review

» Reviewed & tested by the community

Verified that the code is considerably simpler and the bug is fixed.

Comment #10

catch

he/him

English

CreditAttribution: catch commented 28 April 2014 at 19:30

Status:

Reviewed & tested by the community

» Fixed

If this is exposed by https://drupal.org/comment/8677783#comment-8677783 I'm OK with committing this without explicit test coverage - we'll get it once the other issue is committed.

Committed/pushed to 8.x, thanks!

Comment #11

28 April 2014 at 19:30

Commit dda140f on 8.x by catch:

Issue #2239833 by Wim Leers: Regression: Menu contextual links no longer...

Comment #12

12 May 2014 at 20:07

Status:

Fixed

» Closed (fixed)

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

Comment #13

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 23 June 2014 at 11:56

Issue tags:

-sprint

Comment #14

olli CreditAttribution: olli commented 6 September 2014 at 14:30

Status:

Closed (fixed)

» Needs review

File	Size
2239833-fail.patch	1.41 KB

2239833-14.patch	2.04 KB

Here's a test.

Comment #15

6 September 2014 at 15:21

The last submitted patch, 14: 2239833-fail.patch, failed testing.

Comment #16

dawehner

German

CreditAttribution: dawehner commented 6 September 2014 at 18:16

Good catch!

+++ b/core/modules/menu_ui/src/Tests/MenuTest.php
@@ -531,6 +532,12 @@ public function testBlockContextualLinks() {
+    $this->drupalPostForm('admin/structure/block/manage/' . $block->id(), ['settings[cache][max_age]' => Cache::PERMANENT], t('Save block'));
+    $this->drupalGet('test-page');
+    $id = 'block:block=' . $block->id() . ':|menu:menu=tools:';
+    $this->assertRaw('<div data-contextual-id="'. $id . '"></div>', format_string('Contextual link placeholder with id @id exists.', array('@id' => $id)));

I want to understand that. How does that actually work? You first add block caching, then you access the page once. Isn't block caching not returning a value yet? I know the test fails otherwise, so I try to understand why.

Comment #17

olli CreditAttribution: olli commented 6 September 2014 at 19:13

Isn't block caching not returning a value yet?

Not sure what you mean by that but the existing test adds the block with "Maximum age" set to <no caching> and we have two code paths (see IS) so the #pre_render path is not yet covered.

Just curious, would it hurt/work to always run the #pre_render path?

Comment #18

olli CreditAttribution: olli commented 19 October 2014 at 06:24

I think the original fix #4 was broken in 70bed33. Issue referenced in #10 went in but the Edit menu link is still missing.

Comment #19

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 19 November 2014 at 12:49

Status:

Needs review

» Reviewed & tested by the community

File	Size
2239833-19.patch	2.05 KB

#14 only needed a straight reroll against HEAD. It still fixes the problem, and adds the necessary regression test coverage. Here's that reroll.

Comment #20

catch

he/him

English

CreditAttribution: catch commented 20 November 2014 at 10:18

Status:

Reviewed & tested by the community

» Needs work

Sorry folk for issue history please open a new issue rather than re-opening the old one (unless it's for a revert), just adding tests here would have been a normal task, rather than major bug.