Prevent Settings Tray functionality for blocks that have configuration overrides

This would be so much simpler with #2923004: Add method to check if any overrides are applied to \Drupal\Core\Config\Config.

1. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
   @@ -5,7 +5,7 @@
   +(function ($, Drupal, drupalSettings) {
   

   Arrow function. Destructure `Drupal` & `drupalSettings`.

2. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
   @@ -164,6 +164,17 @@
   +          Object.keys(drupalSettings.settings_tray.overridden_blocks).forEach(
   +            (blockId) => {
   +              if (instance.options.url.indexOf(`/admin/structure/block/manage/${blockId}/off-canvas`) !== -1) {
   +                instance.options.url = drupalSettings.settings_tray.overridden_blocks[blockId];
   +              }
   +            },
   +          );
   

   The wrapping of the .forEach method is kind of strange. Additionally the closing parenthesis for the .forEach method shouldn't be on a new line, and the comma shouldn't be there either. I think you want something like,

   if (drupalSettings.hasOwnProperty('settings_tray') && drupalSettings.settings_tray.hasOwnProperty('overridden_blocks')) {
     Object.keys(drupalSettings.settings_tray.overridden_blocks).forEach((blockId) => {
       if (instance.options.url.indexOf(`/admin/structure/block/manage/${blockId}/off-canvas`) !== -1) {
         instance.options.url = drupalSettings.settings_tray.overridden_blocks[blockId];
       }
     });
   }
   

1. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
   @@ -5,7 +5,7 @@
   +(($, Drupal, { settings_tray: settings }) => {
   

   Woahhhh

2. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
   @@ -164,6 +164,15 @@
   +            if (instance.options.url.indexOf(`/admin/structure/block/manage/${blockId}/off-canvas`) !== -1) {
   

   Perhaps an @see to point to the route name?

3. +++ b/core/modules/settings_tray/settings_tray.module
   @@ -58,6 +77,26 @@ function settings_tray_block_view_alter(array &$build) {
   +  if (_settings_tray_has_block_overrides($build['#block'])) {
   +    $url = Url::fromRoute('settings_tray.overridden_block_warning')
   +      ->setRouteParameter('block', $build['#block']->id())
   +      ->setOptions(
   +        [
   +          'query' => [
   +            MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_dialog.off_canvas',
   +          ] + \Drupal::destination()->getAsArray(),
   +        ]
   +      );
   

   Woahhhhhh! This is scary.

4. +++ b/core/modules/settings_tray/settings_tray.module
   @@ -58,6 +77,26 @@ function settings_tray_block_view_alter(array &$build) {
   +    $build['#attached']['drupalSettings']['settings_tray']['overridden_blocks'][$build['#block']->id()] = $url->toString();
   

   This is leaking cacheability metadata.

   ->toString(TRUE), assign to a variable, then use $variable->getGeneratedUrl() and add then do $this->renderer->addCacheableDependency($build, $variable).

Some quick nits:

1. +++ b/core/lib/Drupal/Core/Config/ConfigBase.php
   @@ -295,4 +283,33 @@ protected function castSafeStrings($data) {
   +    if (empty($key)) {
   +      return $data;
   +    }
   +    else {
   +      $parts = explode('.', $key);
   +      if (count($parts) == 1) {
   +        return isset($data[$key]) ? $data[$key] : NULL;
   +      }
   +      else {
   +        $value = NestedArray::getValue($data, $parts, $key_exists);
   +        return $key_exists ? $value : NULL;
   +      }
   

   Is there any harm in always calling NestedArray::getValue here? I think the behavior is the same, even if $key is empty.

2. +++ b/core/modules/settings_tray/tests/modules/settings_tray_override_test/src/ConfigOverrider.php
   @@ -0,0 +1,50 @@
   +    if (TRUE || !empty($GLOBALS['config_test_run_module_overrides'])) {
   

   Is the TRUE here some debug code left over?

And in terms of manual testing, I'm a little confused on how to see the error message on overridden blocks. Here's a flow I went through that seems unchanged with or without the patch:

To me the ideal UX here would be to see "Rechercher" in the title field, even though I know that isn't what this patch is doing. I did expect some behavior to change with the patch, namely that an error is shown on overridden blocks. Are language overrides a special case?

Edit: I re-applied the patch locally this morning and I'm not able to configure a Block with overrides, please ignore my manual testing note.

+++ b/core/modules/settings_tray/js/settings_tray.es6.js
@@ -169,6 +169,16 @@
+        }
+    ¶

lolz trailing whitespace (in the ES6 file, even, so we should fix that). :)

1. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
   @@ -169,6 +169,16 @@
   +            // @see Route entity.block.off_canvas_form
   +            if (instance.options.url.indexOf(`/admin/structure/block/manage/${blockId}/off-canvas`) !== -1) {
   

   Note: this will break if somebody alters the URL for this route: the JS will not know.

   That has a very tiny probability though.

2. +++ b/core/modules/settings_tray/settings_tray.module
   @@ -58,6 +76,29 @@ function settings_tray_block_view_alter(array &$build) {
   +    // The url query options should use the off-canvas dialog and add the
   

   Nit: s/url/URL/

3. +++ b/core/modules/settings_tray/settings_tray.module
   @@ -58,6 +76,29 @@ function settings_tray_block_view_alter(array &$build) {
   +    $query = [
   +      MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_dialog.off_canvas',
   +    ] + \Drupal::destination()->getAsArray();
   +
   +    $url = Url::fromRoute('settings_tray.overridden_block_warning')
   +      ->setRouteParameter('block', $build['#block']->id())
   +      ->setOptions(['query' => $query]);
   +
   +    // Store the URL to the overridden notice for this block in drupalSettings.
   +    // This will be used in the Javascript function prepareAjaxLinks() to
   +    // replace the URL to the Settings Tray edit form. We cannot use
   +    // settings_tray_contextual_links_view_alter() to alter the URL because the
   +    // contextual links will not be rebuilt for every context that could have
   +    // configuration overrides.
   +    $build['#attached']['drupalSettings']['settings_tray']['overridden_blocks'][$build['#block']->id()] = $url->toString();
   +    $generated_url = $url->toString(TRUE);
   +    \Drupal::service('renderer')->addCacheableDependency($build, $generated_url);
   

   I still think this is pretty scary.

   AFAICT it's possible to let PHP override this instead, either in hook_contextual_links_view_alter(), or even earlier, in hook_contextual_links_alter(). Then we also don't need JS to dynamically replace the URL … because it'll contain the correct URL.

   I feel like I'm overlooking something… so I'm sorry if I have!

4. +++ b/core/modules/settings_tray/settings_tray.routing.yml
   @@ -6,3 +6,10 @@ entity.block.off_canvas_form:
      requirements:
        _permission: 'administer blocks'
        _access_block_plugin_has_settings_tray_form: 'TRUE'
   +settings_tray.overridden_block_warning:
   +  path: '/admin/settings-tray-overridden/{block}'
   +  defaults:
   +    _controller: '\Drupal\settings_tray\Controller\OverriddenBlockConfig::overrideNotice'
   +    _title_callback: '\Drupal\settings_tray\Block\BlockEntityOffCanvasForm::title'
   +  requirements:
   +    _permission: 'administer blocks'
   

   This is all making the Settings Tray module decidedly coupled to the Block module. The module description should mention that.

   #2897272: Fix module description, hook_help(), and document module scope in *.api.php file is already doing that, so that's good :)

5. +++ b/core/modules/settings_tray/src/Controller/OverriddenBlockConfig.php
   @@ -0,0 +1,48 @@
   + * because there is not uniform method for edit any potential configuration
   

   s/not/no/
   s/edit/editing/

6. +++ b/core/modules/settings_tray/src/Controller/OverriddenBlockConfig.php
   @@ -0,0 +1,48 @@
   + * @see settings_tray_block_view_alter().
   

   Nit: trailing period that should be removed.

7. +++ b/core/modules/settings_tray/src/Controller/OverriddenBlockConfig.php
   @@ -0,0 +1,48 @@
   +        '#title' => $this->t('Edit Block'),
   

   Should it be Block or block?

8. +++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
   @@ -267,8 +268,10 @@ protected function assertOffCanvasBlockFormIsValid() {
   +   * @param bool $confirm_form
   +   *   Determines if the block form should be confirmed.
   

   Wouldn't $has_confirm_form be better?

9. +++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
   @@ -283,7 +286,10 @@ protected function openBlockForm($block_selector, $contextual_link_container = '
   +    }
   +
      }
   

   Supernit: extraneous \n.

10. +++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
    @@ -577,4 +583,52 @@ protected function getTestThemes() {
    +   * Tests that the blocks with configuration overrides are disabled.
    

    s/the//

11. +++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
    @@ -577,4 +583,52 @@ protected function getTestThemes() {
    +    $this->container->get('module_installer')->install(['settings_tray_override_test']);
    

    Any reason we don't just always install this module for this test? Then we know it also doesn't cause side effects for other tests.

    Plus, makes this test slightly simpler.

12. +++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
    @@ -577,4 +583,52 @@ protected function getTestThemes() {
    +    $overridden_block = $this->placeBlock('system_powered_by_block',
    +      [
    +        'id' => 'overridden_block',
    +        'label_display' => 1,
    +        'label' => 'This will be overridden.',
    +      ]);
    ...
    +    $block = $this->placeBlock('system_powered_by_block',
    +      [
    +        'label_display' => 1,
    +        'label' => 'Labely label',
    +      ]);
    

    WEird indentation.

13. +++ b/core/modules/system/tests/src/FunctionalJavascript/OffCanvasTestBase.php
    @@ -9,6 +9,11 @@
    +   * CSS select for the Off-canvas dialog.
    

    s/select/selector/
    s/Off/off/

#1 pending
#2 fixed
#3 pending
#4 pending
#5 fixed
#6 pending
#7 changed to “block”
#8 fixed
#9 fixed
#10 fixed
#11 pending
#12 fixed
#13 fixed

@Wim Leers I respect your fear.

:D

so this removes all the JS changes 🎉. So #20.1 is not longer an issue

Nice! Do you also like this solution better? It definitely results in a patch that is quite a bit smaller!

Why? Because of the permission? The existing route is already using this permission. Block is already a dependency. Am I missing something?

You can ignore that — like I said in #20.4: #2897272: Fix module description, hook_help(), and document module scope in *.api.php file is already doing that, so that's good :)

1. +++ b/core/modules/settings_tray/settings_tray.module
   @@ -190,3 +207,15 @@ function settings_tray_css_alter(&$css, AttachedAssetsInterface $assets) {
   +      $links['settings_tray.block_configure']['route_name'] = 'settings_tray.overridden_block_warning';
   
   +++ b/core/modules/settings_tray/settings_tray.routing.yml
   @@ -6,3 +6,10 @@ entity.block.off_canvas_form:
   +settings_tray.overridden_block_warning:
   +  path: '/admin/settings-tray-overridden/{block}'
   ...
   +    _controller: '\Drupal\settings_tray\Controller\OverriddenBlockConfig::overrideNotice'
   
   +++ b/core/modules/settings_tray/src/Controller/OverriddenBlockConfig.php
   @@ -0,0 +1,48 @@
   +class OverriddenBlockConfig {
   ...
   +  public function overrideNotice(BlockInterface $block) {
   

   "warning" vs "notice

   "override" vs "overridden"

   I think this could be made a bit more consistent.

2. +++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
   @@ -577,4 +583,49 @@ protected function getTestThemes() {
   +  /**
   +   * Gets the CSS selector for a block.
   +   *
   +   * @param \Drupal\block\BlockInterface $block
   +   *   The block.
   +   *
   +   * @return string
   +   *   The CSS selector for the block.
   +   */
   +  protected function getBlockCssSelector(BlockInterface $block) {
   +    return str_replace('_', '-', $this->getBlockSelector($block));
   +  }
   

   The patch is adding this. But the class already contains this:

     /**
      * Gets the block CSS selector.
      *
      * @param \Drupal\block\Entity\Block $block
      *   The block.
      *
      * @return string
      *   The CSS selector.
      */
     public function getBlockSelector(Block $block) {
   

   This is pretty confusing.

RTBC, but this is blocked on #2923004: Add method to check if any overrides are applied to \Drupal\Core\Config\Config, so marking as postponed instead :)

RTBC++

+++ b/core/modules/settings_tray/settings_tray.module
@@ -54,6 +56,21 @@ function settings_tray_contextual_links_view_alter(&$element, $items) {
+function _settings_tray_has_block_overrides(BlockInterface $block) {
+  return \Drupal::config($block->getEntityType()->getConfigPrefix() . '.' . $block->id())->hasOverrides();
+}

Do we really need a function for this? It's used only once.

Ok, Ted and I just reviewed the patch functionality in a monster 90 minute review session!

The general problem is as described in #2408549: Display status message on configuration forms when there are overridden values (which I didn't realize is still outstanding!), except a bit more "in your face," because we're exposing this form to content author-types. The proposed fix from the patch is that if you hit a block that has overrides (e.g. settings.php hard-coding, translations, etc.) that instead of showing them something that doesn't make sense (like a form that does nothing when you save it), we instead show a message that basically says "buzz off" except with oodles of Drupal jargon. ;)

So my first reaction that was, "Egads, we really need to English-ify that message..."

But my second reaction was, "Why are we even letting them click on Quick edit in the first place then if they in fact can't actually quickly edit?" This is consistent with how we handle e.g. menu items as well; if a menu item would lead someone to a 403 error, we simply don't show them the link so they are not then tempted to click on it.

This is especially annoying because on a given page in Spanish with 8+ clickable regions, 6+ of them will give you the error message if you click on them. But the only way to know which is which is by clicking/"pogo-sticking" on all of them and seeing the result.

So I think the preferred way to handle this, at least from my POV (I will check with the wider UX team in a sec), is to apply the same pattern we have for the page title block to any of these override blocks. In other words, don't show the dotted outline, enticing someone to click, and don't expose a Quick Edit option at all in the first place. Instead, just have the standard contextual links button, as other elements get, and let them choose "Configure block" from there if they want.

@tedbow's concern about this was that people will be confused if in English the page has 8+ regions to click on to quick edit, but in Spanish it only has 2. Which is probably fair. But I don't know that this concern needs to hold the module from being stable; we could iterate on this more post-release, IMO.

Additionally, we hit a couple of bugs during testing which were:

- The "Site name" block is not getting the message treatment, despite also having translatable text inside. Need to do something like inspect "related" config to see if it's overridden.
- For whatever reason, halfway through all of a sudden we stopped getting the message in the tray, even when we were triggering conditions that "should" have triggered the message (e.g. editing the search block while viewing in Spanish). Suspect some kind of caching issue in Contextual Links.
- Somehow the block description got saved as the Spanish translation of the block description?? And we couldn't replicate this from the "normal" block config form.

Also, escalating to "major" since this is a stable blocker for Settings Tray.

#28: good call removing that @internal helper method. It used to be called >1 times, not anymore. #29 is better :)

#30 (@webchick's review):

But my second reaction was, "Why are we even letting them click on Quick edit in the first place then if they in fact can't actually quickly edit?"

@tedbow's concern about this was that people will be confused if in English the page has 8+ regions to click on to quick edit, but in Spanish it only has 2. Which is probably fair. But I don't know that this concern needs to hold the module from being stable; we could iterate on this more post-release, IMO.

I feel like we're going in circles. #30 is re-iterating things already pointed out in #2815709.

Gábor and I have been saying this for a year or so — see #2815709: Settings tray conceptually incompatible with configuration overrides such as translations, which is the issue that spawned this issue. Settings Tray effectively doesn't allow you to do anything on many Multilingual sites.
The thing is that this patch is a step forward compared to the behavior in HEAD, where there is not even this very Drupal-y warning. In HEAD, it'll still show you a form, but it won't actually edit the currently overridden configuration.

IOW: this patch is step 1, #2815709: Settings tray conceptually incompatible with configuration overrides such as translations is step 2. As documented comment at #2815709-39: Settings tray conceptually incompatible with configuration overrides such as translations by @tedbow:

UPDATE: chatted with @xjm again, she thinks #2919373: Prevent Settings Tray functionality for blocks that have configuration overrides is the way to go first then maybe look at the language only situation

#2815709 is a must-have, per @xjm's comment at #2815709-37: Settings tray conceptually incompatible with configuration overrides such as translations.

#31:

1. Okay, these are nice improvements. Was going to ask to remove the controller+route, which #32 already does. #2815709: Settings tray conceptually incompatible with configuration overrides such as translations will have to undo these in some cases (e.g. only language overrides),
   but that's okay.
2. I hate to ask this but … doesn't that mean that we'd be shipping an unstable module? Apparently #33 addressed this!

#38:

1. Should this be marked @internal for now, so that we don't commit to this API just yet? IOW: Settings Tray could ship as stable functionality-wise and API-wise, but this portion of the API would not yet be considered stable.
2. What happens if a block does use related config, but doesn't implement this interface in its off-canvas/settings tray form?

Patch review:

1. +++ b/core/modules/settings_tray/settings_tray.module
   @@ -93,10 +124,14 @@ function settings_tray_preprocess_block(&$variables) {
   +  if (!empty($variables['elements']['#contextual_links']['block']['route_parameters']['block'])) {
   

   When is this not set?

2. +++ b/core/modules/settings_tray/settings_tray.module
   @@ -196,3 +231,15 @@ function settings_tray_css_alter(&$css, AttachedAssetsInterface $assets) {
   +  if (isset($links['settings_tray.block_configure']['route_parameters']['block'])) {
   

   Shouldn't this be
   if (isset($links['settings_tray.block_configure'])) {
   ?

3. +++ b/core/modules/settings_tray/src/Form/BlockFormWithRelatedConfigInterface.php
   @@ -0,0 +1,19 @@
   + * Any interface for forms with related configuration.
   

   s/Any/An/

4. +++ b/core/modules/settings_tray/src/Form/BlockFormWithRelatedConfigInterface.php
   @@ -0,0 +1,19 @@
   +interface BlockFormWithRelatedConfigInterface {
   ...
   +  public function hasOverriddenConfig();
   

   If we're adding this as a non-internal interface, then we should update settings_tray.api.php.

   In fact, shouldn't this simply become the required interface?.

   And shouldn't this become
   SettingsTrayBlockFormInterface extends \Drupal\Core\Plugin\PluginFormInterface? Because … shouldn't every block implement this? Block plugins that don't consume non-block config would always return FALSE;.

I assume that settings try uses the config entity API and not low-level config.

Then #2910353: Prevent saving config entities when configuration overrides are applied should be interesting, what I'm doing there is a) adding a hasOverrides() to any config entity and b) throw an exception when trying to save a config entity with active overrides. This means settings tray would then trigger that exception instead of overriding data.

I assume that settings try uses the config entity API and not low-level config.

It uses the Config API, not the Config Entity API, because things like site name + slogan are stored in simple config, not config entities.

See \Drupal\settings_tray\Form\SystemBrandingOffCanvasForm::submitConfigurationForm() in particular.

Maybe that specific block, but the same problem can also exist for block-level settings like the label. there the API would then prevent
it then.

+++ b/core/modules/settings_tray/src/Form/SystemMenuOffCanvasForm.php
@@ -150,4 +150,11 @@ public function setPlugin(PluginInspectionInterface $plugin) {
+  /**
+   * {@inheritdoc}
+   */
+  public function hasOverriddenConfig() {
+    return \Drupal::config($this->menu->getEntityType()->getConfigPrefix() . '.' . $this->menu->id())->hasOverrides();
+  }

+++ b/core/modules/settings_tray/settings_tray.module
@@ -54,6 +58,33 @@ function settings_tray_contextual_links_view_alter(&$element, $items) {
+function _settings_tray_has_block_related_overrides(BlockInterface $block) {
+  $has_overrides = \Drupal::config($block->getEntityType()->getConfigPrefix() . '.' . $block->id())->hasOverrides();
+  if (!$has_overrides) {
+    // Check if the related configuration that will be exposed in the Settings

this basically. This works around the config entity API right now to get at getOverrides(). With my issue, it wouldn't have to do that anymore.

Also, I didn't read all the of the discussion here, but what's the reason that \Drupal\settings_tray\Form\SystemMenuOffCanvasForm::setPlugin() for example isn't using \Drupal\Core\Config\Entity\ConfigEntityStorageInterface::loadOverrideFree()?

Yes, that could be confusing when you look at the a non-default language and then you edit and you get it in the default config. But isn't it just as confusing that you just won't be able to edit it at all in non-default languages without any visual feedback?

Yes, that could be confusing when you look at the a non-default language and then you edit and you get it in the default config.

IIRC that's basically how it works today, and that's what's so confusing.

But isn't it just as confusing that you just won't be able to edit it at all in non-default languages without any visual feedback?

👌 Indeed! That is supposed to be addressed in #2815709: Settings tray conceptually incompatible with configuration overrides such as translations, this issue is supposed to be the stopgap.

> IIRC that's basically how it works today, and that's what's so confusing.

That is how it works for the simple config (SystemBrandingOffCanvasForm), but that's not how it works with SystemMenuOffCanvasForm for example. That uses a normal load() when it should be using a loadOverridesFree() to get the same behavior.

So what happens there is you load the menu with the translated information, then you change some of it and change the translation back as the original language. That's exactly what #2910353: Prevent saving config entities when configuration overrides are applied wants to prevent.

Given that, I have two points:

A) This issue is currently defined as a task. But the menu case is actually a bug as you can lose your original labels/config. So possibly this issue should be a bug, to unify the different (in some case wrong) cases into one.

B) I'm not really sure that going from settings-tray-always-shows-original to settings-try-never-shows-edit-with-override is really an UX improvement . My approach as a non-ux developer would be instead to show a message indicating that there are configuration overrides/translations (translations is just one use case for having overrides, it could also be domain.module, settings.php based overriddes or something based on the moon phase) and provide a link to the standard form/translate UI.

The second point is just my personal UX opinion (which is likely not worth much :)) combined with the knowledge that we have no use case in core that allows to edit overriden form values in the same place as non-overridden ones, and with the immense complexity of multiple sources of overrides (even possibly on the same config elements), I don't think it's realistic to try and do that.

My approach as a non-ux developer would be instead to show a message indicating that there are configuration overrides/translations (translations is just one use case for having overrides, it could also be domain.module, settings.php based overriddes or something based on the moon phase) and provide a link to the standard form/translate UI.

Isn't that pretty much what this patch does, the screenshot in #30 shows, and @webchick is arguing against in #30?

The is way they will be like all other blocks that don't add the extra configuration. So we can handle that on each 'settings_tray' plugin form.

Makes sense!

Patch in #47 looks good. A few nits, and a bunch of remarks that could make the patch much simpler:

1. +++ b/core/modules/settings_tray/settings_tray.module
   @@ -54,6 +58,21 @@ function settings_tray_contextual_links_view_alter(&$element, $items) {
   + * Checks if a block or related configuration has overrides.
   ...
   +function _settings_tray_has_block_related_overrides(BlockInterface $block) {
   

   No longer checks related configuration. Name needs to be updated. Basically, a revert of #34's interdiff in this hunk.

2. +++ b/core/modules/settings_tray/src/Form/SystemBrandingOffCanvasForm.php
   @@ -56,30 +56,32 @@ public static function create(ContainerInterface $container) {
   -    $form['block_branding']['#type'] = 'details';
   -    $form['block_branding']['#weight'] = 10;
   -
   ...
   +    if (!$this->isSiteInfoOverridden()) {
   +      $form['block_branding']['#type'] = 'details';
   +      $form['block_branding']['#weight'] = 10;
   

   Wrapping this in an if-test and all the indentation changes make this patch much scarier looking than it is.

   I think it could be far simpler, by using #access:

   $form['site_information']['#access'] = AccessResult::forbiddenIf($this->isSiteInfoOverriden());
   

3. +++ b/core/modules/settings_tray/src/Form/SystemBrandingOffCanvasForm.php
   @@ -95,12 +97,26 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
   +    if (!$this->isSiteInfoOverridden() && $form_state->hasValue('site_information')) {
   

   If access is not allowed, this subtree of the form is never rendered, and Form API prevents submitting values that don't exist in the rendered form. So, this is unnecessary.

   Therefore, only keeping the $form_state->hasValue() is sufficient.

4. +++ b/core/modules/settings_tray/src/Form/SystemBrandingOffCanvasForm.php
   @@ -95,12 +97,26 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
   +      $site_info = $form_state->getValue('site_information');
   

   In fact, you could make this the if-test!

5. +++ b/core/modules/settings_tray/src/Form/SystemBrandingOffCanvasForm.php
   @@ -95,12 +97,26 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
   +  protected function isSiteInfoOverridden() {
   

   … which also means this is called in only one place, so you can inline it in point 2.

6. +++ b/core/modules/settings_tray/src/Form/SystemMenuOffCanvasForm.php
   @@ -80,32 +80,34 @@ public static function create(ContainerInterface $container) {
   +    if (!$this->isMenuOverridden()) {
   
   @@ -115,7 +117,10 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +    if (!$this->isMenuOverridden() && $form_state->hasValue('entity_form')) {
   +      $this->getEntityForm($this->menu)->validateForm($form, $form_state);
   
   @@ -123,8 +128,11 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
   +    if (!$this->isMenuOverridden() && $form_state->hasValue('entity_form')) {
   +      $this->getEntityForm($this->menu)->submitForm($form, $form_state);
   
   @@ -150,4 +158,14 @@ public function setPlugin(PluginInspectionInterface $plugin) {
   +  public function isMenuOverridden() {
   

   Same story here.

7. +++ b/core/modules/settings_tray/src/Form/SystemMenuOffCanvasForm.php
   @@ -123,8 +128,11 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
   +    }
   +
      }
   

   Nit: extraneous \n.

8. +++ b/core/modules/settings_tray/tests/modules/settings_tray_override_test/settings_tray_override_test.services.yml
   @@ -0,0 +1,5 @@
   +      - { name: config.factory.override}
   

   Nit: missing space.

👍

1. +++ b/core/modules/settings_tray/settings_tray.module
   @@ -54,6 +56,21 @@ function settings_tray_contextual_links_view_alter(&$element, $items) {
   +function _settings_tray_has_block_overrides(BlockInterface $block) {
   +  return \Drupal::config($block->getEntityType()->getConfigPrefix() . '.' . $block->id())->hasOverrides();
   
   @@ -196,3 +217,15 @@ function settings_tray_css_alter(&$css, AttachedAssetsInterface $assets) {
   +    if (_settings_tray_has_block_overrides($block)) {
   

   I see this is still used twice but a single line now. With my issue referenced before, it will simply become $block->hasOverrides(), maybe inline with a @todo on that issue?

2. +++ b/core/modules/settings_tray/src/Form/SystemBrandingOffCanvasForm.php
   @@ -95,11 +99,13 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
   +    if ($site_info = $form_state->getValue('site_information')) {
   +      $this->configFactory->getEditable('system.site')
   +        ->set('name', $site_info['site_name'])
   +        ->set('slogan', $site_info['site_slogan'])
   +        ->save();
   +    }
   +
   

   getValue() doesn't really make sense, those values will always there, they will simply have the default value that was set on the form, so this it will never be FALSE.

3. +++ b/core/modules/settings_tray/src/Form/SystemMenuOffCanvasForm.php
   @@ -123,8 +127,10 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
   -    $this->menu->save();
   +    if ($form_state->hasValue('entity_form')) {
   +      $this->getEntityForm($this->menu)->submitForm($form, $form_state);
   +      $this->menu->save();
   +    }
   

   Same here, these conditions for validate and save are not really doing anything.

   Also, it's not changed her, but setPlugin() must use loadOverrideFree(). Otherwise this will right now save overridden values now and will throw an exception after my issue is in.

+++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
@@ -577,4 +585,95 @@ protected function getTestThemes() {
+    // Confirm the menu block does not include menu section when the menu is
+    // overridden.
+    $this->container->get('state')->set('settings_tray_override_test.menu', TRUE);
+    $this->drupalGet('user');
+    $this->openBlockForm($this->getBlockSelector($menu_block));
+    $web_assert->elementNotExists('css', '#menu-overview');

So confirming this doesn't show up is not enough to cover what I wrote, you need to save the form and make sure you did not save anything you shouldn't have.

And for that reason, as I wrote earlier, I still think this should be classified as a bug, not a task.

#56.2 + 3: So my advice/analysis in #50.3 was wrong? If so, then I'm unaware of that change in Form API — it definitely used to be the case that you could rely on the values in form state. My bad for putting @tedbow on the wrong path then — sorry Ted! :(

Also, it's not changed her, but setPlugin() must use loadOverrideFree(). Otherwise this will right now save overridden values now and will throw an exception after my issue is in.

That's IMHO out of scope

Also, it's not changed her, but setPlugin() must use loadOverrideFree(). Otherwise this will right now save overridden values now

Good point!

+++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
@@ -625,18 +625,24 @@ public function testOverriddenConfigRemoved() {
+    // Confirm we did not save changes to the configuration.
+    $this->assertEquals('Llama Fan Club', \Drupal::configFactory()->get('system.site')->get('name'));
+    $this->assertEquals('Drupal', \Drupal::configFactory()->getEditable('system.site')->get('name'));

@@ -647,6 +653,12 @@ public function testOverriddenConfigRemoved() {
+    // Confirm we did not save changes to the configuration.
+    $this->assertEquals('Labely label', \Drupal::configFactory()->get('system.menu.main')->get('label'));
+    $this->assertEquals('Main navigation', \Drupal::configFactory()->getEditable('system.menu.main')->get('label'));

Addresses #57, nice!

Details can be refined further later, what matters now is to get this in before alpha. @Berdir's feedback was addressed, moving back to RTBC.

@Wim: Yep #50.3 was wrong and has been wrong as far as I can remember. #access just makes form elements invisible, they're still there and processed by the form system, which means it simply falls back to the default value. It actually explicitly checks access, to not allow submitting a value for those. That's by design, otherwise the very common process of hiding some form elements in some cases with #access (often in form alter hooks) would result in suddenly losing/saving non-existing values.

I think it makes sense to do the loadOverrideFree() change here (thanks for doing that already), it just seems wrong to build the edit fom with overriden values even if we're then not actually going to save it. You never now what people are going to do with that form...

It actually explicitly checks access, to not allow submitting a value for those. That's by design, otherwise the very common process of hiding some form elements in some cases with #access (often in form alter hooks) would result in suddenly losing/saving non-existing values.

See, this is what I remembered.

Apparently #63 wasn't clear enough. The parts of the patch that I can comment on now look fine to me.

The server-side part should in theory work if the override does include the relevant cache contexts. Since you use a custom config override, you might have to clear caches there through the test. But I'll leave that part for Wim.

+++ b/core/modules/settings_tray/js/settings_tray.es6.js
@@ -171,6 +171,15 @@
+
+        if (instance.hasOwnProperty('element')) {
+          // If the ajax element is within a overridden block remove the link.
+          const disabled = $(instance.element).closest('[data-settings-tray-overridden]');
+          if (disabled.length === 1) {
+            $(instance.element).remove();
+          }
+        }
+

Couple of quick questions,

• Will disabled always only ever be one element?
• Can we save the result of $(instance.element) so we don't have to convert it to a jQuery object twice.
• There's an extra blank line, see padded-blocks

@tedbow Great. Thanks. Looks good to me.

They don't fire every time is edit or when new override is added.

Why is this a problem now? Why wasn't it a problem earlier?

This makes me ask a larger question: Shouldn't we then ensure Contextual Links are refreshed when necessary? If we can pass up-to-date information to some JS, we can also ensure that Contextual Links are re-rendered when necessary. This is similar to #2773591: New contextual links are not available after a module is installed.
The tricky bit here is that Contextual Links were originally designed for use cases where links very much remained the same; which links were visible mostly depended on the permissions you had as a user. permissions are one axis to vary by. But now each individual link may need to be become visible or invisible based on changes not in configuration, but in configuration overrides. And configuration overrides do not have detailed dependency information. They do have cacheability metadata. But they don't notify of individual overrides being added, removed or edited. So … sigh … yes, I think the interdiff #62 is probably appropriate. But please create a new issue in https://www.drupal.org/project/issues/drupal?version=8.x&component=conte... to document this need, and add a @todo pointing to that new issue in both settings_tray.es6.js and settings_tray_preprocess_block() to remove the work-arounds in Settings Tray when that issue gets fixed.

Once again: lack of full dependency information is the source of bugs :( :(

Patch review:

1. +++ b/core/modules/settings_tray/settings_tray.module
   @@ -54,6 +56,23 @@ function settings_tray_contextual_links_view_alter(&$element, $items) {
   +  //   and remove the need for this function.
   

   s/the need for this/this/

2. +++ b/core/modules/settings_tray/settings_tray.module
   @@ -93,10 +112,25 @@ function settings_tray_preprocess_block(&$variables) {
   +        // Add class and attributes to all blocks to allow Javascript to target.
   

   Nit: s/Javascript/JavaScript/

3. +++ b/core/modules/settings_tray/settings_tray.module
   @@ -93,10 +112,25 @@ function settings_tray_preprocess_block(&$variables) {
   +        // It not possible to remove the contextual link with
   +        // hook_contextual_links_alter or hook_contextual_links_view_alter
   +        // because they don't get called every time the block will be rendered
   +        // with possibly different configuration overrides.
   

   s/It not/It is not/

   s/because they […] overrides/because they are only called when the client-side cache of contextual links is empty, invalidated, or a new variation is detected, and it currently is not possible to reliably detect configuration override changes, and therefore it is not possible to invalidate the client-side cache of contextual links when needed./

4. Plus the two @todos requested earlier in the comment.

I updated #2937467: Access to certain contextual links may vary not by permissions, but by config overrides: add "config overrides" client-side cache tag based on what I wrote in #73, but with more detail.

Having written that, I realized that actually this patch is getting something wrong: contextual links are automatically omitted if the user does not have access to the route (see my update to #2937467 if you want more nuance). So the behavior we're adding here is to effectively make the entity.block.off_canvas_form route return a 403 if the block has config overrides.

But we're implementing it at the level of "contextual links have been rendered, now let's forcibly remove it in JS based on some metadata the server sends". The correct way to fix it would be to add a requirement to the entity.block.off_canvas_form route that forbids access if the block does have overrides.

So:

+++ b/core/modules/settings_tray/settings_tray.module
@@ -54,6 +56,23 @@ function settings_tray_contextual_links_view_alter(&$element, $items) {
+function _settings_tray_has_block_overrides(BlockInterface $block) {
+  // @todo Replace the following with $block->hasOverrides() in https://www.drupal.org/project/drupal/issues/2910353
+  //   and remove this function.
+  return \Drupal::config($block->getEntityType()->getConfigPrefix() . '.' . $block->id())->hasOverrides();
+}

turn this into an access check and use it for the entity.block.off_canvas_form route. And add test coverage that if you go to that URL directly (without using Settings Tray), you also get a 403. (This is a gap in the current test coverage.)

That would then solve the problem in a way that is A) more robust, B) future-proof: it will allow us to remove all hacks/work-arounds once #2937467: Access to certain contextual links may vary not by permissions, but by config overrides: add "config overrides" client-side cache tag lands.

We'll still need a work-around. But because it's now solved in a forward-compatible way, we can fix the problem in a different way: rather than relying on correct client-side cache invalidation that we don't yet have (that's the task of #2937467), we can fix the problem by … simply not ever client-side caching contextual links for blocks that have a Settings Tray link. We can find those in window.sessionStorage with IDs like:

Drupal.contextual.block:block=bartik_footer:langcode=en|menu:menu=footer:langcode=en|settings_tray::langcode=en

So, our work-around can simply be that whenever drupalContextualLinkAdded is called, we delete all entries in window.sessionStorage whose key starts with Drupal.contextual.block and contains settings_tray:.

Thanks to #2937467-8: Access to certain contextual links may vary not by permissions, but by config overrides: add "config overrides" client-side cache tag, I saw a much simpler solution, which still relies on that access check I mentioned in #75. I simply forgot about the metadata key in Contextual Links metadata.

(And yes, the docs for Contextual Links are terrible. That's at least partially my fault. With better docs, we'd have arrived at this solution before comment 20, instead of after comment 70. 😞)

Really curious to hear what you think about this, @tedbow!

#78:

So from my understanding from your description and looking the contextual module code is:
[…]
This sounds like good approach and really working with the contextual module how it works now.

Exactly!

I am not sure we would need #2937467 anymore this no longer feels like a workaround/hack.

Agreed.

This approach would take more client-side storage, though thinking still minimal and but have better client-side performance because it would be invalidating the client-side links less often.

Agreed.

#79:

1. D'oh, right. My bad! But that means the comment in settings_tray_preprocess_block() does need to be updated (expanded). NW for this.
2. I did just observe this locally as well. Good catch!

NW only for expanding the existing docs in settings_tray_preprocess_block(). Then this is RTBC again.

Oh, #83 just adds an injected service in favor of using \Drupal::. Works for me! Didn't care about this one really, because there's a @todo to remove it anyway.

Adjusting credit.

Hooray!

Ok, excellent!

Ted walked me through this patch three times now as a more "functional" / demo review. We found problems the first two times, but this last time everything worked as expected!

What the patch does is apply the same pattern we have for the Page Title block to any other block with configuration overrides. In other words, it hides the dotted border and removes the "Quick Edit" contextual link, in lieu of showing the user an error in the sidebar directing them to configure the block without Quick Edit. Then, for blocks with "sub"-configuration (such as menu blocks, and the site name block), applies a #access = FALSE to those elements only, still allowing Quick Edit of the remaining elements.

Here's a screenshot showing the three patterns: the primary menu doesn't have a translation, and therefore still has the dotted outline visible. The site name does have a configuration override, but also has other settings provided by the block plugin, so the dotted outline is still visible. And the "local tasks" menu is not quick editable, so shows only the contextual link.

When you click into e.g. the site branding block in English (without overrides), you see the block form in full:

If you click into it in Spanish (with overrides), the site name/slogan fields are removed, but you can still see the remainder of the settings:

Menu blocks work the same way, showing/hiding the menu link entries themselves.

This seems to strike the right balance between addressing the initial concerns of the issue, while utilizing existing UX patterns from elsewhere. (e.g. we don't let you click on a menu link that we know will lead to a 403.)

We also did a code walkthrough. The patch is much more simplified than earlier in this issue—great work, everyone! The addition of the context variant to work around local storage invalidation problems is great. Test coverage is quite extensive too and covers all of the edge cases Ted and I hit in testing.

THEREFORE!

Committed and pushed to 8.6.x, and since Settings Tray is still an experimental module (though hopefully not for too much longer :)), backported to 8.5.x as well.

Looks like it introduced test failures on PHP 5.5 & PostgreSQL 9.1:
https://www.drupal.org/pift-ci-job/864394
https://www.drupal.org/pift-ci-job/864395

Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testOverriddenConfigurationRemoved
Zumba\GastonJS\Exception\JavascriptError: One or more errors were raised in the Javascript code on the page.
            If you don't care about these errors, you can ignore them by
            setting js_errors: false in your Poltergeist configuration (see documentation for details).
AjaxError: 
An AJAX HTTP request terminated abnormally.
...
/var/www/html/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php:666