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 of LanguageNegotiationMethodInterface::getLangcode(), which returns a language ID, not a language object
  • WorkspaceNegotiatorInterface::setActiveWorkspace() is the equivalent of LanguageNegotiationMethodInterface::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

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:

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Issue summary: View changes
Status: Active » Needs review

amateescu’s picture

As for test coverage.. I don't think it's needed because this doesn't really change the behavior of workspace negotiators.

smustgrave’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Ran 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.

amateescu’s picture

Assigned: Unassigned » amateescu
Status: Reviewed & tested by the community » Needs work

Discussed a bit with @catch and I'll do a few changes to the MR.

amateescu’s picture

Assigned: amateescu » Unassigned
Status: Needs work » Needs review

Added a basic token validation for the query parameter workspace negotiator to comply with the interface description of the new method.

fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - looks great to me!

Issue summary could explain the approach with the token - for the most basic validation.

  • catch committed cd464a4e on 10.3.x
    Issue #3443761 by amateescu: Workspace negotiators shouldn't be...

  • catch committed d641512c on 10.4.x
    Issue #3443761 by amateescu: Workspace negotiators shouldn't be...

  • catch committed c84365f7 on 11.x
    Issue #3443761 by amateescu: Workspace negotiators shouldn't be...
catch’s picture

Version: 11.x-dev » 10.3.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Discussed 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.

  • catch committed c84365f7 on 11.0.x
    Issue #3443761 by amateescu: Workspace negotiators shouldn't be...

Status: Fixed » Closed (fixed)

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