Problem/Motivation

#2815709: Settings tray conceptually incompatible with configuration overrides such as translations points out problems when using Settings Tray with blocks that have translation overrides. This problem will also existing with other configuration overrides.

For most blocks the overrides would be on the block entities themselves but for other blocks such as menu blocks that expose menu configuration it adds even more complexity.

Proposed resolution

For blocks that:

  1. Are currently overridden or
  2. if have related configuration they are exposing the ability to edit and that configuration is overridden

Then:

  1. Remove the "Quick edit" link
  2. Remove the ability to click and edit the blocks
  3. Remove the "Editable" styling classes and attributes

Original: Proposed Solution:

As a starting point to make the Settings Tray module stable @xjm, the Drupal 8 release manager, suggested that if a block has overrides we should not allow editing in the Settings Tray but rather give the user a message that it is not available to edit the Settings Tray and provide a link to the Block configuration form. This form would allow the user to configure other overrides.

Remaining tasks

Review patch

User interface changes

The "Quick edit" links for blocks that are overridden or have related configuration overrides would be removed.

API changes

New interface \Drupal\settings_tray\Form\BlockFormWithRelatedConfigInterface

'settings_tray' plugin forms would use denote they have related configuration and that form should not accessible if the configuration is currently overridden.

Data model changes

None

CommentFileSizeAuthor
#89 Screen Shot 2018-01-19 at 10.55.09 AM.png54.24 KBwebchick
#89 Screen Shot 2018-01-19 at 2.07.34 PM.png65.65 KBwebchick
#89 Screen Shot 2018-01-19 at 2.06.38 PM.png98.28 KBwebchick
#89 Screen Shot 2018-01-19 at 2.06.50 PM.png112.94 KBwebchick
#83 2919373-83.patch25.38 KBtedbow
#83 interdiff-81-83.txt2.85 KBtedbow
#81 2919373-81.patch23.46 KBtedbow
#81 interdiff-79-81.txt1.6 KBtedbow
#79 2919373-79.patch22.11 KBtedbow
#79 interdiff-76-79.txt2.63 KBtedbow
#76 2919373-76.patch21.49 KBWim Leers
#76 interdiff-74-76.txt7.54 KBWim Leers
#74 2919373-74.patch21.96 KBtedbow
#74 interdiff-71-74.txt3.21 KBtedbow
#71 2919373-71.patch21.4 KBtedbow
#71 interdiff-69-71.txt1.4 KBtedbow
#69 2919373-69.patch94.93 KBtedbow
#69 interdiff-67-69.txt1.54 KBtedbow
#67 2919373-67.patch22.42 KBtedbow
#67 interdiff-62-67.txt1.26 KBtedbow
#62 2919373-62.patch22.01 KBtedbow
#62 interdiff-60-62.txt4.28 KBtedbow
#60 2919373-60.patch20.96 KBtedbow
#60 interdiff-58-60.txt6.9 KBtedbow
#58 2919373-58.patch19.2 KBtedbow
#58 interdiff-54-58.txt6.61 KBtedbow
#54 2919373-53.patch17.49 KBtedbow
#54 interdiff-49-53.txt12.95 KBtedbow
#52 2919373-51.patch17.59 KBtedbow
#51 interdiff-49-51.txt13.43 KBtedbow
#51 2919373-51.txt17.59 KBtedbow
#49 2919373-49.patch22.09 KBtedbow
#49 interdiff-47-49.txt736 bytestedbow
#47 2919373-41.patch22.27 KBtedbow
#47 interdiff-34-41.txt17.03 KBtedbow
#34 2919373-34.patch14.88 KBtedbow
#34 interdiff-33-34.txt4 KBtedbow
#33 2919373-33.patch15.89 KBtedbow
#33 interdiff-32-33.txt9.04 KBtedbow
#32 2919373-32.patch11.31 KBtedbow
#32 interdiff-31-32.txt2.43 KBtedbow
#31 2919373-31.patch13.74 KBtedbow
#31 interdiff-26-31.txt5.13 KBtedbow
#30 Screen Shot 2018-01-12 at 2.08.41 PM.png83.22 KBwebchick
#29 interdiff-26-29.txt1.79 KBtedbow
#29 2919373-29.patch12.04 KBtedbow
#26 2919373-26.patch12.48 KBtedbow
#24 2919373-24.patch19.55 KBtedbow
#24 interdiff-22-24.txt4.51 KBtedbow
#22 2919373-22.patch19.65 KBtedbow
#22 interdiff-21-22.txt8.51 KBtedbow
#21 interdiff-19-21.txt5.52 KBAdita
#21 2919373-21.patch23.88 KBAdita
#19 2919373-19.patch23.84 KBtedbow
#19 2919373-19-REVIEW-do-not-test.patch16.77 KBtedbow
#16 2919373-16.patch30.72 KBtedbow
#16 2919373-16-REVIEW-do-not-test.patch19.54 KBtedbow
#14 interdiff-11-24.txt2.46 KBtedbow
#14 2919373-14_plus-2923004-8.patch25.51 KBtedbow
#14 2919373-14-do_not_test.patch15.75 KBtedbow
#11 2919373-11-do-not-test.patch15.49 KBtedbow
#11 2919373-11_plus-2923004-8.patch25.26 KBtedbow
#11 interdiff-8-11.txt5.59 KBtedbow
#8 2919373-8_plus-2923004-8.patch25.23 KBtedbow
#8 2919373-8-do-not-test.patch15.46 KBtedbow
#8 interdiff-6-8.txt3.3 KBtedbow
#6 2919373-6_plus-2923004-8.patch25.42 KBtedbow
#6 2919373-6-do-not-test.patch15.65 KBtedbow
#6 interdiff-5-6.txt11.37 KBtedbow
#5 2919373-5_plus-2923004-8.patch17.63 KBtedbow
#5 2919373-5-do-not-test.patch8.98 KBtedbow
#2 2919373-2.patch5.69 KBtedbow
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Status: Active » Needs review
FileSize
5.69 KB

