Problem/Motivation

Running database updates via the UI while having an active workspace in the current user's session leads to all kinds of interesting failures. One example is that post update functions can not save config entities because that's an unsupported operation in the context of a workspace.

Proposed resolution

Ensure that \Drupal\workspaces\WorkspaceManager::getActiveWorkspace() doesn't return a workspace object while running database updates.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

Release notes snippet

Nope.

CommentFileSizeAuthor
#6 3093879-6.patch6.7 KBamateescu
#6 3093879-6-test-only.patch5.49 KBamateescu
#2 3093879.patch8.09 KBamateescu
#2 3093879-test-only.patch3.48 KBamateescu

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 3093879-test-only.patch3.48 KB
new 3093879.patch8.09 KB

This should do it.

catch’s picture

catch
he/him
Primary language English
commented 
+++ b/core/modules/workspaces/src/WorkspaceManager.php
@@ -181,7 +193,10 @@ public function hasActiveWorkspace() {
    */
   public function getActiveWorkspace() {
-    if (!isset($this->activeWorkspace)) {
+    if ($this->kernel instanceof UpdateKernel) {
+      return FALSE;
+    }
+    elseif (!isset($this->activeWorkspace)) {

Is this the current way to figure out if we're in an update or not? I can't see similar anywhere else yet.

Don't we also need to do this in the migrate UI too for similar reasons?

The last submitted patch, 2: 3093879-test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 2: 3093879.patch, failed testing. View results

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 3093879-6-test-only.patch5.49 KB
new 3093879-6.patch6.7 KB

Thanks for the review!

That was my initial thought about handling this, but, after sleeping on it for a bit, I think a better way to do this is to ensure that no workspace negotiators run during database updates, similar to how we handle path aliases in \Drupal\Core\Update\UpdateServiceProvider::alter().

The migration part of this problem is being handled differently in #3052115: Mark an entity as 'syncing' during a migration update.

Also figured out a better way to test this, so here's a fresh set of patches.

catch’s picture

catch
he/him
Primary language English
commented

#6 looks a lot more like I was expecting.

The last submitted patch, 6: 3093879-6-test-only.patch, failed testing. View results

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/workspaces/tests/src/Functional/Update/WorkspacesUpdateTest.php
@@ -127,4 +127,33 @@ public function testWorkspaceParentField() {
+
+    // Check that we didn't have an active workspace while running the updates.
+    // @see workspace_update_test_post_update_check_active_workspace()
+    $this->assertFalse(\Drupal::state()->get('workspace_update_test.has_active_workspace'));

Previously, it would have made sense to make sure that we do have a value, but assertTrue/False are now type sensitive, so assertFalse(NULL) will fail, so that's good.

This looks good I think.

The last submitted patch, 6: 3093879-6-test-only.patch, failed testing. View results

The last submitted patch, 6: 3093879-6-test-only.patch, failed testing. View results

  • catch committed 528ab05 on 9.0.x 
    Issue #3093879 by amateescu, catch: Ensure that there's no active...

  • catch committed b71ccf5 on 8.9.x 
    Issue #3093879 by amateescu, catch: Ensure that there's no active...

  • catch committed 0a178af on 8.8.x 
    Issue #3093879 by amateescu, catch: Ensure that there's no active...
catch’s picture

catch
he/him
Primary language English
commented
Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.0.x, 8.9.x, 8.8.x, thanks!

  • alexpott committed 42145b4 on 9.0.x 
    Issue #3093879 hotfix: Ensure that there's no active workspace while...

  • alexpott committed 3e2c757 on 8.9.x 
    Issue #3093879 hotfix: Ensure that there's no active workspace while...

  • alexpott committed fa02126 on 8.8.x 
    Issue #3093879 hotfix: Ensure that there's no active workspace while...

jibran credited alexpott.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented 
@@ -25,108 +23,11 @@ class WorkspacesUpdateTest extends UpdatePathTestBase {
    */
   public function setDatabaseDumpFiles() {
     $this->databaseDumpFiles = [
-      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.filled.standard.php.gz',
+      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.8.0.filled.standard.php.gz',
       __DIR__ . '/../../../fixtures/update/drupal-8.6.0-workspaces_installed.php',
     ];
   }

I'm making the following change over in #3087644: Remove Drupal 8 updates up to and including 88** and it is complaing about no update hooks whereas test module with post-update hook is present and installed. Any quick pointer here or I'd debug in more detail.

Status: Fixed » Closed (fixed)

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