Follow up to: #2904134: Settings Tray uses the off-canvas dialog type, but "off_canvas" is not an accurate form plugin name, "settings_tray" is

Problem/Motivation

Settings Tray uses the form handler key, 'off_canvas', for a block entity form handler. This ties Settings Tray functionality to a particular dialog type. Although Settings Tray uses the off-canvas dialog the need for the different form handler is the Settings Tray functionality of simplifying the Block entity edit from and bringing in related configuration.

It is totally possible the that another module in core or contrib would want to open up another version of the Block entity form in the off-canvas dialog, for instance as a quick way adjust visibility settings.

Proposed resolution

  1. Change the form handler key from off_canvas to settings_tray
  2. Change the route name from entity.block.off_canvas_form to entity.block.settings_tray_form
  3. Change the path to the form from /admin/structure/block/manage/{block}/off-canvas to /admin/structure/block/manage/{block}/settings-tray
  4. change the block entity link template from off_canvas-form to settings_tray-form

Remaining tasks

The patch

User interface changes

None

API changes

Internal only

Data model changes

None

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Issue summary: View changes
tedbow’s picture

Status: Active » Needs review
FileSize
4.58 KB
tedbow’s picture

Component: outside_in.module » settings_tray.module

Changing to new settings_tray.module component. @drpal thanks for script help! :)

tedbow’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.69 KB

Re-rolled.

tedbow’s picture

@Jo Fitzgerald thanks the re-roll looks good!

