Add an update path test and fix any bugs it finds

It's related to the change in Drupal 8.7: https://www.drupal.org/node/3034742

I'm wondering how that should be dealt with in general. Meaning now, when I'm upgrading to Drupal 8.7 it's kind of easy to go back to 8.6.x, update the affected module (here "consumers"), run the update hooks and then perform the core update to 8.7 afterwards.

But what happens in 6 months, when the sites have been on 8.7 for a while and then a contrib module (which was not updated for some time for some reason) is going to be updated, which includes entity type schema updates. It's probably not the best practice to change update hooks for the past, but I guess it would work. The problem I see is, that the maintainers usually update their modules much more frequent, so they will not encounter an issue like this. I'm afraid this could be troublesome for a couple of contrib modules.

I guess it could be argued, that contrib should be kept up-to-date and it might be recommended to update contrib before doing a core upgrade. But those problems will happen.

I'm hitting this as well. I'm bumping this to Major (personally, I think it should be Critical) because it completely breaks the update path to Drupal 8.7.

Here is a quick, sloppy attempt at a patch. Not sure if this will work, but I'll test it.

Trying to eliminate a new error ("Unsupported entity type")...

I went to @Berdir and @amateescu for advice on this, since they understand the entity update system better than just about anybody.

Here's how our conversation in Slack went (some comments removed for brevity):

phenaproxima (he/him)   [16 minutes ago]
Hey! I was wondering if I could get any guidance from you about https://www.drupal.org/project/consumers/issues/3052959

phenaproxima (he/him)   [16 minutes ago]
I tried to write a patch, but it didn’t really work all that well. I don’t really grok this entity-update stuff

phenaproxima (he/him)   [16 minutes ago]
But I know you were heavily involved in it :slightly_smiling_face:

amateescu   [16 minutes ago]
let's see

phenaproxima (he/him)   [16 minutes ago]
The tl;dr of it is that update 8013 (which has existed for a long time) doesn’t work on 8.7.0 due to changes in the entity update system.

berdir   [15 minutes ago]
yeah tricky, because we can't really control if users update before/after updating to 8.7 :confused:

phenaproxima (he/him)   [15 minutes ago]
@berdir Yup :slightly_smiling_face: That’s why my patch includes a version_compare()

phenaproxima (he/him)   [14 minutes ago]
So I guess my question for you gentlemen is this: given what that update is trying to do, what’s the correct code path to take in 8.7.0?

amateescu   [12 minutes ago]
so I'm looking at the existing code, and it's trying to make the entity type translatable

amateescu   [11 minutes ago]
that's fully supported in 8.7.0 by just using the new APIs (edited)

phenaproxima (he/him)   [11 minutes ago]
Is there a CR for this, or light docs…?

amateescu   [11 minutes ago]
but the problem is that the update function can only run on > 8.7.0 :slightly_smiling_face:

amateescu   [10 minutes ago]
so what I would do is: remove the code from `consumers_update_8103()`

amateescu   [10 minutes ago]
people who already ran that update function won't care

amateescu   [9 minutes ago]
write a new `consumers_update_8107()` that uses the new API

amateescu   [9 minutes ago]
and release it in a new version of the module, which depends on core > 8.7.0

amateescu   [9 minutes ago]
the CR for the new update API is here: https://www.drupal.org/node/3029997

amateescu   [7 minutes ago]
this is the part of the CR that's very relevant for that module: `\Drupal\Core\Entity\EntityStorageInterface has a new method: restore(EntityInterface $entity)`

amateescu   [7 minutes ago]
the current update function has this code

amateescu   [7 minutes ago]
    ```// Special handling for the secret field added by simple_oauth, make sure that it is not hashed again.
    if ($new_consumer->hasField('secret')) {
      $new_consumer->get('secret')->pre_hashed = TRUE;
    }```

amateescu   [6 minutes ago]
you need to put that code in the `restore()` method of a custom storage class for that entity type

So this is going to need to be discussed with the maintainer, and might even necessitate a new major version of the module. Not sure...

How does this look?

This didn't work, sadly; I got the following error (drush updb):

> [error] Unsupported entity type
> [error] Update failed: consumers_update_8107

