Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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()
withshortcut.set_switch
- Remove the injection of the
current_route_match
service fromSwitchShortcutSet::create()
- Remove the
$route_match
parameter including its usage fromSwitchShortcutSet::__construct()
- Remove the
$routeMatch
property onSwitchShortcutSet
Remaining tasks
User interface changes
None.
API changes
None.
Beta phase evaluation
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 |
Comment | File | Size | Author |
---|---|---|---|
#6 | interdiff-1-6.txt | 1 KB | root_brute |
#6 | switchshortcutset_has-2449743-6.patch | 1.93 KB | root_brute |
#1 | switchshortcutset_has-2449743-1.patch | 2 KB | root_brute |
Comments
Comment #1
root_brute CreditAttribution: root_brute commentedComment #2
dawehnerI'm curious, can't we just use $this->url('')?
Comment #3
tstoeckler@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:
Let's remove the docs, too.
Comment #4
tstoeckler@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:
Let's remove the docs, too.
Comment #5
root_brute CreditAttribution: root_brute commentedComment #6
root_brute CreditAttribution: root_brute commentedRemoved the unnecessary docs and changed the route name to
'<current>'
.Comment #7
root_brute CreditAttribution: root_brute commentedComment #8
tstoecklerAwesome, thanks so much @root_brute. Looks great and works, too! :-)
Comment #9
alexpottThis 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.
Comment #10
tstoecklerAdded 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 subclassSwitchShortcutSet
(for whichever reason) and callparent::__construct()
.Comment #11
webchickThanks 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! :)