Ok here is a start. It is not working and I am not sure how to make it work.

Here is what I have done so far.
Created to SettingsTrayServiceProvider to set a container parameter of all the services that are tagged with `config.factory.override`
Created `\Drupal\settings_tray\Block\BlockEntityOffCanvasForm::hasOverrides` to determine if the current block entity has overrides by calling `\Drupal\Core\Config\ConfigFactoryOverrideInterface::loadOverrides`. This does not work a block I know has a translation override.

The current code will call \Drupal\language\Config\LanguageConfigFactoryOverride::loadOverrides if config_translation is enabled but it will not find the override. It seems I would have to call \Drupal\language\Config\LanguageConfigFactoryOverride::setLanguage first.

If I do that then it will find the override but that doesn't help because we are trying to finding overrides for any modules so I can't assume that knownledge.

Ideas? Is the right direction?

bircher’s picture

tedbow’s picture

Title: Prevent Settings Tray functionality for blocks that over configuration overrides » Prevent Settings Tray functionality for blocks that have configuration overrides
tedbow’s picture

Ok. Here is different approach. The working approach takes the patch from here #2923004-8: Add method to check if any overrides are applied to \Drupal\Core\Config\Config.

I had to check in settings_tray_block_view_alter() to determine the block has overrides. We can't check in settings_tray_contextual_links_view_alter() because this won't be called every time there might be new override.

Also we can't check in \Drupal\settings_tray\Block\BlockEntityOffCanvasForm::form() because it will be on different route at this point and won't have the same overrides.

tedbow’s picture

This patch adds a test. It needed a couple changes to the JS because it was not compatible with our PhantomJS configuration.

drpal’s picture

Status: Needs review » Needs work
  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];
        }
      });
    }
    
tedbow’s picture

Status: Needs work » Needs review
FileSize
3.3 KB
15.46 KB
25.23 KB

@drpal thanks for the review!

1. Changed to the arrow function and destructed drupalSettings. I didn't destructure Drupal because not in the patch uses Drupal so it would affect 10 lines that have nothing to do with this issue.

2. Fixed

UPDATE: looks like the test failure was a problem with 8.5.x

Status: Needs review » Needs work

The last submitted patch, 8: 2919373-8_plus-2923004-8.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review

