Make the Entity Field API handle field purging

#2338873: Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data is in.

I'm tempted to demote this issue to Normal priority and postpone it to 8.1. I think it can be done while preserving BC. Any reason to keep it as Major and/or give it any attention prior to 8.0?

I came here when trying to figure out how I might remove a base field that has data, if you disable the exception in core it seems to work just fine, obviously you lose the data but that's the desired outcome.

This is the line.
throw new FieldStorageDefinitionUpdateForbiddenException('Unable to delete a field (' . $storage_definition->getName() . ' in ' . $storage_definition->getTargetEntityTypeId() . ' entity) with data that cannot be purged.');

Is there something else that needs to be implemented to allow successful purging of base fields?

I've been looking into this and created #2905944: Uninstall modules which define base fields.

Guess I should close my new issue in favour of this issue.

I started working on this last night and I should have a patch soon enough :)

@benjy:

Is there something else that needs to be implemented to allow successful purging of base fields?

Yes, the entire "field purge" dance which allows the data to be removed safely while giving other modules a way to react to the deletion of that field. In other words, we need to fire \Drupal\Core\Field\FieldItemList::delete() for every single entity that contains the field, just like we do for configurable fields.

Here's an initial patch which shows how this could be done. The implementation idea for deleting/purging a base field stored in the shared tables is:

1. generate a dedicated table schema for the field and create the dedicated tables with their 'deleted' names (@see \Drupal\Core\Entity\Sql\DefaultTableMapping::getDedicatedDataTableName())
2. copy the field data to the dedicated tables with a cleverish INSERT FROM SELECT query, which is executed very fast (read: instantly) by every database engine
3. put the field and field storage definitions into their corresponding state entries: field.field.deleted and field.storage.deleted
4. add support for all FieldDefinitionInterface and FieldStorageDefinitionInterface objects to field_purge_batch() & friends and let them do their thing as usual (basically what @fago wrote in #3)

This patch does 1. and 2. successfully (manually tested), so we still need to write some code for 3. and 4.

Adding Contrib blocker tag as I've seen this come up for me trying to port termstatus and filefield_paths is having similar issues here #2685731-3: How to uninstall this module ?

@joelpittet, indeed, that's a very good tag for this issue :) Every module that provides a field type used a base field can not be uninstalled because of this. Another example that I know of is Workbench Moderation: #2627012: Delete moderation_state fields when uninstalling workbench_moderation

Anyway, here's another work-in-progress patch that does 3. and 4. from the list in #11. With this patch, field_purge_batch() & friends work entirely with FieldDefinitionInterface and FieldStorageDefinitionInterface objects!

Leaving at 'needs work' because I still need to actually put base fields in the new "deleted fields repository" when they are uninstalled and add tests. We're very close though :)

amateescu++ Cool. Just a guess at the langcode errors: does the langcode checks need to be wrapped in a condition like:

$translatable = $this->entityType->isTranslatable();
if ($translatable) {
  // Add the langcode column.
  ...
}

This is awesome @amateescu, just tested with Workbench Moderation, and it does allow the module to be uninstalled. The moderation_state columns are still in node_field_data and node_field_revision, and listed on the status report page is "The Moderation state field needs to be uninstalled". Running drush entup returns "The "moderation_state" entity type does not exist.".

I guess this is a known issue and part of the todos in #14, but just wanted to share manual test findings.

@joelpittet, not really, an entity type can have a langcode field without being translatable. Those failures were quite a bit trickier to figure out :)

@timmillwood, yup, the thing that you tested was not implemented yet.

However, with this update we can finally remove and purge base fields, as proven by the new test coverage added to \Drupal\KernelTests\Core\Entity\EntityDefinitionUpdateTest::testBaseFieldDeleteWithExistingData().

I still need to look into purging bundle base fields, but this patch is now ready for some actual reviews.

After looking more into it, purging bundle fields is quite complex as well and I propose to handle that in a separate issue because this patch is big enough already.

I've opened followup issue for what I think is out-of-scope here:

#2907780: Add a field purgatory service
#2907779: Add support for deleting entity definitions in \Drupal\Core\Entity\EntityDefinitionUpdateManager::getChangeList()
#2907777: Consider removing the content translation metadata fields when the entity type has no bundle enabled for translation

I've also rewritten the issue summary so this is truly ready for reviews now :)

This is a pretty big change so I've also written a change record: https://www.drupal.org/node/2907785

@amateescu re #17, darn sorry to hear it was more work than that.

I agree with Bundle fields being a separate issue, thanks for already opening up follow-ups.

Hope to give this a test run, likely at drupalcon if nobody gets to it sooner.

This may help with #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted

Corrected coding standards errors.

@Jo Fitzgerald, can you please post a review like everybody else does so the original author of the patch can address it? Thank you.

