Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The indicator in the toolbar showing the current workspace does not update after the workspace label is edited.
Reproduce by
- Install standard and enable workspaces module.
- Visit /admin/config/workflow/workspaces/manage/live/edit, change the label of the live workspace and save the change.
- The toolbar indicator still shows the old name.
The name in the indicator only updates after the cache is cleared.
Proposed resolution
TBD
Remaining tasks
All the things.
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#31 | interdiff-3006719-29-31.txt | 590 bytes | scott_euser |
#31 | workspace-cachetags-3006719-31.patch | 3.09 KB | scott_euser |
#29 | workspace-cachetags-3006719-29.patch | 3.11 KB | dhirendra.mishra |
#23 | workspace-cachetags-3006719-23.patch | 2.74 KB | dhirendra.mishra |
#20 | interdiff-3006719-17-20.txt | 1.93 KB | tetranz |
Comments
Comment #2
tetranz CreditAttribution: tetranz at Third and Grove commentedI think this will do it.
Comment #3
Eli-TJust tested the patch in #2 locally and it works.
Any reason this isn't just in the $items['workspace'] definition above, with #type, tab, #wrapper_attributes etc?
Comment #4
Eli-TScreenshot of patch in #2 showing immediate update of toolbar:
Comment #5
tetranz CreditAttribution: tetranz at Third and Grove commentedThanks and you're quite correct. I was about to do that first but thought it was going to override the #cache key at the top but now I see that we blow that away later.
I think it should retain the user.permissions context too.
New patch coming soon.
Comment #6
tetranz CreditAttribution: tetranz at Third and Grove commentedComment #7
Eli-TWith respect to retaining user.permissions; whilst this seems reasonable, I can't create the problem this is intended to solve. That is the patches in #2 and #6 seem to behave identically given the following steps.
The change seems reasonable but it seems to not be required unless there's a use case I'm missing?
Comment #8
tetranz CreditAttribution: tetranz at Third and Grove commentedYeah, I went through similar steps with similar results so I was unsure whether to leave it in. I should probably remove it.
Comment #9
tetranz CreditAttribution: tetranz at Third and Grove commentedComment #10
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@Eli-T, regarding the testing from #7: I think that you're not seeing any behavior change because some other part of core uses the
user.permissions
context in a render array, so it gets added to the page regardless of the (broken) implementation fromworkspaces_toolbar()
.As for the issue and the patch itself, great catch! :)
I think it would be better to put the new cache tag definition under
$items['workspace']['tab']
(just after#attributes
), and we should also fix the problem that we override the$items['workspace']
item defined at the top of the function, by using$items['workspace'] += [ ...
.Comment #11
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAlso, it would be really good to add some small test coverage for this bug in
\Drupal\Tests\workspaces\Functional\WorkspaceTest
.Comment #12
tetranz CreditAttribution: tetranz at Third and Grove commentedThanks @amateescu I can do that in the next few days if nobody else does but ...
If I put the cache tags definition under tabs rather than under workspace and then append the array rather than overwrite it, what should I do with the cache contexts definition already added to workspace? Should I leave them under workspace so we'll have contexts under workspace and tags under tabs?
Comment #13
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedYup, that's exactly what I would propose to do :)
Comment #14
tetranz CreditAttribution: tetranz at Third and Grove commentedOkay, starting again.
This is just a test which should fail.
Comment #15
tetranz CreditAttribution: tetranz at Third and Grove commentedThis should pass.
Comment #17
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedThis looks good to me! Just a couple minor, things I guess we can shorten directly into drupalPostForm() like attached patch / interdiff.
Comment #18
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI don't think we need to add another user to this test, we can simply add the
access toolbar
permission to the current list defined in thesetUp()
method and use$this->editor1
in the new test.show _the_ correct label :)
Same here, show _the_ new label.
Comment #19
dhirendra.mishra CreditAttribution: dhirendra.mishra at Srijan | A Material+ Company commentedi am uploading the patch which solves point 2 and point 3 from comment #18. Note point 1 is still remaining so i am not putting this under needs review status. I will work on that too soon if i get time in-between. Also someone can pick that remaining task by using my patch.Thanks.
Comment #20
tetranz CreditAttribution: tetranz at Third and Grove commentedI had this ready to go and just noticed #19 as I refreshed the page before uploading so I just renamed it. Sorry if that's an inappropriate thing to do.
Related to this, I wonder if the permissions are correct in workspaces.module in function workspaces_toolbar().
It seems a little odd that we need both 'view own workspace' and 'view and workspace'. I wonder the intention was to need either.
Comment #21
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@tetranz, note that the conditions are negated in
workspace_toolbar()
and they are checked with||
(OR), which means that a user needs either one of those three permissions, not all of them at the same time :)This test variable is not needed anymore.
We can drop these two permissions.
Comment #22
dhirendra.mishra CreditAttribution: dhirendra.mishra at Srijan | A Material+ Company commentedworking on it.
Comment #23
dhirendra.mishra CreditAttribution: dhirendra.mishra at Srijan | A Material+ Company commentedHere is the patch. Correction from #21
Comment #24
tetranz CreditAttribution: tetranz at Third and Grove commented@amateescu Are you sure about the permissions logic? :) Maybe my brain isn't quite in gear this hour of the morning but it looks to me that:
If you don't have 'administer workspaces' or don't have 'view own workspace' or don't have 'view any workspace' then the early return happens. i.e., it's effectively an AND thanks to De Morgan. That's why I added the other user because I didn't want to pollute the other tests too much but maybe this needs to be addressed too. I think it's missing an extra outer set of parenthesis that the original author intended.
A manual test confirms that you need all three before the toolbar appears. I think @dhirendra.mishra's patch will fail because of this.
I think that logic needs to be this but ... I'm not sure if we should do that in this issue or create a separate issue.
Comment #26
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@tetranz, I was 100% sure, and 100% wrong apparently :) I think it's fine to use the same issue to fix that as well, mostly for laziness-related reasons, but I if you want to open a new one please go ahead.
Comment #27
tetranz CreditAttribution: tetranz at Third and Grove commentedNo problem. :)
I can't do it right now so if you want do it @dhirendra.mishra before me then that's fine too although we really should have interdiffs with your patches. Hopefully that change won't break anything else but ... I guess we'll find out.
Comment #28
dhirendra.mishra CreditAttribution: dhirendra.mishra at Srijan | A Material+ Company commentedok.working on it.Thanks.
Comment #29
dhirendra.mishra CreditAttribution: dhirendra.mishra at Srijan | A Material+ Company commentedSorry as had some problem with interdiff. Just have changed the permission logic in workspace.module file as suggested in #24.
Please find patch below.
Comment #31
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedWe need to only return nothing if user doesn't have any of the permissions, so !1 && !2 && !3:
Tests should now pass
Comment #32
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAgreed, #31 accomplishes the same goal as the snippet from #24. Looks good to me now :)
Comment #33
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedAh yes, missed that: same as suggested in #24
Comment #34
dhirendra.mishra CreditAttribution: dhirendra.mishra at Srijan | A Material+ Company commentedOh Yes..Got it.. I had missed that. Thanks @scott_euser
Comment #36
dhirendra.mishra CreditAttribution: dhirendra.mishra at Srijan | A Material+ Company commentedComment #37
catchCommitted and pushed 03fdd37de2 to 8.7.x and f41ff929db to 8.6.x. Thanks!