Problem/Motivation

Brought up by @Fabianx in #2784921-175: Add Workspaces experimental module:

The use case is:

- Send out an email with new content to review (that includes the right workspace parameter).
- Give someone a link for reviewing content-in-progress (e.g. during localization)

Proposed resolution

Bring back the query parameter negotiator from the 8.x-1.x branch.

Remaining tasks

Do it!

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

CommentFileSizeAuthor
#10 interdiff-2968875-10.txt1.41 KBtimmillwood
#10 2968875-10.patch3.76 KBtimmillwood
#8 2968875-8.patch3.57 KBtimmillwood
#8 interdiff-2968875-8.txt3.03 KBtimmillwood
#3 2968875.patch1.77 KBamateescu

Comments

amateescu created an issue. See original summary.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented

As a bit of background here. The 1.x version of the module didn't have the query parameter negotiator, this was added early on in the 2.x development as a way to implement the UI design and preview each workspace with an iframe for each workspace using the query parameter. This didn't really work out and was postponed until we could work out if it was needed or not.

The email use case for the query parameter is a great one but would need to retained in the user's session unlike the original implementation where once a link was clicked the query parameter would be lost and other negotiators would be used to determine the active workspace.

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 2968875.patch1.77 KB

@timmillwood, right, this means we need to depend on the session negotiator and use it to persist the workspace ID received as a query param.

This should do it :)

Edit: the patch doesn't apply to the contrib branch, it's written on top of the core patch from #2784921: Add Workspaces experimental module.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented

I don't think this would set the workspace in the query to the session. When visiting http://www.example.com/?workspace=foo \Drupal\workspace\Negotiator\SessionWorkspaceNegotiator::setActiveWorkspace() isn't called, is it?

Maybe we need an update to WorkspaceSwitcherTest or something to test this? :)

amateescu’s picture

amateescu commented

I don't think this would set the workspace in the query to the session. When visiting http://www.example.com/?workspace=foo \Drupal\workspace\Negotiator\SessionWorkspaceNegotiator::setActiveWorkspace() isn't called, is it?

Right! Which means we need to call it in \Drupal\workspace\WorkspaceManager::getActiveWorkspace right before the break; statement.. I think.

Of course, this would need tests as well :)

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Project: Workspace » Drupal core
Version: 8.x-2.x-dev » 8.6.x-dev
Component: Code » workspace.module
timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Assigned: Unassigned » timmillwood
Issue tags: +Needs tests 
+++ b/core/modules/workspace/workspace.services.yml
@@ -16,6 +16,11 @@ services:
+    class: Drupal\workspace\Negotiator\SessionWorkspaceNegotiator

How did I miss this!!?!?!

Working on it!

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Assigned: timmillwood » Unassigned
Issue tags: -Needs tests
StatusFileSize
new interdiff-2968875-8.txt3.03 KB
new 2968875-8.patch3.57 KB

Fixing #7 and adding a test. Although the test fails because we cache the active workspace in the $activeWorkspace property of the WorkspaceManager.

Status: Needs review » Needs work

The last submitted patch, 8: 2968875-8.patch, failed testing. View results

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Status: Needs work » Needs review
StatusFileSize
new 2968875-10.patch3.76 KB
new interdiff-2968875-10.txt1.41 KB

Rather than removing the caching I updated the test to assert the page rather than the return value from the cache.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

RTBC - Looks good to me.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 716f73d and pushed to 8.6.x. Thanks!

  • catch committed 716f73d on 8.6.x 
    Issue #2968875 by timmillwood, amateescu: Bring back the query parameter...

Status: Fixed » Closed (fixed)

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

amateescu’s picture

amateescu commented
Component: workspace.module » workspaces.module

Fix component following module rename.