@amateescu The coding standards errors had not been addressed for a few days - I thought it would be quicker to simply resolve them myself. Sorry for 'treading on your toes'.

This looks very impressive. I'm not totally qualified to review this, but I put my thoughts and questions down. I think this is probably more for my own benefit/learning than actual feedback :)

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -272,7 +272,7 @@ public function setExtraColumns($table_name, array $column_names) {
      public function allowsSharedTableStorage(FieldStorageDefinitionInterface $storage_definition) {
   -    return !$storage_definition->hasCustomStorage() && $storage_definition->isBaseField() && !$storage_definition->isMultiple();
   +    return !$storage_definition->hasCustomStorage() && $storage_definition->isBaseField() && !$storage_definition->isMultiple() && !$storage_definition->isDeleted();
      }
   

   So as a field, you're allowed to be in the shared table, but not if you are deleted. That makes sense.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1716,23 +1713,4 @@ public function countFieldData($storage_definition, $as_bool = FALSE) {
   -    if ($storage_definition instanceof FieldStorageConfigInterface) {
   -      return $storage_definition->isDeleted();
   -    }
   

   So this is also no longer a feature of just config based field storage. Also makes perfect sense.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -410,25 +434,30 @@ public function onFieldStorageDefinitionCreate(FieldStorageDefinitionInterface $
   +      if ($storage_definition instanceof BaseFieldDefinition && !$storage_definition->isBaseField() && $has_data) {
   

   Hm, maybe needs a comment. instanceof BaseFieldDefinition but not isBaseField() is a bit confusing.

4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -452,11 +486,68 @@ public function onFieldStorageDefinitionDelete(FieldStorageDefinitionInterface $
   +        $base_table = $this->entityType->isTranslatable() ? $this->storage->getDataTable() : $this->storage->getBaseTable();
   

   Can this also depend on if the field is translateable or not? Do we need \Drupal\Core\Entity\Sql\TableMappingInterface::getFieldTableName instead?

5. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -452,11 +486,68 @@ public function onFieldStorageDefinitionDelete(FieldStorageDefinitionInterface $
   +          $this->database->insert($dedicated_revision_table_name)
   +          ->from($this->getSelectQueryForFieldStorageDeletion($revision_table, $shared_table_field_columns, $dedicated_table_field_columns, $base_table))
   +          ->execute();
   

   This is probably a silly question, but why do we have to backup the field data in the first place?

6. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -468,6 +559,76 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
   +    // Add the delta column.
   +    $select->addExpression(':delta', 'delta', [':delta' => 0]);
   

   Maybe we should explain why delta is zero here? I don't really have a grasp of this whole function, but I might assume this only supports cardinality of 1 looking at this.

7. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -804,6 +804,39 @@ public function getUniqueStorageIdentifier() {
   +    return isset($this->definition['deleted']) ? (bool) $this->definition['deleted'] : FALSE;
   

   This is all equiv to return !empty($this->definition['deleted']).

@Sam152, thanks for reviewing!

Re #25:

3. It is confusing indeed so I expanded the comment to explain that check.

4. Excellent point! Fixed.

5. Because of the way field purging works :) We need to have the original field data available when invoking the \Drupal\Core\Field\FieldItemList::delete() method on each field. You can see a more detailed explanation here: https://api.drupal.org/api/drupal/core!modules!field!field.purge.inc/gro...

6. You're right, expanded that comment as well.

7. Heh, that's true. Fixed.

Oh, I forgot to mention that the patch also neede a reroll for #2391829: ContentUninstallValidator relies on the not required ContentEntityStorage::hasData() method, which removed the hasData() method from DynamicallyFieldableEntityStorageInterface.

And since this patch moves the other two methods from DynamicallyFieldableEntityStorageInterface to FieldableEntityStorageInterface, the first interface is now empty! Not sure yet what we should do about it, if anything, but I think it was worth pointing out :)

Like @Sam152, I don't think I am qualified to review, but thought I'd take a look and came out with 3 questions:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -436,10 +460,22 @@ public function onFieldStorageDefinitionDelete(FieldStorageDefinitionInterface $
   +    // There's nothing else we can do if the field storage has a custom storage.
   +    if ($storage_definition->hasCustomStorage()) {
   +      return;
   +    }
   

   I wonder if we should throw a warning / exception or just validate for this use case.

2. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -804,6 +804,39 @@ public function getUniqueStorageIdentifier() {
   +  public function setDeleted($deleted) {
   

   Should this be a part of an interface?

3. +++ b/core/modules/field/src/FieldConfigStorage.php
   @@ -117,16 +105,14 @@ public function loadByProperties(array $conditions = []) {
   +    $deleted_fields_repository = \Drupal::service('field.deleted_fields_repository');
   

   Could this be injected?

Re #28:

1. We already call $this->performFieldSchemaOperation('delete', $storage_definition); just above that line, and I think that's enough.
2. That's still an open question mentioned in the 'remaining tasks' part of the issue summary :)
3. Nope, we can't inject stuff into entity classes.

I guess I should've read the issue summary. My response to the remaining tasks there would be:

• Yes.
• In a follow up.

Re: #28.3 - Can't we add 'field.deleted_fields_repository' to \Drupal\field\FieldConfigStorage::createInstance? or am I missing something?

Addressed #28.3
@timmillwood you are correct and FieldConfigStorage and FieldStorageConfig is not confusing at all :P, former is a storage for FieldConfig entity and latter is an entity itself, used to store field storage settings.

I also found out \Drupal\Tests\rest\Functional\EntityResource\FieldStorageConfig\FieldStorageConfigResourceTestBase::$entity is wrongly typhinted. It should be \Drupal\field\Entity\FieldStorageConfig not \Drupal\field\FieldConfigStorage but that change is out of scope here.

Addressed #28.3 for FieldStorageConfigStorage as well.

About adding the setDeleted() method to FieldStorageDefinitionInterface, there's only one other setter on that interface, setTranslatable(), so I would like more opinions from other entity maintainers about it.

Oops, the interdiff from #33 was correct but the patch was not :)

I've been discussing this patch with @plach at DrupalCon Vienna and he suggested that we should not be using any class from the field module in the new service (we were using Drupal\field\Entity\FieldConfig), so here's an updated patch that cleans up that part and a few other things as well.

Sigh..

Awesome work!

1. +++ b/core/core.services.yml
   @@ -510,12 +510,6 @@ services:
   -  field_uninstall_validator:
   

   Are we allowed to remove services? Shouldn't we deprecate it but leave it around to preserve BC?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -19,7 +19,6 @@
   -use Drupal\field\FieldStorageConfigInterface;
   

   Nice!

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1716,23 +1712,4 @@ public function countFieldData($storage_definition, $as_bool = FALSE) {
   -  protected function storageDefinitionIsDeleted(FieldStorageDefinitionInterface $storage_definition) {
   

   This class is heavily subclassed: although it's not likely this method is actually overridden, what if we just deprecated it?

4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -410,25 +434,25 @@ public function onFieldStorageDefinitionCreate(FieldStorageDefinitionInterface $
   +      // @todo Add support for deleting bundle fields. Bundle field definitions
   +      // are also using the \Drupal\Core\Field\BaseFieldDefinition class, but
   +      // their isBaseField() method returns FALSE.
   +      // @see https://www.drupal.org/node/2907780
   

   We need to indent @todo comments: https://www.drupal.org/node/1354#todo.

5. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -452,11 +488,69 @@ public function onFieldStorageDefinitionDelete(FieldStorageDefinitionInterface $
   +      $updated_storage_definitions = [$storage_definition->getName() => $deleted_storage_definition] + $storage_definitions;
   

   It took me a while to understand why we were updating the definition while deleting it, before realizing we were not. Can we rename the variable to something like $original_storage_definition.

6. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -452,11 +488,69 @@ public function onFieldStorageDefinitionDelete(FieldStorageDefinitionInterface $
   +        if (!$this->database->schema()->tableExists($dedicated_table_name_mapping[$name])) {
   

   We should throw an exception in the else case.

7. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -468,6 +562,77 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
   +      $select->addExpression(':langcode', 'langcode', [':langcode' => LanguageInterface::LANGCODE_DEFAULT]);
   

   This should be LanguageInterface::LANGCODE_NOT_SPECIFIED. We are probably missing some test coverage around this case.

8. +++ b/core/lib/Drupal/Core/Field/DeletedFieldsRepository.php
   @@ -0,0 +1,128 @@
   +   * Constructs a new DeletedFieldsRepository.
   

   Is this a new pattern or should become deleted field repository?

9. +++ b/core/lib/Drupal/Core/Field/DeletedFieldsRepository.php
   @@ -0,0 +1,128 @@
   +    $deleted_storages = $this->getFieldStorages();
   

   For consistency, can we change all these occurrences of the storage(s) term to storage_definition(s)?

10. +++ b/core/lib/Drupal/Core/Field/DeletedFieldsRepository.php
    @@ -0,0 +1,128 @@
    +  public function getFieldStorages() {
    

    Same for these: it would be more consistent to call them ::getFieldStorageDefinitions() and mention definitions everywhere. If you feel names get too long we can omit the field part, since the class is already a field repository.

11. +++ /dev/null
    @@ -1,140 +0,0 @@
    -    // Verify uninstalling entity_test is not possible when there is content for
    -    // the bundle field.
    -    $entity->delete();
    

    Since we are not officially supporting bundle field definitions, it would be great to restore this test coverage. Or we can just check that bundle field work :)

12. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -452,11 +488,69 @@ public function onFieldStorageDefinitionDelete(FieldStorageDefinitionInterface $
    +        if ($this->entityType->isRevisionable()) {
    

    Can we use isset($dedicated_revision_table_name) so we make PHPStorm happy?

13. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -452,11 +488,69 @@ public function onFieldStorageDefinitionDelete(FieldStorageDefinitionInterface $
    +        if ($this->database->supportsTransactionalDDL()) {
    

    Can we use isset($transaction) so make PHPStorm happy?

Awesome review! Thanks, @plach :)

This patch fixes most of the points from #40 except:

1. I'm not sure, we need to ask @xjm or @catch.

11. I'd rather make sure that purging bundle fields work than restoring that test coverage :)

Also, I investigated the failures from #37 and they are indeed correct. They are showing why we needed to alter deleted field definitions at runtime for field config objects, \Drupal\Core\Field\FieldConfigBase::__sleep() unsets the fieldStorage property when the object is serialized so when we try to retrieve it from the deleted repository, it won't have a field storage attached anymore, so calling ->getFieldStorageDefinition() on it will fail.

I'll think of some way to fix that and #40.11 next week :) Leaving at 'needs work' because this patch will just fail in the same way.

I couldn't resist checking if bundle field deletion works and it turns out that my assumption was correct, it works just fine once we remove the checks that actually prevent it from happening :)

This also allows us to remove a great of ugliness in both SqlContentEntityStorageSchema and SqlContentEntityStorage!

This patch should have the same failures as #37, so it's still NW :/

Ok, so the ugliness that I was hoping to remove in #42 needs to stay, I simply forgot which tests fail without it (ViewsEntitySchemaSubscriberIntegrationTest).

And here's a fix for the other fail introduced in #36. @plach's argument was that we shouldn't introduce any knowledge of \Drupal\field\Entity\FieldConfig into the new deleted fields repository, but it is actually \Drupal\Core\Field\FieldConfigBase the one who removes the fieldStorage property in its __sleep() method, so we can just change the instance of check to use the base class.

Forgot to bring back one line in #43.

@plach, the patch is ready for a final review now :)

Not sure what went wrong with the patch from #45, but let's try again :)

Just a question:

Modules that provide base fields for other entity types can now be uninstalled!!!

This is awesome because it's been a blocker for an old module of mine and something of concern in custom modules.

When uninstalling the module, will there be a "warning" explicitly tells that data will be removed?

@johndevman, of course, the usual strong message that appears when you uninstall a module tells you exactly that :) Here's a screenshot for uninstalling the Entityqueue module which provides a content entity type with base fields, for example:

Are we waiting for @plach's review here?

Yup, either @plach or any other entity system maintainer.

Almost there!

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -468,6 +557,77 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
   +   *   (optional) The name of the base entity table.
   

   Missing default description.

2. +++ b/core/core.services.yml
   @@ -510,12 +510,6 @@ services:
   -  field_uninstall_validator:
   -    class: Drupal\Core\Field\FieldModuleUninstallValidator
   -    tags:
   
   diff --git a/core/lib/Drupal/Core/Field/FieldModuleUninstallValidator.php b/core/lib/Drupal/Core/Field/FieldModuleUninstallValidator.php
   deleted file mode 100644
   
   diff --git a/core/tests/Drupal/KernelTests/Core/Field/FieldModuleUninstallValidatorTest.php b/core/tests/Drupal/KernelTests/Core/Field/FieldModuleUninstallValidatorTest.php
   deleted file mode 100644
   
   diff --git a/core/lib/Drupal/Core/ProxyClass/Field/FieldModuleUninstallValidator.php b/core/lib/Drupal/Core/ProxyClass/Field/FieldModuleUninstallValidator.php
   deleted file mode 100644
   

   I'm still not sure about this deletion. It would be good to confirm with a release manager. I guess they can do it while reviewing, though.

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityBundleFieldTest.php
   @@ -105,8 +105,8 @@ public function testCustomBundleFieldUsage() {
   +    // @todo Test field purge and table deletion once supported.
   +    // @see https://www.drupal.org/node/2907780
        // $this->assertFalse($this->database->schema()->tableExists($table), 'Custom field table was deleted');
   

   Do we still need this @todo and commented assertion?

4. +++ b/core/modules/field/src/FieldStorageConfigStorage.php
   @@ -117,11 +117,12 @@ public function loadByProperties(array $conditions = []) {
   +      $deleted_storages = $this->deletedFieldsRepository->getFieldStorageDefinitions();
   
   +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -407,23 +408,19 @@ public static function preDelete(EntityStorageInterface $storage, array $field_s
   +        $storage = clone $field_storage;
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -438,42 +438,90 @@ public function testBaseFieldDeleteWithExistingData() {
   +    $deleted_fields = \Drupal::service('field.deleted_fields_repository')->getFieldDefinitions();
   ...
   +    $deleted_storages = \Drupal::service('field.deleted_fields_repository')->getFieldStorageDefinitions();
   ...
   +    $deleted_fields = \Drupal::service('field.deleted_fields_repository')->getFieldDefinitions();
   ...
   +    $deleted_storages = \Drupal::service('field.deleted_fields_repository')->getFieldStorageDefinitions();
   

   A few more variables needing to switch to the storage definition terminology.

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -438,42 +438,90 @@ public function testBaseFieldDeleteWithExistingData() {
   +    $this->assertFalse($schema_handler->tableExists($dedicated_deleted_table_name), 'A dedicated table was created for the deleted new_base_field.');
   

   Wrong message?

Thanks for getting back to this issue :)

Fixed everything in #52.

Awesome, thanks!

1. +++ b/core/core.services.yml
   @@ -510,12 +510,6 @@ services:
        lazy: true
   -  field_uninstall_validator:
   -    class: Drupal\Core\Field\FieldModuleUninstallValidator
   -    tags:
   -      - { name: module_install.uninstall_validator }
   -    arguments: ['@entity.manager', '@string_translation']
   -    lazy: true
   

   Since this is a tagged service as opposed to something that's supposed to be injected, I think it's fine to remove it.

2. +++ b/core/lib/Drupal/Core/Entity/DynamicallyFieldableEntityStorageInterface.php
   @@ -18,27 +16,4 @@
   -   *   The deleted field whose data is being purged.
   

   I was going to say "I don't think we can remove methods from the interface", but they're added to FieldableEntityStorageInterface so nothing to see here!

I only managed a quick pass through the first 60%-ish of the patch, but it all looks positive so far. I'll try to review again later, mostly leaving this here as a note-to-self for now.

***YAY!!!***, major kudos for nailing this, folks, God knows there's some nasty stuff going in there.
Hats off @amateescu, you so severely rock.

Trying to articulate my thoughts about #27 (DynamicallyFieldableEntityStorageInterface now being empty --> deprecate ?), but I have to run so posting my other remarks for now - also, deprecating it could happen in a followup I guess.

Nothing blocking AFAICT, so leaving at RTBC :

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -272,7 +272,7 @@ public function setExtraColumns($table_name, array $column_names) {
      public function allowsSharedTableStorage(FieldStorageDefinitionInterface $storage_definition) {
   -    return !$storage_definition->hasCustomStorage() && $storage_definition->isBaseField() && !$storage_definition->isMultiple();
   +    return !$storage_definition->hasCustomStorage() && $storage_definition->isBaseField() && !$storage_definition->isMultiple() && !$storage_definition->isDeleted();
      }
   

   Not fully sure what's the impact of this change, but it looks like it could change the table layout (which field goes where) ?
   If so, is this automatically picked up by the update.php / 'drush entup' mechanism, or does there need to be an explicit update somewhere ?

   [edit: I guess it actually doesn't change the table layout on existing sites, since only base fields could be in the shared tables, and they could not be deleted so far. So moving deleted base fields out of the shared tables affects nothing on existing setups. So, probably a moot point]

2. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php
   @@ -139,6 +151,23 @@ public function onFieldStorageDefinitionDelete(FieldStorageDefinitionInterface $
   +        $deleted_storage_definition = clone $storage_definition;
   +        $deleted_storage_definition->setDeleted(TRUE);
   +        $this->deletedFieldsRepository->addFieldDefinition($deleted_storage_definition);
   +        $this->deletedFieldsRepository->addFieldStorageDefinition($deleted_storage_definition);
   
   +++ b/core/modules/field/src/Entity/FieldConfig.php
   @@ -189,27 +189,25 @@ public function calculateDependencies() {
   +        $field = clone $field;
   +        $field->deleted = TRUE;
   +        $deleted_fields_repository->addFieldDefinition($field);
   
   +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -407,23 +408,19 @@ public static function preDelete(EntityStorageInterface $storage, array $field_s
   +        $storage_definition = clone $field_storage;
   +        $storage_definition->deleted = TRUE;
   +        $deleted_fields_repository->addFieldStorageDefinition($storage_definition);
   

   Minor DX : since the repository is now a domain object with proper business methods around State, the add*() methods could take care of the clone/setDeleted() part internally ? Would save some hassle for callers and ensure no definition is ever put in there with deleted = FALSE.

   Or maybe, we could save the clone/setDeleted part altogether, store the definition as is, and have DeletedFieldsRepository::get*Definitions() do the setDeleted() on read instead ? (no clone needed then)

3. +++ b/core/modules/field/field.purge.inc
   @@ -67,20 +66,16 @@
   -function field_purge_batch($batch_size, $field_storage_uuid = NULL) {
   -  $properties = [
   -    'deleted' => TRUE,
   -    'include_deleted' => TRUE,
   -  ];
   -  if ($field_storage_uuid) {
   -    $properties['field_storage_uuid'] = $field_storage_uuid;
   -  }
   -  $fields = entity_load_multiple_by_properties('field_config', $properties);
   +function field_purge_batch($batch_size, $field_storage_unique_id = NULL) {
   +  /** @var \Drupal\Core\Field\DeletedFieldsRepositoryInterface $deleted_fields_repository */
   +  $deleted_fields_repository = \Drupal::service('field.deleted_fields_repository');
   +
   +  $fields = $deleted_fields_repository->getFieldDefinitions($field_storage_unique_id);
   

   Can't say I remember why the previous code was reading deleted fields from entity_load_multiple_by_properties() / FieldConfigStorage rather than directly from State :-/, but this looks cleaner.

   So, right, FieldConfigStorage / FieldStorageConfigStorage implement a special 'include_deleted' feature (= query from both regular config and the deleted fields repo), as a convenience for code that needs to consider *both* deleted & non deleted fields (that's only FieldUninstallValidator in current core)

   ConfigImporterFieldPurger::getFieldStoragesToPurge() still uses entity_load_multiple_by_properties() to load only deleted storages. Maybe it should switch to using the repo directly as well ?
   Consistency++, "code that deals with purging uses the repo, and Storage::loadMultiple('include_deleted') is there for the rest" ?

   Feel free to ignore if you think that's scope creep, this is mostly me trying to wrap my head around the purge landscape again :-)

@yched, thanks! What can I say.. I learned from the best :D

I agree that the fate of DynamicallyFieldableEntityStorageInterface can be decided in a followup, there's no reason to make this complex patch even bigger.

1. As you figured out by yourself in the edited comment, there is no impact to the table layout on existing sites because base fields can not be marked as deleted in HEAD.

2. I actually tried various things in that area, including your proposal, but nothing worked out in the end because the $deleted property of FieldConfig / FieldStorageConfig is protected and there's no setter for it, so it can not be changed from the outside. Apparently, PHP allows you to set it in a static method but only if you're doing it inside the class itself. That's why I left the ->deleted = TRUE; calls in place :)

3. I don't think that's scope creep, I only left that call unchanged because I wanted to have more coverage to ensure that I'm not breaking BC for entity_load_multiple_by_properties() calls with 'include_deleted' => TRUE. But now that the patch is complete and everything worked out nicely (i.e. all tests are passing), we can definitely change it to use the new service directly.

re #58.2
Ah right, not setters on the interfaces :-/. Yup, never mind then.

re #58.3
Oops, I might have spoken too soon on that :-/ :

DeletedFieldsRepository contains both base and config fields,
while entity_load_multiple_by_properties('deteted' + 'include_deleted') only returns config fields - well at least they shoiuld, but in the current patch FieldConfigStorage/FieldStorageConfigStorage::loadByProperties() merge all DeletedFieldsRepository, so they return base fields as well ;-)

So
- FieldConfigStorage/FieldStorageConfigStorage::loadByProperties() need to filter out base fields from the DeletedFieldsRepository before merging with the non-deleted fields
- ConfigImporterFieldPurger::getFieldStoragesToPurge() probably really only wants deleted *config* fields, so it should also fllter out base fields

Maybe the DeletedFieldsRepository::get*Definitions() need an optional param to only get base or config fields ?

Ugh.. right you are :/ The common need seems to be to only get configurable fields so that's what the new parameter does.

Probably I'm missing something here, but why can't the Field module code perform filtering itself? Special-casing a particular field flavor doesn't feel right to me and theoretically there may be other implementations of the interfaces besides those provided by the Entity Field API and the Field module, so the following code might not work as intended in those cases:

+++ b/core/lib/Drupal/Core/Field/DeletedFieldsRepository.php
@@ -57,14 +57,28 @@ public function getFieldDefinitions($field_storage_unique_id = NULL) {
+    if ($only_config_fields) {
+      $deleted_field_definitions = array_filter($deleted_field_definitions, function (FieldDefinitionInterface $field_definition) {
+        return !$field_definition instanceof BaseFieldDefinition;
+      });
+    }
...
+    if ($only_config_field_storages) {
+      $deleted_field_storage_definitions = array_filter($deleted_field_storage_definitions, function (FieldStorageDefinitionInterface $field_storage_definition) {
+        return !$field_storage_definition instanceof BaseFieldDefinition;
+      });
+    }

re: #62 - yeah, sounds like a fair point :-)

So FieldConfigStorage::loadByProperties(), FieldStorageConfigStorage::loadByProperties(), and ConfigImporterFieldPurger::getFieldStoragesToPurge() would need to do the array_filter() themselves, doesn't seem unreasonable...

Looking at the code after some sleep I agree that #61 is a bit ugly, so let's do the filtering where it's needed :) Interdiff is to #58.

Sorry, not really awake yet. Here's the proper patch for #64, the interdiff is correct though.

Yep, #65 seems right.

The last piece of the patch that made me look more closely is this :

+++ b/core/lib/Drupal/Core/Field/DeletedFieldsRepository.php
@@ -0,0 +1,133 @@
+      // Configurable fields have an internal reference to their field storage
+      // which we must keep in sync if the storage itself is also deleted.
+      if ($field_definition instanceof FieldConfigBase && isset($deleted_storage_definitions[$field_definition->field_storage_unique_id])) {
+        $config = $field_definition->toArray();
+        $config['deleted'] = TRUE;
+        $config['fieldStorage'] = $deleted_storage_definitions[$field_definition->field_storage_unique_id];
+        $updated_field_definition = get_class($field_definition)::create($config);

This brings back another batch of bitter-sweet memories :-)

OK, so this transposes some code that used to be done in the "merge deleted & non-deleted" part of FieldConfigStorage::loadByProperties().
It works around the fact $field_config->getFieldStorageDefinition() typically reads from the EntityManager registry of definitions, which, for deleted fields, might not work if the storage is deleted as well. So in those cases, we inject the (deleted) $storage_definintion right into the
$field_config constructor.

1. Minor : Not sure why the '$config['deleted'] = TRUE' line is needed, shouldn't that already be the case ?
2. The previous code injected the storage through the 'field_config' property, which triggered some logic in FieldConfig::__construct() (mostly, setting the internal $this->field_name & $this->entity_type properties).
   The new code injects it through 'fieldStorage', which bypasses the above, so the resulting FieldConfig has no field_name / entity_type properties.
   Chances are, those properties are most likely not used in typical code paths involving a deleted field, but that still feels like an oddity that could bite someone at some point ?
   What was the reason to go from 'field_storage' to 'fieldStorage' ?
3. At the end of the day, that's FieldConfig-specific code baked in \Drupal\Core...
   It's triggered by an 'instanceof FieldConfigBase' check, so the class doesn't reach into the \Drupal\field namespace, but FieldConfigBase is broader, and currently encompasses BaseFieldOverride as well.
   BaseFieldOverrides have no reason to ever be added to the DeletedFieldsRepository, and the current code probably wouldn't break if they were, but still this feels a bit brittle, for the same kind of reasons that @plach pointed in #62 : we can't really know what extends FieldConfigBase ?
   Just pointing this for now, didn't spend too much time thinking how this could be cleaner :-)

That's actually the part that I struggled with the most in this patch. But now that you mentioned it and I looked at it again for the Nth time, I think there's a very simple solution for it: make FieldConfig::getFieldStorageDefinition() query the 'field_storage_config' storage, including deleted fields, instead of looking at the storage definitions given by the entity manager.

Let's see if that works at least.

Ok, so that won't work :( I'll just answer #67 then:

1. That's because the deleted property of the FieldConfig object is lost when we call toArray() on it just above.

2. No real reason, I didn't look closely so I didn't realize that we're doing some extra stuff when we pass in field_storage instead of fieldStorage :)

3. No other ideas on how to improve that..

Interdiff is to #65.

Ok then..

That's because the deleted property of the FieldConfig object is lost when we call toArray() on it just above.

Aw, right. Deleted fields aren't stored in config, so 'deleted' is not part of the config schema, so it does not get exported by toArray().
Might be worth a comment ? (if the suggestion below fails and that code has to stay)

Not sure why #68 failed, but yeah, having FieldConfig::getFieldStorageDefinition() read from confg storage in all cases, rather from the in-memory EntityFieldManager repository currently, would be a perf regression ?

But then what if FieldConfig::getFieldStorageDefinition() was doing something like :
- read from $this->entityManager()->getFieldStorageDefinitions(), as currently
- if not found and $this->deleted, then fallback to the DeletedFieldsRepository
- if still not found, Exception, as currently
?

(heh, FieldConfig::getFieldStorageDefinition() uses a $field var to store a FieldStorage :-), that one must have slipped through the Great Field / Storage Rename)

@yched, I tried to do exactly that for a couple of hours last night but I couldn't get it working. However, it turns out that I just didn't have enough patience with the debugging session, because I tried again today and found the problem immediately:

+++ b/core/lib/Drupal/Core/Field/DeletedFieldsRepository.php
@@ -0,0 +1,119 @@
+    // Add a helper property that allows us to filter by the field storage
+    // unique identifier in getFieldDefinitions().
+    $field_definition->field_storage_unique_id = $field_definition->getFieldStorageDefinition()->getUniqueStorageIdentifier();

This line was causing the "old" (non-deleted) field storage object to be persisted with the (deleted) field definition.

The funny thing is that we're not even using that helper property anymore in getFieldDefinitions() so removing it was a double win :)

With that said, here's a patch which implements #73 and brings the funkiness in the deleted fields repository down to zero! Thanks for your persistence ;)

Since we're pretty much down to nitpicks (I hope), I'll start with one that has been bugging me for a while.

+++ b/core/core.services.yml
@@ -576,9 +570,12 @@ services:
+    arguments: ['@entity_type.manager', '@event_dispatcher', '@entity.last_installed_schema.repository', '@entity_field.manager', '@field.deleted_fields_repository']

I wonder if the name of the service could be improved. Having 'field' as the first item in the "namespace" makes it look like it is provided by the field module. Should we rename it to 'entity_field.deleted_fields_repository'?

Interdiff looks good to me, so happy we got rid of that dynamic property!

Should we rename it to 'entity_field.deleted_fields_repository'?

+100 :)

I'll let @yched confirm we are good here, but looking forward to moving this back to RTBC ;)

Nice, let's do that!

#74 : w00t, super nice ! Thanks for *your* persistence :-)
#75 : +100 as well

Down to nitpicks indeed :

I stumbled on a couple code comments in DefaultTableMapping (getDedicatedDataTableName(), getDedicatedRevisionTableName(), generateFieldTableName()) mentioning the "field UUID" being used in table names.
Those were already outdated ("storage UUID" rather than "field UUID" - yet another leftover from The Great Rename), but now it's also not always a UUID :-)

Other than that, RTBC +1 :-D

Side note : it's still field.module's hook_cron() that triggers the actual purge for config and base fields alike.
I guess this is left for #2907780: Add a field purgatory service to change ?

#78 good point, and easy to fix too :)

Edit: #79: yup, that's exactly what we need to do in that issue.

Thanks @amateescu !
RTBC++

Yay!! Thanks @yched and @plach for all the reviews!

Hope to see you again in #2907780: Add a field purgatory service :D

1. +++ b/core/lib/Drupal/Core/Entity/DynamicallyFieldableEntityStorageInterface.php
   @@ -18,27 +16,4 @@
   -  public function purgeFieldData(FieldDefinitionInterface $field_definition, $batch_size);
   ...
   -  public function finalizePurge(FieldStorageDefinitionInterface $storage_definition);
   

   If the interface is now empty - do we need it? We have a base class, so under our api guidelines its not a BC break right?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1724,15 +1720,12 @@ public function countFieldData($storage_definition, $as_bool = FALSE) {
   +   *   \Drupal\Core\Field\FieldStorageDefinitionInterface::isDeleted() instead.
   

   can we reference the change notice here?

3. +++ b/core/lib/Drupal/Core/Field/DeletedFieldsRepository.php
   @@ -0,0 +1,95 @@
   +class DeletedFieldsRepository implements DeletedFieldsRepositoryInterface {
   
   +++ b/core/lib/Drupal/Core/Field/DeletedFieldsRepositoryInterface.php
   @@ -0,0 +1,71 @@
   +interface DeletedFieldsRepositoryInterface {
   

   Should we mark these internal?

Re #83:

1. We're not yet sure what to do with the empty interface, and this patch is big enough so I'd like to handle it in the followup mentioned in #82.

2. Sure thing ;)

3. I don't think so, that's very much the public API for dealing with deleted fields.

#84.3 should anyone be calling this API though?

@catch, not really :) Would that be a reason for marking it @internal? BTW, I'm not opposed to it, I just didn't find any good reason to do that.

I think it's a good reason - makes it clear it's an implementation detail, although it's starting to make me wonder if we even want an interface now.

Ok, let's mark it as @internal then. I thought all services are somewhat required to have an interface, but I don't have any opinion on that.. TBH I just want this gone from my radar :)

A service is swappable by definition, more or less, so IMO it's always worth an interface, even if it's for @internal purposes :)

Back to @catch

Committed 20564b9 and pushed to 8.5.x. Thanks!

Yay! Awesome work @amateescu!

The changes to tests introduced here can (do) fail with contrib db drivers. Small follow-up created #2925750: EntityDefinitionUpdateTest fails with contrib db driver.

I have recently upgraded my site to D8.5-dev with this commit and now when I running cron on my site, I got the following error:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getTargetEntityTypeId() on array in field_purge_batch() (line 82 of core/modules/field/field.purge.inc).
field_purge_batch(50) (Line: 167)
field_cron()

More info here #2931436: field_purge_batch expects an array of objects but instead gets an array of arrays