Problem/Motivation

The SwitchShortcutSet form injects the current_route_match service.

It actually uses it, but only to generate a link to the current page on the resulting (redirected) page. We now the route name of the current page so we can just specify it directly.

Proposed resolution

  • Replace the usage of $this->routeMatch->getRouteName() with shortcut.set_switch
  • Remove the injection of the current_route_match service from SwitchShortcutSet::create()
  • Remove the $route_match parameter including its usage from SwitchShortcutSet::__construct()
  • Remove the $routeMatch property on SwitchShortcutSet

Remaining tasks

User interface changes

None.

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because we currently have unnecessary coupling between different parts of the system
Issue priority Normal because nothing is broken
Unfrozen changes Not unfrozen
Prioritized changes Not prioritized
Disruption Because we are only removing arguments from a function (a constructor, in this case) there is no disruption even for sub-classes of SwitchShortcutSet who have to override the constructor
CommentFileSizeAuthor
#6 interdiff-1-6.txt1 KBroot_brute
#6 switchshortcutset_has-2449743-6.patch1.93 KBroot_brute
#1 switchshortcutset_has-2449743-1.patch2 KBroot_brute
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

root_brute’s picture

root_brute CreditAttribution: root_brute commented
Status: Active » Needs review
FileSize
switchshortcutset_has-2449743-1.patch2 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/shortcut/src/Form/SwitchShortcutSet.php
@@ -196,7 +187,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+        '@switch-url' => $this->url('shortcut.set_switch' , array('user' => $this->user->id())),

I'm curious, can't we just use $this->url('')?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Needs work

@root_brute Nice work!

Re #2: '' doesn't work, because a route name is expected, but <current> works, which I think is better.

So let's change it to <current>.

Also:

+++ b/core/modules/shortcut/src/Form/SwitchShortcutSet.php
@@ -50,9 +43,8 @@ class SwitchShortcutSet extends FormBase {
    * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
    *   The current route match.

Let's remove the docs, too.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

@root_brute Nice work!

Re #2: '' doesn't work, because a route name is expected, but <current> works, which I think is better.

So let's change it to <current>.

Also:

+++ b/core/modules/shortcut/src/Form/SwitchShortcutSet.php
@@ -50,9 +43,8 @@ class SwitchShortcutSet extends FormBase {
    * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
    *   The current route match.

Let's remove the docs, too.

root_brute’s picture

root_brute CreditAttribution: root_brute commented
Assigned: Unassigned » root_brute
root_brute’s picture

root_brute CreditAttribution: root_brute commented
Status: Needs work » Needs review
FileSize
switchshortcutset_has-2449743-6.patch1.93 KB
interdiff-1-6.txt1 KB

Removed the unnecessary docs and changed the route name to '<current>'.

root_brute’s picture

root_brute CreditAttribution: root_brute commented
Assigned: root_brute » Unassigned
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

Awesome, thanks so much @root_brute. Looks great and works, too! :-)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

I don't think form constructors should be considered an API considering we use the create method.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Added a beta evaluation. In this particular case, we're only actually removing an argument from the constructor, so I can't actually think of anything that could possibly break, even if you don't use create(), i.e. if you subclass SwitchShortcutSet (for whichever reason) and call parent::__construct().

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Thanks for the beta evaluation. Agreed that there's no impact here, and this also seems like it reduces fragility by removing an unneeded dependency.

Committed and pushed to 8.0.x. Thanks!

Welcome to the core team, root_brute! :)

  • webchick committed f07bcff on 8.0.x 
    Issue #2449743 by root_brute, tstoeckler: SwitchShortcutSet has an...

Status: Fixed » Closed (fixed)

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