Problem/Motivation

The post update workspaces_post_update_move_association_data() makes an entity schema change. It deletes the workspace_association entity.

Generic entity type operations in hook_update_N implementations cannot depend on this update because it's a post update. If they are checking something on all entity types then the workspace_association entity type is completely invalid because neither the storage nor the entity type class exist.

As a whole workspaces_post_update_move_association_data() itself cannot be a hook_update_N because it is saving revisions.

Proposed resolution

Move the entity type deletion to an update hook so updates can depend on it.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 2-8-interdiff.txt1.7 KBalexpott
#8 3099986-8.patch4.81 KBalexpott
#8 2-8-interdiff.txt1014 bytesalexpott
#2 3099986-2.patch4.63 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
3099986-2.patch4.63 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: Move workspaces_post_update_move_association_data() to a hook_update_N » Move part of workspaces_post_update_move_association_data() to a hook_update_N
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

There is part of me that thinks maybe this is a hard problem. Any update that does generic operations on entity types is going to need to think about this. Maybe an alternative if for updates that do generic operations on all entity types to be more robust. Like at least check if the entity type class exists. I'm very not sure. It seems impossible for generic entity type updates to really work reliably. Hmmmm.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 
  1. +++ b/core/modules/workspaces/workspaces.install
@@ -174,3 +175,22 @@ function workspaces_update_8802() {
+ * @see workspaces_post_update_move_association_data()

    I don't think it's common practice to put @see comments in the doxygen block of update functions. I guess it won't end up in the text displayed to the users on the update screen because we only show the first line there, but it's still weird to have it there.. 

  2. +++ b/core/modules/workspaces/workspaces.post_update.php
@@ -31,20 +29,14 @@ function workspaces_post_update_remove_default_workspace() {
+  $tables = \Drupal::state()->get('workspaces_update_8803.tables');
+  if (!$tables) {

    We need to delete the state entry here and below where we drop the workspace_association tables. 

  3. +++ b/core/modules/workspaces/workspaces.post_update.php
@@ -31,20 +29,14 @@ function workspaces_post_update_remove_default_workspace() {
-  // Since the custom storage class doesn't exist anymore, we have to use core's
-  // default storage.

    It would be good to move this comment over to the new workspaces_update_8803() function.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

On the other hand, you're right in #4 that update functions that go through each entity type have to very very careful, because a contrib module for example wouldn't be so strict with an update like this one.. so I'm not sure this patch buys us anything in the end :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

One argument is favour of this change is that if you did hit this problem on your site at the moment you have no way of fixing it. With this patch you could use hook_update_dependencies() to force an update order and get around the problem.

I think there's no getting around the fact that

updates that go through all entity types have to be super careful no matter what

but there doesn't mean we shouldn't arrange core updates to be as helpful as possible is your site breaks because of this complexity.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
2-8-interdiff.txt1014 bytes
3099986-8.patch4.81 KB

Addressing #5

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
2-8-interdiff.txt1.7 KB

Now with the correct interdiff...

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Version: 8.9.x-dev » 8.8.x-dev
Status: Needs review » Reviewed & tested by the community

Thanks for the update, looks ready to go now :)

Not sure about the Major priority though, the problem is only theoretical at the moment and it covers only a very narrow list of update functions, so I'd be inclined to demote it to Normal, but the committer who will review this can decide to do that.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

So I agree we should do the schema change in hook_update_N() for the couple of reasons given above, but I'm wondering why we don't move the rest of the post update into the hook_update_N() and empty it out?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

@catch because it contains an entity save and we have no guarantees about the entity storage system working while hook_update_N is running.

  • catch committed 73dc06a on 9.0.x 
    Issue #3099986 by alexpott, amateescu: Move part of...

  • catch committed 4644239 on 8.9.x 
    Issue #3099986 by alexpott, amateescu: Move part of...

  • catch committed fb3b95b on 8.8.x 
    Issue #3099986 by alexpott, amateescu: Move part of...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Ahhh couldn't see it in the patch context but should've checked the actual update.

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

Status: Fixed » Closed (fixed)

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