New contextual links are not available after a module is installed

The alternative I could like to propose is to calculate a hash of all contextual links and send this along when the contextual links are rendered. This also fixes the problem that clearing the cache when changing the YML file, doesn't cause the new links to appear.

The user opens a new browser session
The user logs out and in.

With the Outside In patch, neither of these is sufficient for me to get the contextual links to work. It only works if (as @effulgentsia suggested), I enable the module, do a full site cache clear through the UI, and THEN open a new browser session and log in again.

I am also not sure this is true of all contextual links. Using these steps:

1. Install minimal through the UI with this patch applied, 8.2.x HEAD.
2. Enable contextual (and optionally toolbar).
3. Return to the frontpage. Contextual links are available on the blocks immediately.
4. Install the Menu UI and the modules it depends on.
5. Return to the frontpage. Contextual links for editing menus are available immediately.
6. Install Outside In (and toolbar if you did not already). No contextual links for the module and no sidebar possible.

Note that I tried it both with and without Toolbar enabled at the start, just in case.

So I do not think this is a core bug. Thus far it appears to only happen with Outside In.

I tried a different variation of #4 (all of the following steps except #1 done through UI, not Drush):

1. In standard.info.yml, comment out the views_ui dependency.
2. Install Standard.
3. Add the Who's Online block (which is a View) to the left sidebar.
4. Go to the homepage, and notice that the Who's Online block only contains the "Configure Block" contextual link.
5. Enable Views UI module.
6. Go back to homepage, and notice that the Who's Online block still only contains the "Configure Block" contextual link, and does not contain the "Edit View" link that it now should have thanks to Views UI module.
7. "Clear all caches" via /admin/config/development/performance.
8. Go back to homepage, and notice that the Who's Online block now contains the "Configure Block" contextual link and the "Edit View" link.

So yes, #4's conclusion seems correct that Outside In is behaving differently, because a cache clear alone isn't enough, like it is with Views UI. And instead, Outside In seems to also require some client-side invalidation (per #2753941-193: [Experimental] Create Outside In module MVP to provide block configuration in Off-Canvas tray and expand site edit mode), which Views UI does not seem to require.

Here is a way of reproducing that I stumble and get smad (sad and mad) about all the time.

• Write a new custom module ✓
• Install it ✓
• Decide to expose contextual links ✓
• Goto the browser page to see whether contextual links are enabled ✓
• Now enter something into a links.contextual.yml file ✓
• drush cr ✓
• Goto the page, nothing apppears
• drush cr ✓
• Goto the page, nothing happens
• Remember to use window.sessionStorage.clear()

While there might be some weird problem of the outside in module.

To be honest I somehow cannot believe that views_ui actually is able to fix that problem :)

This is what I have done so far. I am not sure about this is the right direction

@Sweetchuck
This is looking great. I would argue that we maybe still want to vary to links in cache, also when the permissions change.

In addition to the client-side work in #7, there's also the problem that ContextualLinkManager::getContextualLinkPluginsByGroup() calls $this->cacheBackend->set() without passing $this->cacheTags. As a result, when clearCachedDefinitions() is called, such as during module install, these cache entries aren't cleared. Additionally, ContextualLinkManager should probably also extend clearCachedDefinitions() to also clear its local properties, such as $this->pluginsByGroup.

The correct solution is already listed in the IS:

This could be fixed by a relying on a hash of current modules installed in addition to permissions.

That would solve the reported problem.

However, @dawehner is right that his proposed solution in #2 would go even further: it'd also fix the problem of modules having updated their set of contextual links and those changes not being reflected for end users.

So, the patch in #7 is correct in that respect. But it sadly also removes the checking of permissionsHash, which is absolutely incorrect. @dawehner also pointed this out in #8, but he seems to think it's more optional than it is. It's essential.

This shows that we really want JS test coverage of this, which we now can, because we now have JS tests that can use window.sessionStorage! :)

Here's a patch that checks both the permissions hash, and the hash of contextual link plugins, as that was fairly easy to put together. It doesn't go as far as checking a hash of links themselves, sorry.

I've assumed it's ok to just deal with the permissions and plugins hashes separately rather than sticking them somewhere together.

Aw, patch needs a re-roll as it was actually made against 8.3.x, sorry!

Re rolled.

Still needs tests.

