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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

amateescu’s picture

I think all of the "publish" references in code should probably be transitioned over to using "deploy".

It'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.

Sam152’s picture

Issue summary: View changes

Ah! Okay, is that also the case for UI elements like buttons? Adjusting IS.

amateescu’s picture

Status: Active » Needs review
Issue tags: +Workflow Initiative
FileSize
16.73 KB

Here's a patch which moves all the terminology to publish instead of deploy.

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! :)

Status: Needs review » Needs work

The last submitted patch, 4: 2998454.patch, failed testing. View results

Gold’s picture

Rerolled the patch against 8.7.x.

Status: Needs review » Needs work
Gold’s picture

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

Gold’s picture

Okay, the tests are running again locally. Let's see if this one does the trick.

volkswagenchick’s picture

Issue tags: +badcamp 2018

tagging for badcamp 2018

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

volkswagenchick’s picture

Issue tags: +drupalnorth2019

Tagging for DrupalNorth 2019

volkswagenchick’s picture

Issue tags: +dcco2019

Tagging for DrupalCamp Colorado 2019 (Sunday August 4)

volkswagenchick’s picture

Issue tags: +badcamp2019

Tagging for badcamp2019, thanks! (October 2-5)

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

dixon_’s picture

Version: 8.9.x-dev » 8.8.x-dev
FileSize
4.72 KB

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.

amateescu’s picture

amateescu’s picture

Version: 8.8.x-dev » 8.9.x-dev
FileSize
23.52 KB

While #16 was a good try before 8.8.0, we should do the full thing for 8.9.0 :)

amateescu’s picture

The patch needed a reroll for some reason..

Status: Needs review » Needs work

The last submitted patch, 19: 2998454-19.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
22.84 KB
1.1 KB

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

dixon_’s picture

Status: Needs review » Reviewed & tested by the community

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!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2998454-21.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

Random fail :/

catch’s picture

Status: Reviewed & tested by the community » Needs review
  1. diff --git a/core/modules/workspaces/config/install/core.entity_form_display.workspace.workspace.deploy.yml b/core/modules/workspaces/config/install/core.entity_form_display.workspace.workspace.deploy.yml
    deleted file mode 100644
    index 201d636235..0000000000
    --- a/core/modules/workspaces/config/install/core.entity_form_display.workspace.workspace.deploy.yml
    +++ /dev/null
    @@ -1,15 +0,0 @@
    -langcode: en
    -status: true
    -dependencies:
    -  config:
    -    - core.entity_form_mode.workspace.deploy
    -  module:
    -    - workspaces
    -id: workspace.workspace.deploy
    -targetEntityType: workspace
    -bundle: workspace
    -mode: deploy
    -content: {  }
    -hidden:
    -  parent: true
    -  uid: true
    

    Should there not be an update to replace the configuration?

  2. +++ b/core/modules/workspaces/src/Entity/Workspace.php
    @@ -58,7 +57,6 @@
      * )
    diff --git a/core/modules/workspaces/src/Form/WorkspaceDeployForm.php b/core/modules/workspaces/src/Form/WorkspaceDeployForm.php
    
    diff --git a/core/modules/workspaces/src/Form/WorkspaceDeployForm.php b/core/modules/workspaces/src/Form/WorkspaceDeployForm.php
    deleted file mode 100644
    
    deleted file mode 100644
    index cb988ea887..0000000000
    
    index cb988ea887..0000000000
    --- a/core/modules/workspaces/src/Form/WorkspaceDeployForm.php
    
    --- a/core/modules/workspaces/src/Form/WorkspaceDeployForm.php
    +++ /dev/null
    
    +++ /dev/null
    +++ /dev/null
    @@ -1,165 +0,0 @@
    
    @@ -1,165 +0,0 @@
    -<?php
    -
    -namespace Drupal\workspaces\Form;
    -
    -use Drupal\Component\Datetime\TimeInterface;
    -use Drupal\Core\Entity\ContentEntityForm;
    -use Drupal\Core\Entity\EntityRepositoryInterface;
    -use Drupal\Core\Entity\EntityTypeBundleInfoInterface;
    -use Drupal\Core\Form\FormStateInterface;
    -use Drupal\Core\Messenger\MessengerInterface;
    -use Drupal\workspaces\WorkspaceAccessException;
    -use Drupal\workspaces\WorkspaceOperationFactory;
    -use Symfony\Component\DependencyInjection\ContainerInterface;
    -
    -/**
    - * Provides the workspace deploy form.
    - */
    -class WorkspaceDeployForm extends ContentEntityForm implements WorkspaceFormInterface {
    -
    -  /**
    

    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)?

dixon_’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
137.12 KB

@catch

Should there not be an update to replace the configuration?

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:

+/**
+ * Remove the 'deploy' form mode and form display.
+ */
+function workspaces_post_update_remove_deploy_form_mode_and_form_display() {
+  if ($form_display = EntityFormDisplay::load('workspace.workspace.deploy')) {
+    $form_display->delete();
+  }
+  if ($form_mode = EntityFormMode::load('workspace.deploy')) {
+    $form_mode->delete();
+  }
+}
This looks like good clean-up but do we need some kind of best-effort bc-layer in here in case someone is subclassing?

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:

screenshot

Also what happens if someone is relying on the admin/configure/workspaces/manage/{workspace}/deploy route (say with a custom local task/action)?

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 😇

dixon_’s picture

Issue tags: +WI critical

Tagging for a bit more visibility.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Oops, no longer applies.

abhisekmazumdar’s picture

Status: Needs work » Needs review
FileSize
20.27 KB

I have tried to update the patch.

abhisekmazumdar’s picture

Hello everyone

So this are things I have revomed from the patch on #21.

  1. +++ b/core/modules/workspaces/tests/src/Functional/Update/WorkspacesUpdateTest.php
    @@ -121,10 +122,21 @@ public function testWorkspaceParentField() {
    +  public function testWorkspaceDeployFormRemoval() {
    

    Removed this from the patch.

  2. +++ b/core/modules/workspaces/workspaces.post_update.php
    @@ -143,3 +144,15 @@ function workspaces_post_update_update_deploy_form_display() {
    +function workspaces_post_update_remove_deploy_form_mode_and_form_display() {
    

    Removed this from the patch.

volkswagenchick’s picture

Issue tags: +GovCon2020

Tagging for upcoming contribution day at GovCon2020, dates Sept 23-25.
Thanks!

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -badcamp 2018, -drupalnorth2019, -dcco2019, -badcamp2019, -GovCon2020
FileSize
209.44 KB
188.17 KB

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

catch’s picture

Status: Reviewed & tested by the community » Needs work

Needs some cspell ignore.

raman.b’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
209.48 KB
820 bytes

Adding cspell ignore

Please ignore this patch

raman.b’s picture

Should've used cSpell:ignore instead 😓

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update, +Needs screenshots

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

amateescu’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update, -Needs screenshots
FileSize
43.99 KB
44.06 KB
29.67 KB
29.36 KB

Updated the issue summary, with screenshots and everything :)

  • catch committed c01dee4 on 9.2.x
    Issue #2998454 by amateescu, raman.b, Gold, dixon_, catch, Sam152: Make...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the updates, that makes sense so...

Committed c01dee4 and pushed to 9.2.x. Thanks!

Status: Fixed » Closed (fixed)

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

amateescu’s picture

FileSize
21.24 KB

Here's a backport of this patch to 8.9.x in case someone who can't upgrade to D9 yet needs it.