Problem/Motivation

Found in #2919373: Prevent Settings Tray functionality for blocks that have configuration overrides.

The root cause is not re-rendering/altering. That's a consequence of the root cause.

The root cause is that Contextual Links' client-side caching has the same assumptions as the Contextual Links module had in Drupal 7:

  1. contextual links are declaratively/statically defined (in *.links.contextual.yml files)
  2. their visibility depends on permissions

1+2 has resulted in contextual.module's implementation roughly looking like this:
For each place where contextual links must be rendered, there is a placeholder containing an ID that identifies all links that should be rendered.

  1. We don't render them on the server side, because then it would potentially invalidate the entire page. The server side sends a placeholder to the client side.
  2. Some JS on the client side finds these placeholders, and asks the server for a bunch of links (rendered as HTML) to show for that placeholder. Only links that the current user can access are returned.
  3. The HTML that the JS receives is cached on the client side, in window.sessionStorage.
  4. The client-side cache is invalidated whenever A) a new tab is opened (sessionStorage is bound to tabs), B) whenever the set of permissions for the current user changes, thanks to drupalSettings.user.permissionsHash.

This was implemented in #2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links. You can find more detailed information and related issues there. The implementation introduced in that issue had at least one important flaw/bug in its assumptions, that was there from the very beginning: #2773591: New contextual links are not available after a module is installed.

But now in #2919373, we're violating the assumptions that were fine in #2136507: visibility of links in Contextual Links is no longer only determined by access, but also by whether a config entity that is rendered and has contextual links (Block in this case) has configuration overrides.

In other words: this is no longer correct:

  1. The client-side cache is invalidated whenever A) a new tab is opened (sessionStorage is bound to tabs), B) whenever the set of permissions for the current user changes, thanks to drupalSettings.user.permissionsHash.

We need to add C) whenever the set of configuration overrides changes.

In other words: we need a service similar to @user_permissions_hash_generator
(Drupal\Core\Session\PermissionsHashGenerator), but for config overrides: @config_overrides_hash_generator. This would then need to be exposed to the client side just like @user_permissions_hash_generator is exposed to the client side via drupalSettings.uer.permissionsHash. This is the equivalent of a client-side cache tag: a way client-side code to be notified of changes it needs to be able to react to.

Proposed resolution

Therefore this is no longer correct:

  1. The client-side cache is invalidated whenever A) a new tab is opened (sessionStorage is bound to tabs), B) whenever the set of permissions for the current user changes, thanks to drupalSettings.user.permissionsHash.

We need to add C) whenever the set of configuration overrides changes.

In other words: we need a service similar to @user_permissions_hash_generator
(Drupal\Core\Session\PermissionsHashGenerator), but for config overrides: @config_overrides_hash_generator. This would then need to be exposed to the client side just like @user_permissions_hash_generator is exposed to the client side via drupalSettings.uer.permissionsHash. This is the equivalent of a client-side cache tag: a way client-side code to be notified of changes it needs to be able to react to.

Then contextual.es6.js could be updated to not only invalidate its client-side caches when permissions assigned to roles changed, but also when config overrides change. And then Settings Tray would once again work correctly, without the need for hacks.

Remaining tasks

Implement

User interface changes

None

API changes

API addition: drupalSettings.core.configOverridesHash

Data model changes

None.

CommentFileSizeAuthor
#6 2937467-6.patch4.63 KBtedbow
#2 2937467-2.patch4.6 KBtedbow

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Here is patch that just provides a failing test.

It just proves that hook_contextual_links_view_alter and hook_contextual_links_alter can be used to alter the links but after saving the block the links are not alterable via these hooks.

tedbow’s picture

Title: Contextual links are not renderer or alterable for block that has been updated » Contextual links are not re-rendered or alterable for block that has been updated

Status: Needs review » Needs work

The last submitted patch, 2: 2937467-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

wim leers’s picture

Title: Contextual links are not re-rendered or alterable for block that has been updated » Access to certain contextual links may vary not by permissions, but by config overrides: add "config overrides" client-side cache tag
Category: Bug report » Task
Issue summary: View changes
Issue tags: +D8 cacheability
Related issues: +#2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links

I think the issue title is wrong.

The root cause is not re-rendering/altering. That's a consequence of the root cause.

The root cause is that Contextual Links' client-side caching has the same assumptions as the Contextual Links module had in Drupal 7:

  1. contextual links are declaratively/statically defined (in *.links.contextual.yml files)
  2. their visibility depends on permissions

1+2 has resulted in contextual.module's implementation roughly looking like this:
For each place where contextual links must be rendered, there is a placeholder containing an ID that identifies all links that should be rendered.

  1. We don't render them on the server side, because then it would potentially invalidate the entire page. The server side sends a placeholder to the client side.
  2. Some JS on the client side finds these placeholders, and asks the server for a bunch of links (rendered as HTML) to show for that placeholder. Only links that the current user can access are returned.
  3. The HTML that the JS receives is cached on the client side, in window.sessionStorage.
  4. The client-side cache is invalidated whenever A) a new tab is opened (sessionStorage is bound to tabs), B) whenever the set of permissions for the current user changes, thanks to drupalSettings.user.permissionsHash.

This was implemented in #2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links. You can find more detailed information and related issues there. The implementation introduced in that issue had at least one important flaw/bug in its assumptions, that was there from the very beginning: #2773591: New contextual links are not available after a module is installed.

