Problem/Motivation

Someone in IRC was having problems with the theme.negotiator.visual_diff service (ended up just needing to clear caches).

While I was helping debug, I read the code in VisualDiffThemeNegotiator, and couldn't figure out why it extended AdminNegotiator from user.module. It overrides all public methods, and uses no helper methods.

Proposed resolution

Remove the dependency and implement ThemeNegotiatorInterface directly
Make isDiffRoute() protected as it is not on any interface
Improve the unit tests to cover each public method separately, and all code paths.

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#2 2825032-diff_negotiator-2.patch8.17 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
Status: Active » Needs review
FileSize
2825032-diff_negotiator-2.patch8.17 KB

Note that if the module was already stable, this would need an empty post_update hook to trigger a container rebuild, since diff.services.yml is changing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Needs review » Fixed

Wonderful, thx for pushing QA with cleanup and test coverage! Committed.

Status: Fixed » Closed (fixed)

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