Setting to "Needs review" because tests passed.

tedbow’s picture

tedbow’s picture

Just some small fixes.

Wim Leers’s picture

  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).

tedbow’s picture

@Wim Leers thanks for the review!

1. This is because we now have black-magic.js in our vender directory!
Just kidding, possible in es6 with destructuring.

2. Added
3. I have broken this code up a bit to make it more readable. But maybe there is a bigger probelm?
4. Fixed

samuel.mortenson’s picture

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.

tedbow’s picture

@samuel.mortenson thanks for the review!

1. this is from the changes in #2923004: Add method to check if any overrides are applied to \Drupal\Core\Config\Config. I added the "REVIEW" patch to make it more clear what is needed for just this issue. When #2923004 get committed we can remove all that from this patch.
2. 😅 whoops yeah that whole surrounding if was testing and can be removed. Fixed.

I update this patch to use the latest patch from #2923004 which now uses hasOverrides().

Status: Needs review » Needs work

The last submitted patch, 16: 2919373-16.patch, failed testing. View results

xjm’s picture

+++ 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). :)

tedbow’s picture

Status: Needs work » Needs review
FileSize
16.77 KB
23.84 KB

@xjm whoops, thanks for catching that!

Fixed the patch in #16 because my 8.5.x was not up-to-date.

Also fixed "REVIEW" patch so it only has settings_tray related changes

Wim Leers’s picture

Status: Needs review » Needs work
  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/

Adita’s picture

Status: Needs work » Needs review
FileSize
23.88 KB
5.52 KB

#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

tedbow’s picture

@Adita thanks for the patch.

Reviewing #21
2. 👍
5, 👍
7. 👍
8. 👍
9. 👍
10 👍
12. There was still problem with the indentation. fixed
13. 👍

Addressing remaining #20

1. No longer an issue b/c JS removed in 3)
3.

I still think this is pretty scary.

@Wim Leers I respect your fear. Let see if we can fix this.

Looks like this is possible with hook_contextual_links_alter() probably in hook_contextual_links_view_alter() but better earlier
(also I forget about hook_contextual_links_alter() because for some reason it is in core/lib/Drupal/Core/Menu/menu.api.php and not contextual.api.php not sure why)

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

This also would allow a contrib module to use hook_contextual_links_view_alter() if it could create a good way to edit overridden blocks off-canvas dialog.

4.

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

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

6. Fixed

11. No reason just thinking of performance of one more module installed. But it should be fine to always have it enabled. Fixed.

Wim Leers’s picture

@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.

tedbow’s picture

#23

Nice! Do you also like this solution better?

I do like it better. I tried to reproduce problem I thought I had found before with contextual links not being render for a different language context. My previous comment in the code

+++ b/core/modules/settings_tray/settings_tray.module
@@ -58,6 +76,29 @@ function settings_tray_block_view_alter(array &$build) {
+    // 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();

This, especially the last sentence, seems not to be true.

  1. Change to "notice" because "warning" implies there is something wrong with an overridden block.
  2. Whoops. I messed this up. Fixing getBlockSelector() so I don't need the new method.
Wim Leers’s picture

Title: Prevent Settings Tray functionality for blocks that have configuration overrides » [PP-1] Prevent Settings Tray functionality for blocks that have configuration overrides
Status: Needs review » Postponed

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 :)

tedbow’s picture

Status: Postponed » Reviewed & tested by the community
FileSize
12.48 KB

#2923004: Add method to check if any overrides are applied to \Drupal\Core\Config\Config was committed!!!!
This #24 except with those changes removed.

Unpostponing, also setting to RTBC since @Wim Leers said

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 :)

Wim Leers’s picture

Title: [PP-1] Prevent Settings Tray functionality for blocks that have configuration overrides » Prevent Settings Tray functionality for blocks that have configuration overrides

RTBC++

xjm’s picture

+++ 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.

tedbow’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
12.04 KB
1.79 KB

#28 nope, removed

webchick’s picture

Priority: Normal » Major
Status: Needs review » Needs work
FileSize
83.22 KB

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

The general problem is as described in #2408549: There is no indication on configuration forms if 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. ;)

