Updating from 1x to 2x causes warning with site settings revision log message field definition

Taking a look

Should work now with this patch; added better support for the range of fields that may or may not exist depending on the version you are updating from.

Okay in any case this does no harm. Feel free to reopen if it hasn't solved for you.

Hi!

I was wondering why the warning would stop the execution of the post update.

And so I updated the module and hoped that that will fix the issue.

But now I noticed that even with the latest version I get

Entity/field definitions
Mismatched entity and/or field definitions
The following changes were detected in the entity type and field definitions.
Site Setting
The Site Setting entity type needs to be updated.
The Revision log message field needs to be installed.
The Revision log message field needs to be uninstalled.

entity_update module can fix it of course, but not sure why this happens, when it seems it did create the right tables for revision.

Maybe something was wrong on my system, but I reopen just in case anyone sees the same.

I tried to update from 1.x dev.

Does this help at all revealing some other issue? https://drupal.stackexchange.com/a/307172

Not much more information I am afraid.
I basically reload my db dump before the upgrade, do the upgrade rc4 and I get this:

drush eval "print_r(\Drupal::entityDefinitionUpdateManager()->getChangeList());"
Array
(
    [site_setting_entity] => Array
        (
            [entity_type] => 2
            [field_storage_definitions] => Array
                (
                    [revision_log] => 1
                    [revision_log_message] => 3
                )

        )

)

Ah and the output of the updb

>  [notice] Update started: site_settings_update_10004
>  [notice] Update completed: site_settings_update_10004
>  [notice] Update started: site_settings_update_10005
>  [notice] Update completed: site_settings_update_10005
>  [notice] Update started: site_settings_update_10006
>  [notice] Update completed: site_settings_update_10006
>  [notice] Update started: site_settings_update_10007
>  [notice] Update completed: site_settings_update_10007
>  [notice] Update started: site_settings_update_10008
>  [notice] Update completed: site_settings_update_10008
>  [notice] Update started: site_settings_update_10009
>  [notice] Update completed: site_settings_update_10009
>  [notice] Update started: site_settings_update_10010
>  [notice] Update completed: site_settings_update_10010
>  [notice] Update started: site_settings_post_update_1_make_revisionable
>  [notice] Site setting entities have been converted to be revisionable.
>  [notice] Update completed: site_settings_post_update_1_make_revisionable
>  [notice] Update started: site_settings_post_update_2_make_revisionable
>  [notice] Update completed: site_settings_post_update_2_make_revisionable
>  [notice] Update started: site_settings_post_update_check_loader
>  [notice] Update completed: site_settings_post_update_check_loader
 [success] Finished performing updates.

Hmmm maybe revision log message has since had updates, that update hook is quite old. Perhaps it's setting the update to be into a destination state that is outdated and the expected state by core perhaps is now different.

Updating title as per comment in #8

As I see it is mainly following this change record: https://www.drupal.org/node/3034742

But something must be missing maybe a method call somewhere.

I'm using Drupal 10.3.1, not sure if the issue comes from that.

Hmm but that has been the case for ages though. These two functions are attempting to do the manual process:

• site_settings_post_update_1_make_revisionable()
• site_settings_post_update_2_make_revisionable()

The challenge is keeping those two methods in sync indefinitely with a thing that is controlled partially by Core. That's my guess at least, perhaps something in Core has since changed to the revisionable table structure and making these post update functions needing a code change to match that. Ie, there must be some difference between a fresh install and an upgrade from 1,x for this to be flagged if I understand it right.

Confirmed I can reproduce this, steps added to card description

Hi!

I found a similar patch on drupal commerce.

I did the following test:
I installed commerce 2.39
I created a store and a test product.
Then I applied the patch from
https://git.drupalcode.org/project/commerce/-/merge_requests/243/diffs

            "drupal/commerce": {
                "test": "https://git.drupalcode.org/project/commerce/-/merge_requests/243.diff"
            },

This patch updates product entity to be revisionable.

And interestingly this did not cause mismatched entity for me, while the revision tab appeared on the entity.

But they organize the code differently, and they add the revision message log in an update hook (commerce_product_update_8213()) not in the post update.

Okay issue is there is meant to be an index name 'site_setting_entity__status_type' on table 'site_setting_entity_field_data' that is a mix of 'status', 'type', and 'id' but the index is missing when upgrading from 1x to 2x

And the other issue is the revision_log_message column is missing from both site_setting_entity_field_data and site_setting_entity_field_data_revision tables.

But they organize the code differently, and they add the revision message log in an update hook (commerce_product_update_8213()) not in the post update.

Hmm yeah, its not in their commerce_product_make_revisionable(), interesting!

For now at least I can see this sorts the issue for those who come across this:

ddev composer require 'drupal/entity_update:^3.0'
ddev drush pm-enable entity_update
ddev drush upe site_setting_entity
ddev drush pm-uninstall entity_update
ddev composer remove drupal/entity_update

It does however in that case one would need to run that module in production.

Very true; I am just not sure how to solve this myself (this feature was contributed by others in the first place). If someone can figure out how to solve it great!

I think this might explain the issue:
https://www.drupal.org/project/sitewide_alert/issues/3194076#comment-139...

So the entity definition might be wrong

 *   revision_metadata_keys = {
 *     "revision_user" = "revision_user",
 *     "revision_created" = "revision_created",
 *     "revision_log_message" = "revision_log"
 *   },

It should be "revision_log_message" = "revision_log_message",

There is a patch which renames it in that module, but it seems a bit long.
But then it is kind of wrong in the main entity definition as well.

Or alternatively we can change the post update to

-  $field_storage_definitions['revision_log_message'] = BaseFieldDefinition::create('string_long')
-    ->setName('revision_log_message')
+  $field_storage_definitions['revision_log'] = BaseFieldDefinition::create('string_long')
+    ->setName('revision_log')

This removes the log_message issue.

Now we just have

The Site Setting entity type needs to be updated.

I added

  if ($field_storage_definitions['status']) {
    $field_storage_definitions['status']->setRevisionable(TRUE);
  }

But it is still there, so we still miss something from the main entity.

Maybe it is just a matter of adding the status_type index to the site_setting_entity_field_data table, but ALTER ing it in won't work of course.

Thanks! Very got spot!! Easiest is to make the update hook match existing working sites I think to avoid a bigger update needed to convert existing sites. Annoying that its another thing that doesn't match standard Drupal Core entities though :(

On status index just reading up on https://www.drupal.org/project/drupal/issues/3005447 now, hoping something in there helps

Looks like he solved it in https://www.drupal.org/project/commerce/issues/2907367, but got complicated it seems, going to see if I can do something with a schema alter to let SqlContentEntityStorageSchema help here

Okay no luck here yet and my daughter woke up early so my time is up for the morning.

Last thing I tried is pushed which does seem to get the tables to match but not recognised by the entity update manager.

I think I got it.

You need to add

  if ($field_storage_definitions['status']) {
    $field_storage_definitions['status']->setRevisionable(TRUE);
  }

and also at the end where you added the index the following:

    $key_value = \Drupal::keyValue('entity.storage_schema.sql');
    $key_name = 'site_setting_entity.entity_schema_data';
    $storage_schema = $key_value->get($key_name);

    if (isset($storage_schema['site_setting_entity_field_data'])) {
      $storage_schema['site_setting_entity_field_data']['indexes']['site_setting_entity__status_type'] = ['status', 'type', 'id'];

      $key_value->set($key_name, $storage_schema);
    }

source: https://www.drupal.org/docs/drupal-apis/update-api/updating-database-sch...

This way it worked for me.

Amazing, thank you so much! Added an update test as well which fails to have an empty change list without your suggested code, but results in empty change list after the code addition, so that confirms it as well.

Updating credits

I see this "drupal.10.3-site-settings-8.x-1.x.php.gz" file in the commits, should that be there?

The part with $key_value = \Drupal::keyValue('entity.storage_schema.sql'); I think should be inside the if (!$index_exists) { part, as normally we would only want to set it if the index did not exist.
It won't cause any issues of course either way.

It works fine!

Yeah the .gz file is a fixture for the automated test https://www.drupal.org/docs/drupal-apis/update-api/writing-automated-upd...

Okay will nest it inside the index check, good point (probably no harm yeah but better to be safe).

Thanks!

Thanks very much for your help with this! Was a real head scratcher

Sorry to reopen again,
but there is one more issue, that the revision UI is not enabled, while it is part of the entity definition.

But even if I add it to the post update hook:

@@ -42,6 +42,7 @@ function site_settings_post_update_1_make_revisionable(&$sandbox) {
   $entity_type->set('entity_keys', $entity_keys);
   $entity_type->set('revision_table', 'site_setting_entity_revision');
   $entity_type->set('revision_data_table', 'site_setting_entity_field_data_revision');
+  $entity_type->set('show_revision_ui', TRUE);
   $revision_metadata_keys = [

It does not want to show up, and so I cant actually create a new revision in this case.

Not sure if I am missing something.

You'd just need to disable the 'Hide advanced options' from /admin/config/site-settings/config. By default trying to keep the UI very simple like 1.x. Lots of features in over the years, but default should still be simplest case which would typically be labels/header/footer settings/etc where revisions are probably not necessary. If you want something more there, maybe a separate issue as separate feature request?

Nice! Thanks!