Convert Linky entity to revisionable

+++ b/src/Entity/Linky.php
@@ -37,15 +38,24 @@ use Drupal\user\UserInterface;
+ *   revision_table = "linky_revision",
+ *   revision_data_table = "linky_field_revision",
+ *   show_revision_ui = TRUE,
...
+ *     "revision" = "revision_id",
...
+ *   revision_metadata_keys = {
+ *     "revision_user" = "revision_uid",
+ *     "revision_created" = "revision_created",
+ *     "revision_log_message" = "revision_log"
+ *   },

Wondering if these keys also need to be manually installed on the entity type definition?

If you want to test this out, any entity query against linky entities should fail if these keys haven't been installed on the entity type definition.

+++ b/src/Entity/Linky.php
@@ -37,15 +38,24 @@ use Drupal\user\UserInterface;
+ *   revision_data_table = "linky_field_revision",

This is used if the entity is translateable which linky is not so this can be removed.

+++ b/linky.post_update.php
@@ -0,0 +1,95 @@
+  $revisionCreatedField = BaseFieldDefinition::create('created')
...
+    ->setInitialValueFromField('changed');
...
+  $update->expression('revision_created', 'changed');

You don't need $update->expression('revision_created', 'changed'); if you set >setInitialValueFromField('changed'); for $revisionCreatedField.

Let's bump the min core version and use the new API.

because we should never rely on live entity type or field definitions in an update function

FWIW, as the author of both the new (EntityDefinitionUpdateManager::updateFieldableEntityType()) and the old (SqlContentEntityStorageSchemaConverter) API, I chose not to use the old API for converting taxonomy terms and menu links to revisionable in core 8.6.x, and wrote the new API for 8.7.x for the reason quoted in #10.

I also maintain a fairly used D8 module (Entityqueue, with ~9.000 installs) where I need to do the same conversion to revisionable, and I will bump the requirements in order to be able to use the new API :)

Thanks, for addressing the feedback. The patch is looking good.
Can we have some upgrade path tests, please?

1. +++ b/composer.json
   @@ -10,5 +10,7 @@
   +    "drupal/core": "~8.7.0"
   

   This isn't needed because Drupal only looks at the dependencies defined in the linky.info.yml file.

   Removing it should also fix the testbot failure.

2. +++ b/linky.post_update.php
   @@ -0,0 +1,119 @@
   +function linky_post_update_revisionable_data_revision_date(&$sandbox) {
   

   This is not needed as well, the entity update API does it for you :)

1. +++ b/linky.post_update.php
   @@ -0,0 +1,119 @@
   +    ->setInitialValueFromField('user_id');
   ...
   +r.revision_uid = base.user_id');
   

   setInitialValueFromField should set it up, rigth?

2. +++ b/linky.post_update.php
   @@ -0,0 +1,119 @@
   +r.revision_created = r.changed,
   

   We might need this because they are different field types.

+++ b/src/Entity/Linky.php
@@ -149,7 +172,6 @@ class Linky extends ContentEntityBase implements LinkyInterface {
       ->setDefaultValueCallback('Drupal\node\Entity\Node::getCurrentUserId')

Why is this using node function?

Addressed #15.1
#15.2 is need without table is showing default field values.
Fixed #18 and clean up the Linky::baseFieldDefinitions() method and removed the redundent methods.

There was ID mismatch so added update hook for that.

+++ b/linky.install
@@ -0,0 +1,31 @@
+  $entity_field_manager = \Drupal::service('entity_field.manager');
+  $entity_field_manager->clearCachedFieldDefinitions();
...
+  $storage_definition = $entity_field_manager->getFieldStorageDefinitions('linky')['id'];

I think this is causing

 [error]  Update aborted by: linky_update_8101 

 [error]  Finished performing updates. 

> [error] The entity type linky does not have an "owner" entity key.

It might fix #23.

Added desc as well.

This might fix it.

With PHPCS fixes.

Upgrade tests are blocked by #3056357: Unbale to install linky with testing profile.

First pass for the upgarde path.

Update path test is working now.

@amateescu I commented out linky_post_update_set_default_revisionable_data. And the failing test will show that setInitialValueFromField is not working properly. Is there something I'm missing or is it a core bug?

Yeah, setInitialValueFromField() only works during a "regular" field install operation. not during entity schema migrations like we're doing here.

A workaround would be to create those fields that need initial values in a hook_update_N() function, which always runs before to the post_update one.

Reroll after the latest release.

RE #38: I like the current state linky_post_update_make_linky_revisionable. Splitting it into hook_update_N means moving bunch of stuff around so I'd say let's leave that and re-instate the linky_post_update_set_default_revisionable_data.

Now with --binary

Moved linky_post_update_make_linky_revisionable to linky_update_8102

This should fix. Ignore #42. Interdiff is from #40.

Reroll of #44

@acbramley, I thought you were not intrested in this issue. :D

Now including the db fixture.

I think the patch needs a reroll after #3097042: Drupal 9 readiness.

+++ b/src/Entity/Linky.php
@@ -136,23 +105,19 @@ class Linky extends ContentEntityBase implements LinkyInterface {
       ->setDefaultValueCallback(static::class . '::getDefaultEntityOwner')

This not needed anymore.

This not needed anymore.

Fixed on commit 🎉