+++ b/core/modules/contextual/contextual.module
@@ -65,7 +66,13 @@ function contextual_page_attachments(array &$page) {
+  $page['#attached']['drupalSettings']['contextual']['linkDefinitionsHash'] = hash('sha256', Settings::getHashSalt() . implode(':', array_keys($cl_definitions)));

Do we really need to salt this?

We have in our project a similar issue, but it's caused by the fact that some permissions come from the organic group module.
This leads to:

• wrong (not updated) links are shown when a user membership change (for example, it becomes a moderator of a group);
• (less severe, but still applies) when you are logged in as user1 which has a certain role in a group, and then you log out and log in (using the same browser) as user2 which has a different set of permissions in that same group, you will again get an outdated list of links.

A solution might be to swap out the user_permissions_hash_generatorservice to include the og permissions/roles, but that will mess with the cache_context.user.permissions and we don't think it's the proper approach.

Let's get this moving forward again. This would remove two @todos from the Settings Tray module and would help the Drupal contrib ecosystem (see #24) too.

I agree that limiting this use case only to enabling / disabling modules is not enough. A very simple use case we are having in Organic Groups is affected by this:

1. Visit a group page as non-member of a group: you won't have the contextual links available that allows you to perform actions that a member can (such as adding group content etc.).
2. Join the group.
3. The contextual links menu should now show the newly available options, but the old menu is still shown.

Joining and leaving a group are very trivial actions for a user in a site that uses OG. It basically makes the use of contextual links impossible for managing group related stuff.

Neither checking the list of enabled modules, or hashing the set of available contextual menus will solve this case. The ideal solution would be to have a cache context / cache tag like system. But being able to nuke the links from the PHP side would be a good workaround. Also having an event that fires when the hash is calculated would be a good idea, then developers can hook into it if they have any need for customizing the hash generation.

Saw someone else get bit by this again today

#21 looks like it needs a re-roll for both 9.1.x/9.2.x

Re-rolled patch in #21 as suggested in #37... Thanks!

Updated the es6 file.

Typo in patch sorry!

Tried to fix Custom Commands failure in #40

I think this should fix the custom commands failures. At least commit-code-check.sh passes on my local system.

So we may be seeing this error again. But it’s not consistent as some devs are seeing while others not. Is there a cache table we could check?

1. +++ b/core/modules/contextual/contextual.module
   @@ -66,7 +67,13 @@ function contextual_page_attachments(array &$page) {
   +  $cl_manager = \Drupal::service('plugin.manager.menu.contextual_link');
   +  $cl_definitions = $cl_manager->getDefinitions();
   

   I don't think we need the intermediary $cl_manager variable for a single use

2. +++ b/core/modules/contextual/contextual.module
   @@ -66,7 +67,13 @@ function contextual_page_attachments(array &$page) {
   +  ksort($cl_definitions);
   ...
   +  $page['#attached']['drupalSettings']['contextual']['linkDefinitionsHash'] = hash('sha256', Settings::getHashSalt() . implode(':', array_keys($cl_definitions)));
   

   ubernit, but we don't need the cl_ prefix here in my opinion

3. +++ b/core/modules/contextual/js/contextual.es6.js
   @@ -21,8 +21,18 @@
   -  if (cachedPermissionsHash !== permissionsHash) {
   -    if (typeof permissionsHash === 'string') {
   +  const cachedDefinitionsHash = storage.getItem(
   +    'Drupal.contextual.linkDefinitionsHash',
   

   We need to retain the old hash too right? For when permissions are changed.

   So I think we need both flavours

4. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -39,13 +39,13 @@ protected function setUp(): void {
   -    $contextualLinks = $this->assertSession()->waitForElement('css', '.contextual button');
   -    $this->assertEmpty($contextualLinks);
   +    $contextualButton = $this->assertSession()->waitForElement('css', '.contextual button');
   +    $this->assertEmpty($contextualButton);
   ...
   -    $contextualLinks = $this->assertSession()->waitForElement('css', '.contextual button');
   -    $this->assertEmpty($contextualLinks);
   +    $contextualButton = $this->assertSession()->waitForElement('css', '.contextual button');
   +    $this->assertEmpty($contextualButton);
   
   @@ -54,13 +54,33 @@ public function testContextualLinksVisibility() {
   -    $contextualLinks = $this->assertSession()->waitForElement('css', '.contextual button');
   -    $this->assertNotEmpty($contextualLinks);
   +    $contextualButton = $this->assertSession()->waitForElement('css', '.contextual button');
   +    $this->assertNotEmpty($contextualButton);
   ...
   -    $contextualLinks = $this->assertSession()->waitForElement('css', '.contextual button');
   -    $this->assertNotEmpty($contextualLinks);
   +    $contextualButton = $this->assertSession()->waitForElement('css', '.contextual button');
   +    $this->assertNotEmpty($contextualButton);
   

   These changes feel out of scope and should probably be done in their own issue

5. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -54,13 +54,33 @@ public function testContextualLinksVisibility() {
   +  protected function assertContextualLinkWithClass($class) {
   +    $contextLink = $this->assertSession()->waitForElement('css', ".contextual-links li.$class a");
   

   This is probably too reliant on the classy base theme, which we're hoping to remove.

   Can we instead assert the link text, rather than the class?

Resolved the 1st point suggested in #47

#48 works
drupal 9.4.5

Curious at #48 why just fix one issue from #47 when a few more were low hanging

Starting at #43 addressed the bullets in #47

Interdiff had a hard time generating though so may not contain all the changes.

Patch #51 Failed to Apply on D9.5.x.

Rerolled for 9.5 + 10.1

+++ b/core/modules/contextual/contextual.module
@@ -66,7 +67,11 @@ function contextual_page_attachments(array &$page) {
   }
 
+  $definitions = \Drupal::service('plugin.manager.menu.contextual_link')->getDefinitions();
+  ksort($definitions);
+
   $page['#attached']['library'][] = 'contextual/drupal.contextual-links';
+  $page['#attached']['drupalSettings']['contextual']['linkDefinitionsHash'] = hash('sha256', Settings::getHashSalt() . implode(':', array_keys($definitions)));
 }

This looks like a good solution, but it needs a comment.

Added a comment.

1. +++ b/core/modules/contextual/contextual.module
   @@ -66,7 +67,13 @@ function contextual_page_attachments(array &$page) {
   +  // Pass a hash of the current contextual link definitions to the javascript
   

   nit: JavaScript

2. +++ b/core/modules/contextual/contextual.module
   @@ -66,7 +67,13 @@ function contextual_page_attachments(array &$page) {
   +  $page['#attached']['drupalSettings']['contextual']['linkDefinitionsHash'] = hash('sha256', Settings::getHashSalt() . implode(':', array_keys($definitions)));
   

   I don't know if we should be using the hash salt for this, its kind of leaking privileged data, we really only want a hash, doesn't need to be secure

3. +++ b/core/modules/contextual/js/contextual.js
   @@ -21,17 +21,31 @@
        if (typeof permissionsHash === 'string') {
   

   do we need the equivalent type check for the new hash?

+++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
@@ -67,6 +67,29 @@ public function testContextualLinksVisibility() {
+  protected function assertContextualLinkWithText(string $text) {
+    $contextLinks = $this->assertSession()->waitForElement('css', ".contextual-links li a");
+    $this->assertNotEmpty($contextLinks);
+    foreach ($contextLinks as $contextLink) {
+      if ($contextLink->getText() === $text) {
+        break;
+      }

this doesn't fail if the link is missing, we need $this->fail() outside the loop

#59
1 = fixed
2 = removed hash salt
3 = don't think we need it as there is a check before this code

#60
1 = actually showed the tests weren't working and false positive. Updated tests.

+++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
@@ -67,6 +67,14 @@ public function testContextualLinksVisibility() {
+    drupal_flush_all_caches();

Use $this->resetAll() instead (and maybe a line break after, for readability)

So #61 the tests pass with the cache clear.

Is the original goal to have contextual links appear without a cache clear?

Not sure if the tests or fix need work but should of got red/green

afaik, a cache clear currently doesn't fix the bug in any way, because it's stored in sessionStorage:

while data in localStorage doesn't expire, data in sessionStorage is cleared when the page session ends

I'm not sure how best to ensure that a test is happening within the same page session. But that strikes me as the problem here

Number #54 patch no longer applies to Drupal 9.5.9.

Here's a patch that applies to Drupal 9.5.x; just to assist those of us needing that. It includes the es6.js file and the transpiled version, since both are needed in D9.5 patches as far as I can see.

+++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
@@ -64,6 +64,14 @@ public function testContextualLinksVisibility() {
+    drupal_flush_all_caches();

Does the comment in #9 help? It might remove the need for this and get you a failing test?

I know Drupal 9 is close to the end. But for last months maybe it is necessary for someone:
Drupal 9.5.11 patch.

Update for broken patch:
Core 9.5.11

Fixed failures in #70.

Reroll patch for drupal 10.1.7

Fixed the #73 testing failed issue. Please review.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Was tagged for issue summary which is still needed. Please don't put tickets in review without checking core gates and any open tags.

Now that we are mixing patches and MRs that will need to be cleaned up and noted in the updated issue summary.

I could not get the patch in #74 nor the diff from the MR to apply cleanly in 10.3. I've attached a re-rolled the patch for 10.3

Amending attribution.