But now in #2919373, we're violating the assumptions that were fine in #2136507: visibility of links in Contextual Links is no longer only determined by access, but also by whether a config entity that is rendered and has contextual links (Block in this case) has configuration overrides.

Therefore this is no longer correct:

  1. The client-side cache is invalidated whenever A) a new tab is opened (sessionStorage is bound to tabs), B) whenever the set of permissions for the current user changes, thanks to drupalSettings.user.permissionsHash.

We need to add C) whenever the set of configuration overrides changes.

In other words: we need a service similar to @user_permissions_hash_generator
(Drupal\Core\Session\PermissionsHashGenerator), but for config overrides: @config_overrides_hash_generator. This would then need to be exposed to the client side just like @user_permissions_hash_generator is exposed to the client side via drupalSettings.uer.permissionsHash. This is the equivalent of a client-side cache tag: a way client-side code to be notified of changes it needs to be able to react to.

Then contextual.es6.js could be updated to not only invalidate its client-side caches when permissions assigned to roles changed, but also when config overrides change. And then Settings Tray would once again work correctly, without the need for hacks.

Updated the issue summary with this. (This is a more detailed version of what I already wrote at #2919373-73: Prevent Settings Tray functionality for blocks that have configuration overrides.)

tedbow’s picture

Status: Needs work » Needs review
StatusFileSize
new4.63 KB

@Wim Leers thank you for the thorough explanation again here.

It seems though we added the `@config_overrides_hash_generator` and relied on the in the contextual module the we would causing the client side html to be invalidated much more often than is needed. This is because this would happen whether or not you had the Settings Try module enabled.

If there has not been a need for varying contextual links based on config overrides then it seems like this is a small use and we should force all contextual links to invalidate based on this.

Also everytime some module comes up with a new value the contextual links should be invalidated upon we have then update the contextual module itself.

What if we added the ability for other modules to designate that they needed client-side contextual links to invalidate based on other hashes?

Here is patch that adds drupalSettings.contextual.hashKeys. Then in contextual_js_settings_alter() it sets:
$settings['contextual']['hashKeys'][] = 'user.permissionsHash';.

So the patch only functionality works the same but give the ability for other modules to set hashes and have the contextual module invalidate links when they changes.

So Settings Tray module would have to
$settings['contextual']['hashKeys'][] = 'settings_tray.block_overridesHash';
and then make sure drupalSettings.settings_tray.block_overrides is set. Probably in the way that @Wim Leers describes above. Though only for block overrides not all config overrides.

We could of course just have modules set drupalSettings.contextual.hashValues not have to set another value on drupalSettings but has drupalSettings.user.permissionsHash shows these hashes are of value outside of the just caching contextual links.

The patch gets rid of the old test for now and just shows the current system works with this new more flexible architecture.

I wanted to get a quick patch to see if this might work so copied the JS function getNested() from here: https://medium.com/javascript-inside/safely-accessing-deeply-nested-valu...
This allows settings $settings['contextual']['hashKeys'][] which will correspond to a nested value on drupalSettings

If might be simpler just do
$settings['contextual']['hashValue'][] = \Drupal::service('user_permissions_hash_generator')->generate(Drupal::currentUser());
if generating the hash 2x is not too expensive. then you won't need to look up values on drupalSettings

wim leers’s picture

because this would happen whether or not you had the Settings Try module enabled.

Fair point. Maybe we should rewrite the entire client-side caching logic of Contextual Links!

What if we added the ability for other modules to designate that they needed client-side contextual links to invalidate based on other hashes?

We could, but could a module actually reliably know that?

#2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links assumed all route access checks (for routes used in contextual links) would always only depend on permissions. For the route for the contextual link that Settings Tray is adding, that's not true: it also depends on config overrides (for that particular block entity). But really, an access check could depend on anything. And entity access checks themselves are actually alterable.

Given that, how could a module indicate which "client-side cache tag" hashes should be used for every contextual link?

The more I think about it, the more I think that the approach that I originally proposed and got committed in #2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links 4 years ago (in 4 days!) is just fundamentally flawed.

wim leers’s picture

The more I think about it, the more I think that the approach that I originally proposed and got committed in #2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links 4 years ago (in 4 days!) is just fundamentally flawed.

Because of writing this, I started looking at the patch in #2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links again.

And I noticed something:

       $build['#contextual_links']['custom_block'] = array(
         'route_parameters' => array('custom_block' => $entity->id()),
+        'metadata' => array('changed' => $entity->getChangedTime()),
       );

That metadata key was introduced in #2136507-2: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links, where I wrote:

However, more care is needed in this case, since contextual links for entities may vary depending on the state of the entity. So, the contextual links may only be cached for as long as the entity remains unchanged

[…]
The only way I see we can do this without introducing API changes is to make every entity type that adds contextual links to its entities, also set #contextual_links's 'metadata' key to array('changed' => $entity->getChangedTime()). That immediately resolves the problem.

This is pretty much exactly the problem that we're facing in #2919373: Prevent Settings Tray functionality for blocks that have configuration overrides! Can't we just add the overriddenness to that metadata key? That'd instantly solve it.

tedbow’s picture

Status: Needs review » Closed (works as designed)

We are now solving #2919373: Prevent Settings Tray functionality for blocks that have configuration overrides by adding the has_overrides key to the contextual link metadata. This provides a new contextual link variation.

I still think my patch in #6 is an interesting idea to add more flexibility but it is not needed now.