Problem/Motivation
Workspace negotiators are currently required to ensure that the negotiated workspace actually exists, and that's problematic for a few reasons:
- modules that want to vary entire cache bins by a (negotiated) workspace ID can't do that because it happens super early in the request where an entity can't be loaded yet
- modules can't easily add additional requirements for validating a negotiated workspace, e.g. for implementing the notion of open/closed workspaces
Proposed resolution
In Drupal 10.3 add a new negotiation interface, with a single method: \Drupal\workspaces\Negotiator\WorkspaceIdNegotiatorInterface::getActiveWorkspaceId(), and trigger a deprecation from WorkspaceManager::getActiveWorkspace() for negotiators that don't implement it. Also, move the responsibility for ensuring that the negotiated actually exists to WorkspaceManager::getActiveWorkspace().
In Drupal 11 deprecate the newly introduced WorkspaceIdNegotiatorInterface, and remove it in Drupal 12.
This ensures that every negotiator will be prompted to implement the new method in Drupal 10.3 and 11, then we can safely add ::getActiveWorkspaceId() to the main interface \Drupal\workspaces\Negotiator\WorkspaceNegotiatorInterface.
An additional benefit of this change is that workspace negotiation will be more similar to language negotiation:
WorkspaceIdNegotiatorInterface::getActiveWorkspaceId()is the equivalent ofLanguageNegotiationMethodInterface::getLangcode(), which returns a language ID, not a language objectWorkspaceNegotiatorInterface::setActiveWorkspace()is the equivalent ofLanguageNegotiationMethodInterface::persist()
The URL workspace negotiatior, because the token comes from user input in the request, adds an additional token parameter (similar to the image style itok) so that we know that what has been passed in is actually a workspace ID or not, otherwise it wouldn't be able to validate the ID without trying to load a workspace, which would defeat the benefits of only having to return an ID.
Remaining tasks
Review.
User interface changes
Nope.
API changes
- \Drupal\workspaces\Negotiator\WorkspaceNegotiatorInterface::getActiveWorkspace() is deprecated, to be removed in Drupal 11.
Data model changes
Nope.
Release notes snippet
Nope.
Issue fork drupal-3443761
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:
- 3443761-10.3.x
changes, plain diff MR !7777
- 3443761-workspace-negotiators-shouldnt
changes, plain diff MR !7773
Comments
Comment #2
amateescu commentedComment #4
amateescu commentedAs for test coverage.. I don't think it's needed because this doesn't really change the behavior of workspace negotiators.
Comment #6
smustgrave commentedRan the test-only feature https://git.drupalcode.org/issue/drupal-3443761/-/jobs/1442858 and can see the coverage
MR 7777 correctly has the deprecations while MR 7773 has them removed (thanks!)
Scratched out one API change as that deprecation didn't appear to be there.
Code wise everything looks good to me.
Comment #7
amateescu commentedDiscussed a bit with @catch and I'll do a few changes to the MR.
Comment #8
amateescu commentedAdded a basic token validation for the query parameter workspace negotiator to comply with the interface description of the new method.
Comment #9
fabianx commentedRTBC - looks great to me!
Issue summary could explain the approach with the token - for the most basic validation.
Comment #13
catchDiscussed the token approach with @amateescu in slack and the end result looks good to me. Committed/pushed to 11.x and cherry-picked to 10.4.x and 10.3.x.