Text informing user they can't use the quick edit form.

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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
5.13 KB
13.74 KB

OK. So the thought from the discussion with @webchick was:

  1. For overridden blocks:
    1. Remove "Quick edit" link
    2. Remove the ability to clock in the blocks to open the Settings Tray
    3. Remove the class and attributes that make the block appear different when Edit mode.

    This patch does this.

  2. In addition Blocks should be considered overridden if the related configuration that Settings Tray form is exposing is overridden. The module only does this with the Site Branding block and menu blocks. This is more complicated this patch doesn't try this
  3. 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.

    I am not positive right now this didn't happen because I was debugging I am might have stopped a page request in the middle. I will look out for this but haven't seen it before except once months ago.

  4. - 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.

    This happen when I was demo'ing the problem now without the patch. So I edited a block that was currently being overridden with a translation. With any version of this patch so far you would not be able to do this.

tedbow’s picture

Since the patch no longer opens the tray at all for overridden forms then there is no need for the new route or controller

This patch removes them.

tedbow’s picture

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.

This patch handles this. It introduces \Drupal\settings_tray\Form\BlockFormWithRelatedConfigInterface
The 2 settings tray plugin forms implement it.

it simply has hasOverriddenConfig(). It should be to the block form to decide whether it should be consider overridden

Added test coverage for the site name being overridden. Then the branding block should not have "Quick edit".

Later when we tackle #2815709: Settings tray conceptually incompatible with configuration overrides such as translations we could decide that we want to actually let the block be edited and how it should behave.

tedbow’s picture