:( Any thoughts?

+++ b/src/ConsumerStorage.php
@@ -0,0 +1,21 @@
+  public function restore(EntityInterface $entity) {

We also need to call the parent method here.

+++ b/consumers.install
@@ -187,3 +121,43 @@ function consumers_update_8106() {
+  $field_storage_definitions['langcode'] = BaseFieldDefinition::create('language')
...
+  $field_storage_definitions['default_langcode'] = BaseFieldDefinition::create('boolean')

We need to set a few additional things on these base field definitions:

    ->setName('langcode')
    ->setTargetEntityTypeId('consumer')
    ->setTargetBundle(NULL)

and

    ->setName('default_langcode')
    ->setTargetEntityTypeId('consumer')
    ->setTargetBundle(NULL)

Also, the update function should be a post_update one, and we need to return early if the entity type is already translatable :)

Thanks, @amateescu! How does this look?

Ugh; I had named the post-update file consumers.post_update.module. My bad!

@phenaproxima, this looks much better. I think there's just one thing missing: in the post-update function we also need to set the new storage class on the entity type object.

Also, since we are using the new Entity Update API from 8.7.0, the module needs a dependency on this version of core.

Hello! I'm curious about the state of the patch in the previous comment —it looks like there are still a few small-ish changes that need to be made, but for site maintainers currently blocked by this problem, is it safe to use the patch as-is to work around the issue?

@jazzslider, I've been using it and it seems okay, more or less. But, if you're up for it, please feel free to make the changes to the patch here, then maybe we can get it committed quickly. :)

I applied the patch from #12 but I receive the following. How are you getting it to work?

[error] Exception thrown while performing a schema update. SQLSTATE[42S22]: Column not found: 1054 Unknown column 'base.label' in 'field list': SELECT base.id AS id, base.uuid AS uuid, base.owner_id AS owne
r_id, base.label AS label, base.description AS description, base.image__target_id AS image__target_id, base.image__alt AS image__alt, base.image__title AS image__title, base.image__width AS image__width, base.i
mage__height AS image__height, base.third_party AS third_party, base.secret AS secret, base.confidential AS confidential, base.redirect AS redirect, base.user_id AS user_id, base.is_default AS is_default
> FROM
> {consumer} base
> WHERE base.id IN (:db_condition_placeholder_0); Array
> (
> [:db_condition_placeholder_0] => 1
> )
>
> [error] Update failed: consumers_post_update_make_consumer_entity_type_translatable

Any movement on this?

What's the best way for me to replicate this problem in order to test the patch? Do I need to install a version of consumers module before consumers_update_8103() was introduced? And then move to the latest version and run the updates?

1. Install Drupal 8.6.x
2. Install Consumers 8.x-1.2
3. Upgrade Drupal to 8.7.x (Automatic entity updates will stop working from here)
4. Upgrade Consumers to 8.x-1.9 (The update hook 8103 should fail).

When using the patch, step 4 should be successful.

subscribing.

I'm running 8.7.5 so comment #20 wont work in my case.

I've been having very similar problems I am already running 8.7.5 and hitting problems with installation.

https://www.drupal.org/project/consumers/issues/3081101 contains links to admin screen shots basically i cant install database updates due to version incompatability errors.

Are there any plans to make consumers "just work" on 8.7.x and above?

thanks

Okay. I've tested this out, and the patch in #12 works for me in these scenarios:

• Fresh install of Consumers 8.x-1.x with patch applied and Drupal 8.7.x
• The scenario described in #20 with the exception that I had to start with Consumers 8.x-1.0-beta3 because consumers_update_8103() was added in 8.x-1.0-beta4. Though, I did test and see the failure occur without the patch applied. And the failure resolved after the patch was aplied.

According to #14 the patch here still needs to update the .info.yml file to reflect that it's dependent on core >= 8.7.0.

I also took a look at the code and it looks correct to me. That said, I don't know a whole lot about the Entity Update API so I'm not totally comfortable reviewing the code as I just don't know what I might be missing. It would be great to get a sign off from someone like @amateescu (or really anyone who feels comfortable with the update API) on the code changes if possible.

Here's the same patch, including a test of the update path all the way from Consumers 8.x-1.0-beta1 on Drupal 8.4.0. Whew! It will fail the test because there are other bugs in the update path, but at least now we have a path to get them fixed.

Most of this patch is a binary database fixture, so ignore that part.

This will still fail, but for different reasons.

Let's see if this passes. Worked for me...

Re-titling for accuracy, and escalating this to Critical priority. The update path has straight-up bugs in it, which can break sites or modify data in unexpected ways, and if that's not critical, I don't know what is. This patch at least mitigates the known problems by testing the update path as far back as it goes, and correcting any failures that happen in the course of the update.

Thanks @phenaproxima for finishing this up, and especially for adding tests to prove it's working as expected. And thanks @amateescu and @szeidler for help with reviews and testing. You all rock!

I've committed the patch from #25. I'm going to see what if any other open issues this helps fix. And then I'll tag a new release.

Doesn't this patch fix the previous #3032218: Consumers owner_id field schema differs from database column?

@sam711 I believe it does, but I haven't confirmed it yet. If you can confirm that it does indeed fix #3032218: Consumers owner_id field schema differs from database column feel free to mark it as a duplicate of this one.

@eojthebrave It fixes part of the problem and simplifies it.

I've updated #3032218: Consumers owner_id field schema differs from database column to address the remaining part and be consistent with HEAD.

Thanks for the contributions everyone!