Add tests for module-provided contextual links on blocks

I took a look at this but didn't find the cause. I agree that block cache is a prime suspect but I don't see the connection. Block cache is now implemented at the level of 'content' for a given block which is one level down from the level where #contextual_links appear in the $page array. So, contextual links are not cached at all.

The missing links are supposed to be added by menu_context_links() and that function is not finding them. Thats a menu system fail AFAICT.

The attached patch fixes up a couple minor things with contextual that I noticed. Access control in theme layer makes little sense. Please put this in the final patch here if possible.

So the Contextual Links Example has tests that catch this... but apparently core doesn't. We should get them in here. See #1310536: Contextual Links Example completely broken (Edit object doesn't work in block)

Sorry I'm a little late to the party. I tracked this down independently a few days ago using git bisect and found that it was broken by commit 08b9bd41723a "Issue #918808 by moshe weitzman, Damien Tournoud, dww: Standardize block cache as a drupal_render() #cache." Which is exactly what the original poster figured out. The Contextual Links Example tests run green until that commit, then after that commit they fail because that module adds a contextual link to a block, but the link does not show. Reverting that commit makes the links show up on the block again, and the tests work again. There's no doubt in my mind that the problem was caused or exposed by the above commit.

On tests: The tests for the contextual links example in Examples would help core. It would be easy enough to grab that, change the module name, use the module as a mock module, and make the tests into a test set for contextual links (or add to what's there, if anything). That should be a novice task...

Trying to fix it with this patch.

Could we get a description about why we built contextual links on one level and then copy them to another. Looks a bit odd. Also, needs reroll.

It seems that it's due to the block caching. To me the links did appear in when you render the content of a block. the new function _block_get_renderable_block is not using the cache as it was in _block_render_blocks. The code seems now to omit the contextual links for some reason and the $build[$key] array was not right to display them.

The patch needs a whitespace cleanup and a test, but sending to the bot.

Great to see progress on this issue!

@julien: In addition to what @xjm said, I think there needs to be code comments explaining what you said in #10, because the reason for those lines is not at all obvious.

Fix the white space and added comments.
It seems that to display the contextual links, this variable needs to be set:

$build[$key]['#contextual_links']

But the $build variable array is not set right, the right data are set here:

$build[$key]['content']['#contextual_links']

That's why i'm changing the array. The better way should be to set $build[$key]['#contextual_links'] directly in _block_get_renderable_block function.

Thanks @julien! Couple notes:

+++ b/core/modules/block/block.moduleundefined
@@ -369,6 +369,10 @@ function _block_get_renderable_region($list = array()) {
+      // To render the contextual links for a block, the variable $build[$key]['#contextual_links'] needs to be set
+      // instead of $build[$key]['content']['#contextual_links'] that is returned by the previous _block_get_renderable_block function

The comment lines should wrap at 80 characters. Also, it would be better not to reference the previous code, and just explain the why of the current code.

+++ b/core/modules/block/block.moduleundefined
@@ -369,6 +369,10 @@ function _block_get_renderable_region($list = array()) {
+		$build[$key]['#contextual_links'] = $build[$key]['content']['#contextual_links'];

There's still some weird indentation here. The indentation should be two spaces.

Also, you can mark the issue "Needs Review" when you upload a patch. That makes testbot test the patch automatically.

Sorry, this 'ghost in the machine' style patching will not fly. Why are we building at the wrong level in the first place? We need understanding, not 'I can make it work'. I'll try to look int ot his when time permits.

@moshe weitzman: I fully agree that this seems to be treating the symptom rather than the underlying problem, but I applaud @julien for taking the time to work on this and identify why this symptom is appearing. Now for how to fix it - my own knowledge of the FAPI internals isn't great enough to help pinpoint a solution. But as I pointed out in #4 it's very clear what commit broke this behavior, so hopefully someone who does have this knowledge, or someone who has time to track this back, can use the information @julien has provided to figure out what went wrong.

@David, That's exactly right.

It's maybe better to try to fix it in the function _block_get_renderable_block instead than in _block_get_renderable_region, makes more sense.

// Add the content renderable array to the main element.
$element['content'] = $block->content;
// Also add the contextual links renderable array to the main element
if (isset($block->content['#contextual_links']))
  $element['#contextual_links'] = $block->content['#contextual_links'];

Note that removing the ['content'] key would break D8 again, see #1300290-11: In drupal_render(), element_info is not merged recursively, preventing #type from serving its purpose and #1283892: Let Render API fail in a tale-telling way on invalid $element.

That if statement needs {}

Yes. thanks.

@julien -- All patches to core need to follow the coding standards before they are committed. So that is how @tim.plunkett's comment helps to solve the issue. The patch will not be committed with code style inconsistencies. Thanks!

I think I agree with David that reverting to the D7 way of doing it (without the 'content' element) looks good. I vaguely recall that adding the 'content' element to avoid some key collision. We'll see when we try to rip that out. I don't yet follow the problem that Tim links to in #21 . Have to study that.

But we have a deeper problem. Contextual module makes an alluring promise: just add #context_links to any render element and the list of links provided get transformed into lovely hover navigation. Links that are inaccessible for a given user will not be shown.

The flaw with this promise is that it is completely incompatible with render cache. The render cache stores fully themed HTML, so you can't just store the same HTML for all users. Render cache requires that if your HTML varies, you must declare corresponding cache keys so that each person gets the right variant. This is not a regression - contextual is incompatible with render cache in Drupal 7 but we had not noticed yet, since it is very lightly used in core.

So, I think #contextual_links implementations are going to have to add/edit cache keys in addition to listing links. I worry that cache keys are going to be hard to determine given that menu items don't currently declare their access control keys separately. The actual declaring may get easier once we have #636454: Cache tag support.

I note that contextual module does a lot of work during contextual_preprocess() which is wrong IMO. The theme layer is not the right place for access control, nor to be transforming a simple #contextual_links array into a #type=contextual_links render array which later transforms into a #type=links render array. It would be better if we built a #type=contextual_links render array to start with. I'm not yet sure how to do that. The whole title_suffix thing is pretty oddball.

@moshe, that issue with contextual links completely breaking HTML caching is being tracked in #914382: Contextual links incompatible with render cache. I came to a similar conclusion as you in there.

OK, we can deal with the caching problem in the other issue.

I forgot that I decided that moving the block content up a level would not work for any block that uses block caching. You will run into the same cache problem that forum block has. You can't have #cache and #contextual_links in same level until ##914382 is resolved. It would be expedient to move #cache down a level so that it is a sibling of the new 'content' element, but I think we'll have a problem getting the links HTML printed since there is no $title_suffix when themeing this element.

This is surely not critical. Even the OP:

I wouldn't totally object if it were downgraded to major, though.

I rerolled #1300290-15: In drupal_render(), element_info is not merged recursively, preventing #type from serving its purpose, that should help illustrate why removing the ['content'] key would break.

attached is a patch containing a test for this issue. I didn't merge it with the current patch because of Tim's comments above.

Sending to testbot to check the failures.

Thanks @disasm! It looks like the new test is exposing the failure correctly. A few style notes below. I'm also wondering if we need the tests for logged out users here or can that be covered in #914382: Contextual links incompatible with render cache?

+++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualLinksTest.phpundefined
@@ -0,0 +1,56 @@
+  public static $modules = array('contextual','menu');

Space after the comma.

+++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualLinksTest.phpundefined
@@ -0,0 +1,56 @@
+   $this->assertNoLinkByHref('admin/structure/menu/manage/navigation/list?destination=node', 0, 'List NoLinks link for navigation menu is not displayed for logged out user.');

This assertion text looks like it should read "List Links link…". Maybe all these assertions can have quotes added for clarity because of the repetition of the word "link" - i.e. "List Links" link for navigation menu is not displayed for logged out user.

+++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualLinksTest.phpundefined
@@ -0,0 +1,56 @@
+      'description' => 'Tests if contextual links in blocks are cached properly and not shown to users without the correct permissions.',

Description should be updated to reflect what is being tested here.

attached are requested changes.

Let's see if it still applies.

Updated the tests now that the tests run without the standard profile, so we need to enable block module, place a menu block and put a link in the menu to make it actually appear. Also updated the actual patch to conform to code style.

The tested paths (menu configuration vs. block configuration ID differences) show the reason for a bug that I found which was obscured by this issue: #1868348: Missing contextual links on system menu blocks.

#36: drupal-contextual_links-1304470-36-tests-only.patch queued for re-testing.

#36: drupal-contextual_links-1304470-36.patch queued for re-testing.

Wow, thats wierd. Thanks for fixing this :)

This will conflict with #1535868: Convert all blocks into plugins, I'd prefer to see this postponed on that, but I defer to catch on that.

@tim.plunkett: Based on our recent discussions #1535868: Convert all blocks into plugins is not yet close to being committed, so not sure if a major bug's fix should be held back on that.

We've actually agreed to commit blocks-as-plugins as soon as the cache decorator issue is fixed, with everything else dealt with in followups. I'd really suggest letting this particular issue lie for a few more days, rather than eating up another hour or more of someone's time to try to resolve the conflict. If it turns out that we get stuck on the cache decorator then we can commit this one, but it might mean another hour or whatever of Tim's time or mine to sort it, and that happening over and over again is a serious drag on the blocks-as-plugins issue's momentum (and VDC's and SCOTCH's generally).

that's great that at least the patches are fixing the actual issue before it did reach the back in the future state. Maybe #1535868: Convert all blocks into plugins will override this anyway in a soon future.

Yeah I think that is mainly #914382: Contextual links incompatible with render cache, how we add contextual links in general needs a rethink - it should really be the same mechanism as the new inline editing so it doesn't interfere with caching at all.

Given there appears to be a plan to get the blocks patch in within the next week, I'm OK holding off on committing this since xjm and timplunkett have had enough inflicted on them with that patch as it is :(

If it drags on much further though it's not reasonable to hold up a straight major bug fix on that one, , so will give it until Jan 6th.

#36: drupal-contextual_links-1304470-36.patch queued for re-testing.

#36: drupal-contextual_links-1304470-36.patch queued for re-testing.

#20: contextual-links-1304470-5361818.patch queued for re-testing.

#36: drupal-contextual_links-1304470-36-tests-only.patch queued for re-testing.

#36: drupal-contextual_links-1304470-36.patch queued for re-testing.

Retesting the patch itself because the test results errors ain't clear enough to me.

I have a hard time understanding how would "Lock extended by this request." in "LockFunctionalTest.php" fail due to this patch. Sending for retest again.

#36: drupal-contextual_links-1304470-36.patch queued for re-testing.

Weird to get different errors on each test. Can't reproduce those errors locally, contextual, blocks and system tests are running fine, must be test server side related.

@julien: see, it passed now :) The testbot does produce intermittent failures sometimes which are visibly unrelated to the patch being tested. Moving back to RTBC as per the above.

@Gábor yes it's weird. nice to see that it's back to RTBC. :)

Now that blocks as plugins is in, this will need a re-roll.

Note that as of #62, menu blocks do have 3 contextual links showing up: List links, Edit menu, Configure block. I don't know whether to mark this issue fixed or if there's something still problematic that's within the scope of this issue.

At least the tests for module provided links to avoid this regression happen again?

Agreed, let's see what happens with the test now. I'll convert this test.

Er.

Retitling according to that. No major bug anymore.

As per #62 needs reroll.

Rerolled the patch for this issue.
Please check it out.

Rerolling the patch again for this issue.

I've fixed the patch so it no longer has the PHP error.

The test cannot work in principle because the links are not rendered to the html but rather fetched in an additional request.

/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualDynamicContextTest.php provides you potentially some ideas how to actually test it.

does not need a re-roll as the patch successfully applies

However, the class does need to be moved to PSR-4, and as @dawehner points out it also needs to be rewritten.

We now have sufficient test coverage for this, it was added as part of other issues.