Self review

  1. +++ b/core/modules/settings_tray/settings_tray.module
    @@ -54,6 +58,32 @@ function settings_tray_contextual_links_view_alter(&$element, $items) {
    + * Checks if a block has overrides.
    + *
    + * @param \Drupal\block\BlockInterface $block
    + *   The block to check for overrides.
    + *
    + * @return bool
    + *   TRUE if the block has overrides otherwise FALSE.
    + *
    + * @internal
    + */
    +function _settings_tray_has_block_overrides(BlockInterface $block) {
    

    The comment and function name should be updated to indicate that it not only checks the block also related configuration.

  2. +++ b/core/modules/settings_tray/settings_tray.module
    @@ -54,6 +58,32 @@ function settings_tray_contextual_links_view_alter(&$element, $items) {
    +    // Check if the related config has overriddes
    

    Nit: period.
    Also update comment

  3. +++ b/core/modules/system/tests/src/FunctionalJavascript/OffCanvasTestBase.php
    @@ -9,6 +9,11 @@
    +  /**
    +   * CSS selector for the off-canvas dialog.
    +   */
    +  const OFF_CANVAS_CSS_SELECTOR = '.ui-dialog[aria-describedby="drupal-off-canvas"]';
    

    This change is no longer needed. Removing all changes to this file.

Status: Needs review » Needs work

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

tedbow’s picture

Status: Needs work » Needs review

Unrelated test fail in MediaSourceImageTest::testMediaImageSource()
Retesting

tedbow’s picture

Issue summary: View changes
tedbow’s picture

+++ b/core/modules/settings_tray/src/Form/BlockFormWithRelatedConfigInterface.php
@@ -0,0 +1,19 @@
+/**
+ * Any interface for forms with related configuration.
+ */
+interface BlockFormWithRelatedConfigInterface {
+
+  /**
+   * Determines if the related configuration is overridden.
+   *
+   * @return bool
+   *   TRUE if the related config for the form is in an overridden state
+   *   otherwise returns false.
+   */
+  public function hasOverriddenConfig();

This is the new interface I introduced so that it can be determined if the "Quick edit" functionality should be enable for the block. @see _settings_tray_has_block_related_overrides()

Right now it is connect to the idea of overridden configuration. My other idea was to just provide something more general like preventSettingsTrayQuickEdit that would return bool instead.

That was the 2 forms provided by the module could use this interface to prevent "Quick edit" being used based on overridden configuration but then contrib or other future core uses could prevent a particular block from being used with Settings Tray under other circumstances.

Note if a block should not be used with Settings Tray at all then it should use set the 'settings_tray' form handler to FALSE like is done in \Drupal\settings_tray_test\Plugin\Block\SettingsTrayFormAnnotationIsFalseBlock

Wim Leers’s picture

#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;.

Berdir’s picture

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

Then #2910353: Disallow saving config entities with applied configuration overrides 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.

Wim Leers’s picture

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.

Berdir’s picture

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.

Berdir’s picture

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?

Wim Leers’s picture

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.

Berdir’s picture

> 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: Disallow saving config entities with applied configuration overrides 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.

Wim Leers’s picture

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?

tedbow’s picture

#39 thanks for review.

Re:

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

After that comment I chatted with @xjm and commented below that solution in this issue sufficient enough for a first pass to mark the module stable because it will stop the user from seeing config values that are currently being overridden.

I wrote

I chatted with @xjm and she said first pass for this issue may be:

But I forgot explicitly say that this what would be needed for the module to be marked "stable". I will ask @xjm to comment on that issue or #2922603: Mark Settings Tray module as stable.
#2922603 only lists this issue and I know she has reviewed that.

#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.

I think we should actually change this a bit so won't need this API.

We should remove the "Quick edit" link if the just the related configuration is overridden. We should just add extra configuration fields. 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. This patch will do that. So I am removing the new interface.

So now #39 patch review.
1. thats better. fixed
2. why not check down to the nested level we are actually going to need?
3. No longer applies because interface removed.
4. No longer applies because interface removed.

#40

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

Isn't $this->configFactory->getEditable('system.site') low-level config. Or maybe I don't know the right terms.

tedbow’s picture

+++ b/core/modules/settings_tray/settings_tray.module
@@ -9,8 +9,12 @@
+use \Drupal\settings_tray\Form\BlockFormWithRelatedConfigInterface;
...
+use \Drupal\Core\Plugin\PluginWithFormsInterface;

forgot to remove these use statements.

tedbow’s picture

This patch is fixing use statements as per #48

#46
Yes before #30 we were showing the Settings Tray dialog when you clicked "Quick edit" but then giving you only:
The message "This block cannot be edited in the Settings Tray form because it has configuration overrides in effect."
The link to edit the block.

It was not handling the related configuration, either site name/slogan or menu, being overridden. This is what @xjm had suggested as to be before stable.

Wim Leers’s picture

Status: Needs review » Needs work

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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
17.59 KB
13.43 KB

@Wim Leers thanks for review!
1. fixed
2. fixing along with 5)
3-4 fixed
6. fixed
7. fixed
8. fixed

Yay a smaller patch!

tedbow’s picture

At least I am making new mistakes and not the same ones. 😅
Uploading with .patch extension

tedbow’s picture

+++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
@@ -622,6 +622,8 @@ public function testOverriddenConfigRemoved() {
+    $config = $this->config('system.site');
+    $config->set('name',$config->get('name'))->save();

Ok this is an old mistake, leaving in code that I was using to debug 😁

tedbow’s picture

And so it goes...

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

👍

Berdir’s picture

Status: Reviewed & tested by the community » Needs work
  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.

Berdir’s picture

+++ 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.

tedbow’s picture

Category: Task » Bug report
Status: Needs work » Needs review
FileSize
6.61 KB
19.2 KB

@Berdir thanks for review.
#56

  1. Added to @todo inside existing function.
  2. Changing to check AccessResult::allowedIf(!$site_config_immutable->hasOverrides('name') && !$site_config_immutable->hasOverrides('slogan'))->isAllowed()
  3. I create hasMenuOverrides() and checking that in submitConfigurationForm() and validateConfigurationForm(). Then changing the form element to use '#access' => AccessResult::allowedIf(!$this->hasMenuOverrides()),
  4. 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.

    I am changing here but would it actually make a difference? because with these updates it would never save the menu unless hasOverrides() is false.

#57
Added test coverage for saving the form when the elements have been removed. Changing to "bug"

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#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.

tedbow’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.9 KB
20.96 KB

@Wim Leers thanks for the RTBC but I found some more problems

  1. +++ b/core/modules/settings_tray/src/Form/SystemMenuOffCanvasForm.php
    @@ -115,7 +117,9 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
    +    if ($this->hasMenuOverrides()) {
    +      $this->getEntityForm($this->menu)->validateForm($form, $form_state);
    +    }
    
    @@ -123,8 +127,10 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
    +    if ($this->hasMenuOverrides()) {
    +      $this->getEntityForm($this->menu)->submitForm($form, $form_state);
    +      $this->menu->save();
    +    }
    

    I got these if statements wrong. They should both be
    if (!$this->hasMenuOverrides()) {
    If there are no overrides then we can validate and save the menu.

    So why weren't the tests failing. I checked manually and what happens in this situation is that the menu links just get disabled.

    @webchick this is what was causing the menu blocks to disappear when we were testing but still show on the block page. The block was there but since no links where enabled the block didn't render.

    So I put a test in to make sure the block and the link still renders.

    I hope it passes. It is randomly failing for me locally. Literally no code changes and pass then fail. I suspect our good friend phantomjs.

  2. +++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
    @@ -577,4 +585,107 @@ protected function getTestThemes() {
    +  public function testOverriddenConfigRemoved() {
    

    I split this up into 2 methods.

    testOverriddenBlock() which tests for when the block itself is overridden. I noticed we were only testing when the block was overridden before it was rendered the first time.

    And it testing it seems that if rendered a block then added a language override then it would not always have it "Quick edit" link remove but it would have the data-drupal-settingstray attribute removed.

    So I updated the new testOverriddenBlock() to display the block once and then do the override. This currently will fail 😒

Status: Needs review » Needs work

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

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.28 KB
22.01 KB

So I updated the new testOverriddenBlock() to display the block once and then do the override. This currently will fail 😒

Yay for predicting failure 😛

Ok. So no longer think it is possible to remove contextual links via the server-side contextual links relate hooks.
They don't fire every time is edit or when new override is added.

So I am now adding and attribute data-settings-tray-overridden in settings_tray_preprocess_block().

Then JS prepareAjaxLinks() I simply check the ajax element, which are already looping through is within a block with the attribute and if so remove the element. Don't worry @Wim Leers this Javascript is nothing like the scary JS I originally had in this patch 😜

Fingers crossed all tests should pass.

Berdir’s picture

@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...

Wim Leers’s picture

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.

Berdir’s picture

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.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tedbow’s picture

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

@Berdir thanks, I am sure it was clear enough for everyone but me 😜

This patch just adds a comment to settings_tray_preprocess_block() explaining what the data-settings-tray-overridden is and why we aren't using the contextual hooks.

drpal’s picture

Status: Needs review » Needs work
+++ 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’s picture

Status: Needs work » Needs review
FileSize
1.54 KB
94.93 KB

@drpal thanks for the review! 👊

Will disabled always only ever be one element?

yes, see https://api.jquery.com/closest/
Since it gets the ancestors in the DOM tree that matches it will always only get 1(or none). The only way it could get more is instance.element referenced more that 1 element then there would be other problems. So should only react to 1.

Can we save the result of $(instance.element) so we don't have to convert it to a jQuery object twice.

Fixed

There's an extra blank line, see padded-blocks

Fixed

Status: Needs review » Needs work

The last submitted patch, 69: 2919373-69.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
21.4 KB

Ok obviously I something up moving development against 8.6.x.

Rerolled #69

Also in process of the reroll I noticed

+++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
@@ -267,8 +271,10 @@ protected function assertOffCanvasBlockFormIsValid() {
+   * @param bool $has_confirm_form
+   *   Determines if the block form should be confirmed.
    */
-  protected function openBlockForm($block_selector, $contextual_link_container = '') {
+  protected function openBlockForm($block_selector, $contextual_link_container = '', $has_confirm_form = TRUE) {

This new @param $has_confirm_form is not needed anymore. It was added in an earlier version of this patch when we were showing the overridden notice in the Settings Tray and so not all instances had an actual form.

Removing

drpal’s picture

@tedbow Great. Thanks. Looks good to me.

Wim Leers’s picture

Status: Needs review » Needs work

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.
tedbow’s picture

Status: Needs work » Needs review
FileSize
3.21 KB
21.96 KB

#73

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

Yes this was probably always problem we just didn't test coverage that caught it.
\Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testOverriddenBlock now tests for this.

I created #2937467: Access to certain contextual links may vary not by permissions, but by config overrides: add "config overrides" client-side cache tag.
Currently I have a failing test there that proves that the contextual links are not alterable after you save a block with updated data.

The review
1. fixed
2. fixing but just to be clear this a existing line that just was surrounded in a new if statement
3. fixed
4. added todo's

Wim Leers’s picture

Status: Needs review » Needs work

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:.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.54 KB
21.49 KB

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!

Status: Needs review » Needs work

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

tedbow’s picture

@Wim Leers this great!

So from my understanding from your description and looking the contextual module code is:

  1. Contextual modules varies sets of contextual links based factors such as route parameters and language
  2. For each variation a will be cached client-side
  3. You can see this by looking at data-contextual-id for example `block:block=bartik_search&langcode=en|settings_tray::langcode=en`
  4. We can add our own variation by `has_overrides` key/value here
  5. So the client side will store new variation/combination that distinguishes a block when it has overrides and when it doesn't

This sounds like good approach and really working with the contextual module how it works now.

I am not sure we would need #2937467: Access to certain contextual links may vary not by permissions, but by config overrides: add "config overrides" client-side cache tag anymore this no longer feels like a workaround/hack.

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.

If we added #2937467: Access to certain contextual links may vary not by permissions, but by config overrides: add "config overrides" client-side cache tag and were to add a `config_overrides_hash` that would invalidate contextual links every time and config override was changed, whether or not you had Settings Tray installed.

there will be couple test failures. working on patch

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
22.11 KB

Just 2 problems that I see.

  1. In \settings_tray_preprocess_block() we still need to check !_settings_tray_has_block_overrides() or it will add the `settings-tray-editable` class and `data-drupal-settingstray` attribute to blocks that don't have a "Quick edit" link.
  2. In settings_tray_block_view_alter() we need to check isset($build['#contextual_links']['block']) before adding the has `has_overrides` metadata.

    This did not cause a test failure but I saw it locally when went to `admin/config/regional/language` in \_contextual_links_to_id() because the help block didn't have any contextual link.

Wim Leers’s picture

#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.

tedbow’s picture

#80 I am glad we agree!

Here is the updated comment for settings_tray_preprocess_block() that mentions overrides.

I will close now #2937467: Access to certain contextual links may vary not by permissions, but by config overrides: add "config overrides" client-side cache tag as Works as Designed. Someone can reopen if it is ever needed

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
tedbow’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.85 KB
25.38 KB

Just a couple small things we missed in \Drupal\settings_tray\Form\SystemMenuOffCanvasForm::hasMenuOverrides()

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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.

webchick’s picture

Adjusting credit.

  • webchick committed 4e66ea2 on 8.6.x
    Issue #2919373 by tedbow, Wim Leers, Adita, webchick, Berdir, drpal, xjm...

  • webchick committed 9aaf79d on 8.5.x
    Issue #2919373 by tedbow, Wim Leers, Adita, webchick, Berdir, drpal, xjm...
xjm’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Hooray!

webchick’s picture

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.

See above explanation.

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

Site name block in sidebar, with extra fields

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:

Site name block in sidebar, without extra fields

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.

tacituseu’s picture

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
tedbow’s picture

These are actually failing on Quick edit ajax callback. Currently Quick edit has no Functional Javascript test so there may be problem with testing on DrupalCI or something else going on. Hard to know because we have never tested it.

I created #2938308: Add QuickEdit Javascript tests

But first I think we should do #2938309: Only install Quick Edit when necessary for Settings Tray tests which I just created.

Quick edit was added to $modules because it is needed for \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testQuickEditLinks() but it doesn't need to be on for the rest.
It would not be bad idea to have it on for other tests to prove they work together but think that should not happen until after at least basic test for Quick edit in #2938309

UPDATED: I have uploaded a patch to #2938309: Only install Quick Edit when necessary for Settings Tray tests. I am testing the patch against all php/db combinations.

Status: Fixed » Closed (fixed)

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