Remember the page you were on and take you back there when switching Workspaces

I was very confused by this indeed.

Let me know if you need help with

- Find a way to bypass the caching of the link in hook_toolbar

Here is an initial working patch. One note is, the cache context never used because the $item array was missing +

@@ -162,12 +163,12 @@ function workspace_toolbar() {
   /** @var \Drupal\workspace\WorkspaceInterface $active_workspace */
   $active_workspace = \Drupal::service('workspace.manager')->getActiveWorkspace();

-  $items['workspace'] = [
+  $items['workspace'] += [

Tested #3 manually and it's working great!

Adding tests (test-only file is inter-diff)...

Fix component following module rename.

updated patch for the change in #10.

Not sure why it's failing on toolbar visibility but both test cases are green locally.

Turns out we found a bug (#2987956: Entity queries with sorting and ordering are not working in a non-default workspace) as part of the tests here.

1. We are not getting user.permissions context because toolbar has content specific to the user (i.e. username) which is more granular. reference from docs:

Drupal automatically uses the hierarchy information to simplify cache contexts as much as possible. For example, when one part of the page is varied per user (user cache context) and another part of the page is varied per permissions (user.permissions cache context), then it doesn't make sense to vary the final result (e.g.: the page) per permissions, since varying per user is already more granular.
In other words: optimize([user, user.permissions]) = [user].

2. I believe test failure because of width wasn't set.

3. Added some access tests.

The off-canvas test looks really good to me. Here's a review for the other parts:

1. +++ b/core/modules/workspaces/tests/src/Functional/WorkspaceAccessTest.php
   @@ -0,0 +1,188 @@
   +  public function testWorkspaceAccess() {
   

   This method seems to assert only cache contexts, so not really "access" parts. Can we move all the cache context assertions to the existing WorkspacePermissionsTest test?

2. +++ b/core/modules/workspaces/tests/src/Functional/WorkspaceAccessTest.php
   @@ -0,0 +1,188 @@
   +  public function testWorkspaceToolbarAccess() {
   

   I'm not sure what exactly is being tested here, that a hook_toolbar() implementation forces a user cache context on every page?

   If so, \Drupal\Tests\toolbar\Functional\ToolbarCacheContextsTest::testToolbarCacheContextsCaller() already provides test coverage for that, so I think we can remove this test class altogether :)

3. +++ b/core/modules/workspaces/tests/src/FunctionalJavascript/WorkspaceToolbarIntegrationTest.php
   @@ -0,0 +1,86 @@
   +      'access content overview',
   

   Is this permission really needed?

4. +++ b/core/modules/workspaces/tests/src/FunctionalJavascript/WorkspaceToolbarIntegrationTest.php
   @@ -0,0 +1,86 @@
   +    // Make sure stay on same page after switch.
   

   Missing a 'we' between 'sure' and 'stay' :P

5. +++ b/core/modules/workspaces/workspaces.module
   @@ -144,13 +145,6 @@ function workspaces_cron() {
   -  $items['workspace'] = [
   -    '#cache' => [
   -      'contexts' => [
   -        'user.permissions',
   -      ],
   -    ],
   -  ];
   

   I don't think this should be removed, our hook_toolbar() implementation still depends on the user's permissions, and the fact that other implementations like shortcut_toolbar() add the more generic user context should not affect us in any way.

Thanks for the review @timmillwood & @amateescu. Here is an update for #22

#22.1 - Moved
#22.2 - Removed
#22.3 - Removed
#22.4 - Fixed
#22.5 - Added back. but the "user" context is granular and "user.permissions" is generic. However, I see there is no harm in having it.

+++ b/core/modules/workspaces/tests/src/Functional/WorkspacePermissionsTest.php
@@ -161,16 +170,19 @@ public function testDeleteOwnWorkspace() {
     $this->drupalGet("/admin/config/workflow/workspaces/manage/{$packers->id()}/delete");
...
+    $this->assertCacheContext('user');
...
     $this->drupalGet("/admin/config/workflow/workspaces/manage/{$bears->id()}/delete");
...
+    $this->assertCacheContext('user.permissions');

These cache context are different even though we are basically visiting the same page, so I wonder what exactly are we testing with them?

+++ b/core/modules/workspaces/tests/src/Functional/WorkspacePermissionsTest.php
@@ -161,16 +170,19 @@ public function testDeleteOwnWorkspace() {
     $this->drupalGet("/admin/config/workflow/workspaces/manage/{$packers->id()}/delete");
...
+    $this->assertCacheContext('user');
+    $assert_session->statusCodeEquals(200);
...
     $this->drupalGet("/admin/config/workflow/workspaces/manage/{$bears->id()}/delete");
...
+    $this->assertCacheContext('user.permissions');
+    $assert_session->statusCodeEquals(403);

It's just matter of what's been rendered on that page. first one is 200 which renders something user specific and the second one is 404. just permission level.

Got it! I don't have anything else to complain about, great work :)

1. +++ b/core/modules/workspaces/tests/src/FunctionalJavascript/WorkspaceToolbarIntegrationTest.php
   @@ -0,0 +1,85 @@
   +    $this->assertNotEmpty($this->assertSession()->waitForElement('css', 'body.toolbar-horizontal'));
   ...
   +    $this->assertTrue($this->assertSession()->elementExists('css', '.workspaces-dialog'), 'Workspace canvas is not visible after closing.');
   ...
   +    $this->assertNotEmpty($this->assertSession()->waitForElement('css', 'body.toolbar-horizontal'));
   ...
   +    $this->assertSession()->waitForElementVisible('css', '.messages--status');
   ...
   +    $this->assertSession()->responseContains('<em class="placeholder">Stage</em> is now the active workspace.');
   +    $this->assertSession()->addressEquals('admin');
   

   can we use some local variables here for assertSession() to avoid creating new objects each time?

2. +++ b/core/modules/workspaces/workspaces.module
   @@ -162,12 +161,12 @@ function workspaces_toolbar() {
   +      '#url' => $active_workspace->toUrl('collection', ['query' => ['destination' => Url::fromRoute('<current>')->toString()]]),
   

   we should use ['query' => \Drupal::destination()->getAsArray()] here

Thanks @larowlan for the review :) Addressed #30 and back to RTBC since they were very simple changes.

Fixed the failing test because of the more generic cache context.

Fixed the typo.

Discussed with @jeqq and he told me that we have a test module in core specifically for removing the user item from the toolbar (toolbar_disable_user_toolbar.module), and we could've used it in our test, but he went with the approach of testing both cases for better coverage, and I agree :)

Unrelated error in the tests for the new composer scaffold component.

@alexpott reviewed this patch and asked why do we need all the changes to WorkspacePermissionsTest. Since this patch does not change anything related to cache context, I agree with him that those extra assertions are not really in scope of this issue.

So here's a reroll of #35 with all the changes to WorkspacePermissionsTest reverted.

Reviewing the patch again, the changes to WorkspaceSwitcherTest and WorkspaceTestUtilities are also not in scope, so I've removed them as well.

Adjusting credits.

I verified the removed test parts are indeed unrelated. The patch looks good otherwise and got reviews from @larowlan and @alexpott already.

Committed c2d897e and pushed to 8.8.x. Thanks!

Should be merged to 8.7 once that is out of freeze.

Cherry-picked to 8.7.x! Thanks again!

I recently started trying out the Workspaces module since it seems like something that would be very helpful for our process. The destination that is appended to the Live/Stage URLs appears to be cached for me. So, I'll be on a page, switch my Workspace, and then taken back to some page I was on previously.

My expectation is that the destination should be correctly updated to point to the page that the user is on.

I haven't had time to dig into that to see if it's specific to my instance since it's somewhat low priority (although frustrating), but I wanted to post a comment here in case anyone else is seeing that too.

Not sure if this is the best way to go about it, but it was quick and got the job done, without having to waste a lot of time debugging. I also didn't bother spending the time to get into why it wasn't working to begin with since I don't have that kind of time for the project I'm working on right now, and I just need to move forward. This worked for me. I hope it helps anyone else running into this same problem.

I implemented this hook in a custom module I have in my Drupal instance:

/**
 * Implements hook_toolbar_alter.
 */
function YOUR_MODULE_toolbar_alter(&$items) {
  $items['workspace']['tab']['#cache']['max-age'] = 0;
}

Make sure to clear your cache once after adding that hook function, and you should see it working with the proper destination being appended.