Problem/Motivation
Workspaces / navigation support is implemented, but we also need to move toolbar support so we can deprecate toolbar.
Steps to reproduce
Proposed resolution
Move toolbar integration from Workspaces to Toolbar.
Remaining tasks
Review.
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
LLM disclosure
LLM was used to turn the removal into a move. With human review.
| Comment | File | Size | Author |
|---|
Issue fork drupal-3620812
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3620812-11-x
changes, plain diff MR !17064
- 3620812-remove-toolbar-support
changes, plain diff MR !16989
Comments
Comment #2
catchComment #4
catchThis also removes workspaces support from toolbar. It would be theoretically possible to move all that logic across but really think people can move to navigation if they want all bells and whistles.
Comment #6
amateescu commentedLooks good to me now :)
Comment #7
gábor hojtsyI was wondering the same by @catch above in #4 but I think its fine this way too.
Comment #8
gábor hojtsyI also looked at the scoping there was in #3618746: Toolbar uses in Announcement, User, Contextual originally, where this was broken out of. That was about moving the CSS along with the builders. This removes both so I think this is fine too. It would have been a problem if the CSS is removed but the builders were kept.
Comment #9
longwaveWhat if someone is using this functionality in 11.4 and upgrades to 11.5? Many users who started on 11.0 or earlier will still be using Toolbar and not Navigation, as it was experimental until 11.4. Can we only land this in main where toolbar will be removed?
Comment #10
longwaveComment #11
larowlanYeah I'm with @longwave on this, I was expecting to see the code from workspaces_ui moved into toolbar
Comment #12
gábor hojtsyOk changed it to move code instead of remove code which is a 30 net line code addition apparently.
11 files +55 −22Comment #13
gábor hojtsyComment #14
longwaveI think we could simplify by registering the lazy builder service always, and then it only gets used if Workspaces is installed, there is no harm in an unused service so conditionally enabling it is just an extra complication.
Comment #15
gábor hojtsyI went by how the existing
NavigationServiceProvideruses the conditional pattern fornavigation.workspaces_lazy_buildersalready, that is how it was done there :) But it does look like other core modules use a nullable argument yeah, so modified to that now.Comment #16
amateescu commentedAgreed that this approach is much more update friendly :)
Comment #18
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #19
catchCommitted/pushed to main, thanks!
This doesn't cherry-pick cleanly to 11.x, not sure we really need to, but if we do then will need a backport MR.
Comment #22
quietone commented11.x MR available
Comment #23
godotislateCompared 11.x MR diff to main commit diff, and they look essentially the same, other than adding
autowireto the service definition.Comment #24
catchCommitted/pushed to 11.x, thanks!