So this patch is pretty simple so I think just a review to get it to RTBC.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/settings_tray/settings_tray.module
@@ -86,8 +86,8 @@ function settings_tray_theme() {
-    ->setFormClass('off_canvas', BlockEntityOffCanvasForm::class)
...
+    ->setFormClass('settings_tray', BlockEntitySettingTrayForm::class)

I first thought this was a BC break, but it isn't, this is an implementation detail. Great, this is then finally completely consistent! :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2905098-6.patch, failed testing. View results

Wim Leers’s picture

Issue tags: +Needs reroll
error: patch failed: core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php:78
error: core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php: patch does not apply
Yogesh Pawar’s picture

Status: Needs work » Needs review
FileSize
4.71 KB

Rerolled the patch #6 because it's failed to apply on 8.5.x branch.

Yogesh Pawar’s picture

Issue tags: -Needs reroll
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, @Yogesh Pawar! I manually verified that #6 and #10 are equivalent.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Oops, this should have gone in before beta. Since it's in beta now and the route and link template are things contrib might interact with, I don't think we should backport it to 8.4.x.

Patch looks good. Can we also get a small change record for the same reason? Since these are things a contrib/custom module would conceivably interact with.

tedbow’s picture

Status: Needs work » Needs review

Add change record for form and link template keys? https://www.drupal.org/node/2929216

+++ b/core/modules/settings_tray/settings_tray.module
@@ -86,8 +86,8 @@ function settings_tray_theme() {
-    ->setLinkTemplate('off_canvas-form', '/admin/structure/block/manage/{block}/off-canvas');

+++ b/core/modules/settings_tray/settings_tray.routing.yml
@@ -1,8 +1,8 @@
-    _entity_form: 'block.off_canvas'
-    _title_callback: '\Drupal\settings_tray\Block\BlockEntityOffCanvasForm::title'
+    _entity_form: 'block.settings_tray'
+    _title_callback: '\Drupal\settings_tray\Block\BlockEntitySettingTrayForm::title'

Since this route is to an @internal form do we need a change record for this?

tedbow’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: change_block_entity-2905098-10.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Reviewed & tested by the community

DrupalCI Memory problem

larowlan’s picture

+++ b/core/modules/settings_tray/settings_tray.module
@@ -86,8 +86,8 @@ function settings_tray_theme() {
-    ->setFormClass('off_canvas', BlockEntityOffCanvasForm::class)
-    ->setLinkTemplate('off_canvas-form', '/admin/structure/block/manage/{block}/off-canvas');

Do we have to remove these keys? Can we retain both sets. I'm concerned removing them after this has been in the wild for most of the 8.4 cycle will be disruptive

We can retain the existing class, mark it deprecated and use trigger_error

Then we can communicate that it will be removed in the future.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: change_block_entity-2905098-10.patch, failed testing. View results

tedbow’s picture

Re #19 we could keep both the keys but I think we shouldn't because:

  1. Its still an experimental module so code that integrates with it should be prepared to be updated
  2. Keep both keys would be very confusing and I don't think would have any function. Settings Tray itself could only use 1 set to actually show the form in the off-canvas dialog. Would the only reason to leave is a module may have altered the form class for `off_canvas`.

    I think this is very unlikely but also if they altered it would most likely be with the intention of having Settings Tray use this form in the off-canvas dialog. So then we would get into situation of having to check if `off_canvas` key still after this change and if it was altered from `BlockEntityOffCanvasForm::class` then to use the new class instead. But then what if 1 module alters the deprecated `off_canvas` key and another modules alters the current `settings_tray` key? When would probably want to pick the current `settings_tray` key to respect but then what is the point of leaving `off_canvas` key as deprecated if we aren't going to respect it all the time.

My vote would be just rip the band-aid off and just switch to the new key.

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.75 KB

The patch also need a re-roll

larowlan’s picture

I'm more concerned about the link template than the form class

The reason to leave them is if someone else is displaying the form, or using the link template, it will stop working.

E.g.

// This would raise a RouteNotFoundException.
$link = $block->toLink($block->title(), 'off_canvas-form');

This would hard break that, which is something we shouldn't do for a beta module.

Added #2928750: Allow to deprecate routes as related for our eventual exit strategy.

So I think we need to, rename the class but keep the old one (subclass as an empty shell), mark it as deprecated and add a trigger_error.

And keep both old keys.

Then wait on #2928750: Allow to deprecate routes to deprecate the route name.

What do you think @xjm

Wim Leers’s picture

I agree with @larowlan — this is why I RTBC'd it in #8, in September, before 8.4.0 was released, i.e. when Settings Tray was still alpha.

Now that it's beta, we have to retain BC.

For deprecating routes, I think the issue that spawned [#29289750] settled on a nice solution, based on discussion between @dawehner and I. That issue is #2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent. What it does is:

  1. add the new route
  2. keep the old route, but make it a completely empty route definition
  3. this means you end up with a new route definition, and an old route definition that only contains a route name and is otherwise an empty shell
  4. add an outbound route processor that detects routes being generated for the old route name, and then automatically inherits the route definition from the new route, and does @trigger_error(…, E_USER_DEPRECATED)

This strategy reduces future maintenance burden, maximally signals that the old thing shouldn't be used anymore, yet keeps BC.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs review » Needs work

Alright, @tedbow has a day off today, so I'll implement what I proposed in #25 so he only needs to review and hopefully is comfortable RTBC'ing the route deprecation logic that I'll propose.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
Issue tags: +@deprecated
FileSize
5.65 KB
9.65 KB

Done. Includes test coverage proving that the old route name still generates a URL, that that URL matches the updated URL, and that an appropriate E_USER_DEPRECATED error is triggered.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

I was confused in #22 because I thought the proposal to was to also keep the form handler key.

Keeping the route key as deprecated seems like a good idea since the module already experimental beta.

The outbound route processor and the test coverage looks good.

Thanks @larowlan and @Wim Leers!

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: 2905098-27.patch, failed testing. View results

rajeshwari10’s picture

Status: Needs work » Needs review
FileSize
27.9 KB

Hi,

Applied the patch in #27 against Drupal core 8.6.x branch but it failed to apply. So created the patch with the changes against Drupal core 6.x branch. Please Review.

Status: Needs review » Needs work

The last submitted patch, 31: 2905098-31.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
9.79 KB

@rajeshwari10 thanks for the patch but it included unrelated changes.

When I am doing a reroll usually I check to make sure the patch file is about the same size the previous patch and then that it only modified the same files.
#31 is much bigger patch than #27

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

No changes just a reroll

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 33: 2905098-33.patch, failed testing. View results

MerryHamster’s picture

Changed `\Drupal::url` to `Url::fromRoute` in hook_help.
Added changes for applying the patch.

MerryHamster’s picture

Status: Needs review » Needs work

The last submitted patch, 36: 2905098-36.patch, failed testing. View results

tedbow’s picture

@MerryHamster(BTW love username ;) ) thanks for the rerolled patch.

  1. +++ b/core/modules/settings_tray/settings_tray.module
    @@ -19,12 +19,12 @@ function settings_tray_help($route_name, RouteMatchInterface $route_match) {
    -      $output .= '<p>' . t('The Settings Tray module allows users with the <a href=":administer_block_permission">Administer blocks</a> and <a href=":contextual_permission">Use contextual links</a> permissions to edit blocks without visiting a separate page. For more information, see the <a href=":handbook_url">online documentation for the Settings Tray module</a>.', [':handbook_url' => 'https://www.drupal.org/documentation/modules/settings_tray', ':administer_block_permission' => \Drupal::url('user.admin_permissions', [], ['fragment' => 'module-block']), ':contextual_permission' => \Drupal::url('user.admin_permissions', [], ['fragment' => 'module-contextual'])]) . '</p>';
    +      $output .= '<p>' . t('The Settings Tray module allows users with the <a href=":administer_block_permission">Administer blocks</a> and <a href=":contextual_permission">Use contextual links</a> permissions to edit blocks without visiting a separate page. For more information, see the <a href=":handbook_url">online documentation for the Settings Tray module</a>.', [':handbook_url' => 'https://www.drupal.org/documentation/modules/settings_tray', ':administer_block_permission' => Url::fromRoute('user.admin_permissions', [], ['fragment' => 'module-block']), ':contextual_permission' => Url::fromRoute('user.admin_permissions', [], ['fragment' => 'module-contextual'])]) . '</p>';
    ...
    -      $output .= '<p>' . t('To edit blocks in place, either click the <strong>Edit</strong> button in the toolbar and then click on the block, or choose "Quick edit" from the block\'s contextual link. (See the <a href=":contextual">Contextual Links module help</a> for more information about how to use contextual links.)', [':contextual' => \Drupal::url('help.page', ['name' => 'contextual'])]) . '</p>';
    +      $output .= '<p>' . t('To edit blocks in place, either click the <strong>Edit</strong> button in the toolbar and then click on the block, or choose "Quick edit" from the block\'s contextual link. (See the <a href=":contextual">Contextual Links module help</a> for more information about how to use contextual links.)', [':contextual' => Url::fromRoute('help.page', ['name' => 'contextual'])]) . '</p>';
    

    Thanks for catching this but we have to keep this issue scope limited.

    Take a look at https://www.drupal.org/core/scope#definition
    But generally

    Drupal core issues should include only a single scope, for example fixing a single bug with test coverage, or making one specific type of cleanup.

  2. +++ b/core/modules/settings_tray/settings_tray.services.yml
    @@ -7,3 +7,12 @@ services:
    diff --git a/core/modules/settings_tray/src/Block/BlockEntityOffCanvasForm.php b/core/modules/settings_tray/src/Block/BlockEntityOffCanvasForm.php
    
    diff --git a/core/modules/settings_tray/src/Block/BlockEntityOffCanvasForm.php b/core/modules/settings_tray/src/Block/BlockEntityOffCanvasForm.php
    deleted file mode 100644
    
    +++ /dev/null
    @@ -1,194 +0,0 @@
    diff --git a/core/modules/settings_tray/src/Block/BlockEntitySettingTrayForm.php b/core/modules/settings_tray/src/Block/BlockEntitySettingTrayForm.php
    
    diff --git a/core/modules/settings_tray/src/Block/BlockEntitySettingTrayForm.php b/core/modules/settings_tray/src/Block/BlockEntitySettingTrayForm.php
    new file mode 100644
    

    We should be able to get git to recognize this as rename instead of delete and then new file.
    This will greatly reduce the size of the patch which will help reviewers because they won't to check to sure the new and old files are actually the same.

    To do this you can do something like
    git diff -M25% 8.6.x > ../mypatchfile.patch

  3. +++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
    --- /dev/null
    +++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php.orig
    

    This file is just left over from the `patch` command probably.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
9.7 KB

Re-roll.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@Jo Fitzgerald looks good thanks

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks great from a consistency POV, and has a BC layer in case someone for some random reason was relying on these old names.

Committed and pushed to 8.6.x; cherry-picked to 8.5.x. Thanks!

  • webchick committed aa08b8c on 8.6.x
    Issue #2905098 by tedbow, Jo Fitzgerald, Wim Leers, MerryHamster, Yogesh...

  • webchick committed 3df4d82 on 8.5.x
    Issue #2905098 by tedbow, Jo Fitzgerald, Wim Leers, MerryHamster, Yogesh...

Status: Fixed » Closed (fixed)

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