Stop invoking pre-save methods in EntityStorageInterface::restore()

I tested this patch in conjunction with #3066440: Taxonomy update to revisionable is taking hours with paragraphs fields on our installation with about 6000 terms and 15000 paragraphs and the update took a minute instead of 5 hours, as it did before.
This is quite nice indeed!

Might not be part of this patch but EntityStorageInterface::restore probably needs better documentation and probably tests too because I can't find adequate documentation or test either but it might be just me. What is a "restore"? How is it different from save, when should it be called instead of save? I filed #3066674: Missing typehints in SqlFieldableEntityTypeListenerTrait to begin travelling the road to understanding.

This might be the same root cause as #3056540: taxonomy_post_update_make_taxonomy_term_revisionable() timeouts with large numbers of terms.

Still needs tests and a change record but otherwise looks good.

1. +++ b/core/modules/system/tests/modules/entity_test/src/EntityUpdateTestStorage.php
   @@ -0,0 +1,23 @@
   +class EntityUpdateTestStorage extends SqlContentEntityStorage {
   

   This storage class should be named EntityTestUpdateStorage :)

2. +++ b/core/modules/system/tests/modules/entity_test/src/EntityUpdateTestStorage.php
   @@ -0,0 +1,23 @@
   +    $entity->preSave($this);
   +    parent::restore($entity);
   

   Let's throw the exception here directly instead of calling the entity's preSave() method. The goal of that exception is to test for something going wrong while calling $storage->restore(), so it doesn't matter if it's thrown by the entity class or the storage handler.

3. +++ b/core/modules/system/tests/src/Functional/Entity/Traits/EntityDefinitionTestTrait.php
   @@ -415,11 +415,13 @@ protected function deleteEntityType() {
   +   * @param string $storage_class
   +   *   (optional) Use a custom storage class.
   

   I'm not sure about this, why don't we set the custom storage class directly in the entity type definition?

Nice! I think this is ready to go, hoping that the testbot agrees as well.

Yay, all green :)

Great, we just had an issue while upgrading a site to D8.7 caused by these calls to preSave (without calling presave hooks).

Not sure if I should open a separate issue, so I just give some background here:

* The site implements a hook_ENTITY_TYPE_presave that overrides the behavior of MenuLinkContent::preSave() always setting $entity->setRequiresRediscovery(FALSE) for non-"internal:" links.
* The post update menu_link_content_post_update_make_menu_link_content_revisionable() calls this new ::restore() method which in turn calls $entity->preSave() for any menu link content.
* Our hook_ENTITY_TYPE_presave is never called, and thus our rediscover override is not called.
* Now that the rediscover flag was not set for the menu links we want to be rediscovered (so we can programatically attach children links to it) the menu_tree was totally broken.

Calling $entity->preSave (without invoking hooks) in ::restore() is not only slow, it can destroy your data!

@casey, thanks for sharing that use case, it's super useful! And I think it makes this issue critical.

@casey, just to clarify my understanding of the problem described in #25, I think the current patch fixes it because we stop calling MenuLinkContent::preSave() during restore() so the discoverability and the menu tree are not impacted anymore by the conversion to revisionable, right?

It's possible that this will result in a custom/contrib entity type running into the same issue as taxonomy module, but I think the risk of that is outweighed by the criticality of the bug and the number of cases we know are affected. So +1 on the general approach.

One point though:

+++ b/core/modules/taxonomy/src/TermStorage.php
@@ -441,6 +441,14 @@ public function getVocabularyHierarchyType($vid) {
 
+  /**
+   * {@inheritdoc}
+   */
+  public function restore(aa
EntityInterface $entity) {
+    $entity->preSave($this);
+    parent::restore($entity);
+  }
+
   /**

This is only saving three lines of code, why not copy it? It runs the potentially of running additional code that could be added to Term::preSave() as it currently is. Or at least add a clear comment explaining why it needs to be called here.

I also considered pointing out that we could copy that code, so I'm glad both of you think it's better this way :)

@amateescu #27, correct

If I understand correctly the goals of restore then TermStorage::restore is wrong -- my understanding is you have the entity coming from somewhere else but it's a entity previously saved in D8 (perhaps in another Drupal instance, of course) and as such the target_id was zeroed on a previous (presumed) save(). If this mental model is correct then restore should be declared final so you can't break the assumption.

I was wondering about that too. Term parents used to be very special as it was only a half-field, but that has been changed a while ago and I think that is there (and not only a default value for the field) to make it impossible to break the tree by having no value at all.

However, it should never have no value in a restore situation. So I think this method isn't needed. I'm not quite sure yet if there *are* use cases for overriding that method and if it should be final or not.

If this mental model is correct then restore should be declared final so you can't break the assumption.

I'm not quite sure yet if there *are* use cases for overriding that method and if it should be final or not.

Here's at least one use case for being able to "massage" the data before the restore process: https://git.drupalcode.org/project/entityqueue/blob/e099b13506f664afe46e...

If we want to be able to remove the restore() override from the latest patch, we need to look into and fix the failures from the patch in #12.

Excellent find and fix!

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
   @@ -168,6 +168,11 @@ public function save(EntityInterface $entity);
   +   *   This method should never be used to perform a regular entity save. It's
   

   It's -> Its :)

2. +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
   @@ -168,6 +168,11 @@ public function save(EntityInterface $entity);
   +   *   overriding this method to fix data prior to restoring is a sign that the
   

   "is a sign" -> "could be a sign"?

3. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -70,7 +69,7 @@ function taxonomy_update_8502(&$sandbox) {
   -      'langcode' => $default_langcode,
   +      'langcode' => $row->langcode,
   

   We should add some test coverage for this in \Drupal\Tests\taxonomy\Functional\Update\TaxonomyParentUpdateTest::testTaxonomyUpdateParents().

+++ b/core/modules/taxonomy/taxonomy.install
@@ -195,3 +195,32 @@ function taxonomy_update_8701() {
+function taxonomy_update_8702() {

This is a content change, could it go in a post update instead?

Also very nice find this is a lot better.

+++ b/core/modules/taxonomy/taxonomy.install
@@ -195,3 +195,32 @@ function taxonomy_update_8701() {
+  if (!$field_storage_definition->isTranslatable()) {
+    $default_langcode = \Drupal::languageManager()
+      ->getDefaultLanguage()
+      ->getId();
+    // taxonomy_update_8502() populated the langcode field of
+    // 'taxonomy_term__parent' using the term's langcode but the field is not
+    // translatable. If taxonomy_post_update_make_taxonomy_term_revisionable()
+    // has already run prior to
+    // https://www.drupal.org/project/drupal/issues/3066439 then this will have
+    // already fixed the field. However on sites that have not run that update
+    // yet we need to run this update the langcode as calling
+    // \Drupal\Core\Entity\Sql\SqlContentEntityStorage::restore() no longer
+    // calls \Drupal\taxonomy\Entity\Term::preSave().
+    \Drupal::database()->update('taxonomy_term__parent')
+      ->fields(['langcode' => $default_langcode])
+      ->execute();
+  }

what about terms whose default langcode isn't the site default langcode?

I think it should be the default langcode of the entitty, not the site.

> Note that overriding this method to fix data prior to restoring is a sign that the current data is corrupt.

My primary language is not English (it's PHP :P ) but wouldn't "Do not override this method unless fixing corrupt data prior to restoring is necessary" be better? And if this is what it is, is it the role of restore to fix corrupted data, isn't that the role of the caller? I still would love to see restore as final. The test could override doSave instead ; by making restore final we could remove this note altogether and send a strong message about how it should be used. Sorry I am a bit stubborn here, I am mostly just scared of what we are getting into again, we had special saves for the update path before and it ended up in tears and very slightly corrupted data which eventually caused the weirdest migration faults...

Re #41 - nope unfortunately it can't - it has to come before the revisionable post update. And the post updates are by definition not in any guaranteed order.

The documentation of hook_post_update_NAME() says that ordering is possible:

 * NAME can be arbitrary machine names. In contrast to hook_update_N() the
 * alphanumeric naming of functions in the file is the only thing which ensures
 * the execution order of those functions. If update order is mandatory,
 * you should add numerical prefix to NAME or make it completely numerical.

+++ b/core/modules/system/tests/fixtures/update/drupal-8.taxonomy-parent-multilingual-3066439.php
@@ -0,0 +1,51 @@
+for ($i = 0; $i < 60; $i++) {

+++ b/core/modules/taxonomy/taxonomy.install
@@ -195,3 +195,53 @@ function taxonomy_update_8701() {
+      if ($sandbox['current'] >= Settings::get('entity_update_batch_size', 50)) {

+++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyTermParentMultilingualTest.php
@@ -0,0 +1,82 @@
+    // There are 5 terms in the db already and we create an extra 60 so the last
+    // term is 65. Ensure that taxonomy_update_8702() has fixed its parent data.

Note that we are setting entity_update_batch_size to 1 since #3007606: Force all update path tests to only run one entity per batch, so we're executing 65 batch process for this update test :)

1. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -195,3 +195,55 @@ function taxonomy_update_8701() {
   +  if (!$field_storage_definition->isTranslatable() && !$entity_type->isRevisionable()) {
   ...
   +    $select = $database->select('taxonomy_term__parent', 'tp');
   

   Since we're doing straight db queries, we need to ensure that the entity type is using core's default SQL content entity storage.

2. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -195,3 +195,55 @@ function taxonomy_update_8701() {
   +    // revisionable. However on sites that have not run that update yet we need
   +    // to run update taxonomy_term__parent.langcode as calling
   +    // \Drupal\Core\Entity\Sql\SqlContentEntityStorage::restore() no longer
   +    // calls \Drupal\taxonomy\Entity\Term::preSave().
   

   I don't think we should document how code used to work :)

3. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyTermParentMultilingualTest.php
   @@ -0,0 +1,88 @@
   +  public function testMultilingualTermParentUpdate() {
   

   How about adding this test method to the existing TaxonomyParentUpdateTest test class?

Nice! I think this patch is good to go now :)

Looks great now. Committed/pushed to 8.8.x and cherry-picked to 8.7.x, thanks!

Looks good but is the first part really only for php 5.5/8.7?

Does the CR related here need to be updated or published? It is still in draft status.