Problem/Motivation

Deprecate toolbar module

Steps to reproduce

Proposed resolution

Move toolbar hook implementations to the toolbar module

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3611762

Command icon 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:

About issue forks

Comments

quietone created an issue. See original summary.

quietone opened merge request !16354

quietone’s picture

quietone commented
Status: Active » Needs review
nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Got some suggestions on the MR.

quietone’s picture

quietone commented
Status: Needs work » Needs review

@nicxvan, thanks.

I agree that removing 'Hook' is an improvement. And, like you it seems, on the fence about 'Toolbar' in the method name. Somehow, with these long methods, I think the name helps a bit.

meeni_dhobale’s picture

meeni_dhobale commented

Tested MR !16354 against 11.x (PHP 8.3).

The hook consolidation itself works: verified the toolbar renders identically before and after applying the MR (home, administration, announcement, contextual, user items all present with the same permissions/conditions as the original separate hook implementations), and the toolbar, contextual, and announcements_feed test suites pass.

However, this introduces a regression. ToolbarHooks::toolbar() gates the workspace item on $this->moduleHandler->moduleExists('workspaces'), but workspacesToolbar() depends on the workspaces_ui.lazy_builders service, which only exists when the workspaces_ui module is enabled. workspaces and workspaces_ui are separate installable modules (workspaces_ui depends on workspaces, not the reverse), so a site with only workspaces enabled will hit a fatal InvalidArgumentException: Class "workspaces_ui.lazy_builders" does not exist. the moment the toolbar renders. Before this MR that was safe, since the hook lived entirely inside workspaces_ui's own hook class and was simply never invoked unless workspaces_ui was enabled. I think the check needs to be moduleExists('workspaces_ui') instead.

But flagging this for a maintainer to confirm in case I'm missing some context on how these two modules are meant to interact.

quietone’s picture

quietone commented

@meeni_dhobale, thanks for catching that error. I think that is a simple oversight on my part and it should be workspaces_ui. I have made those changes.

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Hook snuck back into the name of the updated method. I added a new round of suggestions.

quietone’s picture

quietone commented
Status: Needs work » Needs review

Arg!

@nicxvan, thanks for making suggestions.

smustgrave’s picture

smustgrave commented

Left 1 question on the MR but looking good.