Problem/Motivation
Currently the UI refers to deploying a workspace but the code has a mixture of "deploy" and "publish".
Proposed resolution
All references to "deploy" should use "publish" instead.
Additionally, since we're editing all of the files/screens related to the action of publishing a workspace, we can take the opportunity to remove the Deploy
form mode and switch the workspace publish screen from an entity form to a regular confirmation form. The reason for doing this is that whenever a new field is added to the Workspace entity type, site builders need to take care and remove it from the Deploy
form mode, otherwise it will appear in the workspace publish confirmation form, which is not desired in most (all?) cases.
In #3062486: Add the ability to create sub-workspaces in order to enable the dev -> stage -> live workflow for content, we already introduced this pattern by making the "merge" operation a regular confirmation form instead of an entity form.
Remaining tasks
None.
User interface changes
Workspace list before:
Workspace list after:
Workspace toolbar before:
Workspace toolbar after:
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#43 | 2998454-8.9.x-backport.patch | 21.24 KB | amateescu |
#39 | workspace-toolbar-after.png | 29.36 KB | amateescu |
#39 | workspace-toolbar-before.png | 29.67 KB | amateescu |
#39 | workspace-list-after.png | 44.06 KB | amateescu |
#39 | workspace-list-before.png | 43.99 KB | amateescu |
Comments
Comment #2
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIt's the other way around :) "deploy" is the old term used for the "push these changes to Live", but we switched to "publish" in the meantime.
Comment #3
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAh! Okay, is that also the case for UI elements like buttons? Adjusting IS.
Comment #4
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedHere's a patch which moves all the terminology to
publish
instead ofdeploy
.I'm not sure yet whether this a good change to make, mostly because
WorkspaceDeployForm
was supposed to be generic and could be used for the merge operation as well, but I think that ship has sailed with #2958752: Refactor workspace content replication plugin system to three services and we need a new form for that operation anyway.Any other opinions are welcome! :)
Comment #6
GoldRerolled the patch against 8.7.x.
Comment #8
GoldHmm... That applied fine against 8.7.x for me here. :/
Will take another look at it in the morning.
edit: ...and it's tests that are failing. Will run those in the morning too.
Comment #9
GoldOkay, the tests are running again locally. Let's see if this one does the trick.
Comment #10
volkswagenchicktagging for badcamp 2018
Comment #12
volkswagenchickTagging for DrupalNorth 2019
Comment #13
volkswagenchickTagging for DrupalCamp Colorado 2019 (Sunday August 4)
Comment #14
volkswagenchickTagging for badcamp2019, thanks! (October 2-5)
Comment #16
dixon_The previous patch in #9 is unfortunately outdated and does no longer apply. I haven't had a time to take a closer look, but here's an alternative patch that all it does changes the terminology in the user interface (it doesn't rename any APIs, methods or anything like that).
Also marking this for Drupal 8.8 as I think it's relevant for the release before marking Workspaces stable.
Comment #17
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedComment #18
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWhile #16 was a good try before 8.8.0, we should do the full thing for 8.9.0 :)
Comment #19
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe patch needed a reroll for some reason..
Comment #21
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAfter some debugging on that test fail, it seems that
\Drupal\Core\Config\ConfigFactory::doGet()
creates a new config object if the requested one doesn't exist, so we need to change the assertions and try to load the config entities instead.Comment #22
dixon_I love this patch! It's a really good UX improvement because terminology is nw much more consistent. But it's also small code cleanup.
I manually tested the upgrade path and it's working well. Before applying the patch and running the updates I made some changes to the default workspace entity form. After applying the patch and running the updates my changes to the default form persists. Nice!
Code and tests look good. I think this is good to go!
Comment #24
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRandom fail :/
Comment #25
catchShould there not be an update to replace the configuration?
This looks like good clean-up but do we need some kind of best-effort bc-layer in here in case someone is subclassing?
Also what happens if someone is relying on the admin/configure/workspaces/manage/{workspace}/deploy route (say with a custom local task/action)?
Comment #26
dixon_@catch
There is an update function to deal with this. The update function deletes the form display (config entity) which cleans up after itself and updates any dependencies. I think this enough? Or maybe I am missing your point somehow? 🙂
See the update function below:
I agree we should always strive for BC, even in experimental mode. But I think the use cases for extending the deploy form in its current state are minimal. The form essentially acts more or less like a confirmation form. See the screenshot:
Same goes for this, I think the use cases for local tasks/actions are minimal on the deploy form. In fact, I can't really think of any reason why anyone would add that here.
IMO, I don't think we need a BC layer for this cleanup.
Putting this back to RTBC 😇
Comment #27
dixon_Tagging for a bit more visibility.
Comment #29
xjmOops, no longer applies.
Comment #30
abhisekmazumdarI have tried to update the patch.
Comment #31
abhisekmazumdarHello everyone
So this are things I have revomed from the patch on #21.
Removed this from the patch.
Removed this from the patch.
Comment #32
volkswagenchickTagging for upcoming contribution day at GovCon2020, dates Sept 23-25.
Thanks!
Comment #34
amateescu CreditAttribution: amateescu as a volunteer commented@abhisekmazumdar, those things should not have been removed from the patch, they contained the required post update function and its test coverage.
Here's a proper reroll of #21. The interdiff is quite heavy because we have to add back the helper fixture for installing the Workspace module "manually", which was removed in 9.0.0.
Also moving back to RTBC per #26.
Comment #35
catchNeeds some cspell ignore.
Comment #36
raman.b CreditAttribution: raman.b at OpenSense Labs commentedAdding cspell ignorePlease ignore this patch
Comment #37
raman.b CreditAttribution: raman.b at OpenSense Labs commentedShould've used
cSpell:ignore
instead 😓Comment #38
catchThis could do with an issue summary update explaining why the change from an entity form to a confirm form. Before I started reviewing the patch I was expecting just a wording change, but this is quite a bit more than that. Seems like a good change just wasn't expecting to see that given the issue title.
Could also use screenshots to show the UX change.
Comment #39
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedUpdated the issue summary, with screenshots and everything :)
Comment #41
catchThanks for the updates, that makes sense so...
Committed c01dee4 and pushed to 9.2.x. Thanks!
Comment #43
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedHere's a backport of this patch to 8.9.x in case someone who can't upgrade to D9 yet needs it.