Add a field purger service

Discussed this with @amateescu and we agreed to re-scope this to perform all the final clean-ups to make sure the Entity Field API is fully capable of dealing with purging of any field, flavor including non-configurable bundle fields, and get rid of any dependency on the Field module.

Nice :)

1. +++ b/core/lib/Drupal/Core/Field/FieldPurgatory.php
   @@ -0,0 +1,140 @@
   +      // We cannot purge anything if the entity type is unknown (e.g. the
   +      // providing module was uninstalled).
   +      // @todo Revisit after https://www.drupal.org/node/2080823.
   ...
   +      // We cannot purge anything if the entity type is unknown (e.g. the
   ...
   +      // @todo Revisit after https://www.drupal.org/node/2080823.
   

   Are these comments still valid? The pointed issue is closed/fixed.

2. +++ b/core/lib/Drupal/Core/Field/FieldPurgatory.php
   @@ -0,0 +1,140 @@
   +    // Retrieve all deleted field storages. Any that have no fields can be purged.
   

   80 chars ;)

3. +++ b/core/modules/field/tests/src/Kernel/BulkDeleteTest.php
   @@ -281,7 +281,7 @@ public function testPurgeWithDeletedAndActiveField() {
   +    \Drupal::service('entity_field.purgatory')->purgeBatch(50);
   
   @@ -326,7 +326,7 @@ public function testPurgeField() {
   +      \Drupal::service('entity_field.purgatory')->purgeBatch($batch_size);
   
   @@ -352,7 +352,7 @@ public function testPurgeField() {
   +    \Drupal::service('entity_field.purgatory')->purgeBatch($batch_size);
   
   @@ -385,7 +385,7 @@ public function testPurgeFieldStorage() {
   +    \Drupal::service('entity_field.purgatory')->purgeBatch(10);
   
   @@ -403,7 +403,7 @@ public function testPurgeFieldStorage() {
   +    \Drupal::service('entity_field.purgatory')->purgeBatch(0);
   
   @@ -422,7 +422,7 @@ public function testPurgeFieldStorage() {
   +    \Drupal::service('entity_field.purgatory')->purgeBatch(10);
   
   @@ -440,7 +440,7 @@ public function testPurgeFieldStorage() {
   +    \Drupal::service('entity_field.purgatory')->purgeBatch(0);
   
   +++ b/core/modules/field/tests/src/Kernel/FieldImportDeleteTest.php
   @@ -107,15 +107,15 @@ public function testImportDelete() {
   +    $deleted_storage_definitions = \Drupal::service('entity_field.deleted_fields_repository')->getFieldStorageDefinitions();
   ...
   +    $deleted_storage_definitions = \Drupal::service('entity_field.deleted_fields_repository')->getFieldStorageDefinitions();
   
   +++ b/core/modules/field/tests/src/Kernel/FieldImportDeleteUninstallTest.php
   @@ -150,8 +150,8 @@ public function testImportAlreadyDeletedUninstall() {
   +    $deleted_storage_definitions = \Drupal::service('entity_field.deleted_fields_repository')->getFieldStorageDefinitions();
   
   @@ -161,8 +161,8 @@ public function testImportAlreadyDeletedUninstall() {
   +    $deleted_storage_definitions = \Drupal::service('entity_field.deleted_fields_repository')->getFieldStorageDefinitions();
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -444,7 +444,7 @@ public function testBaseFieldDeleteWithExistingData() {
   +    \Drupal::service('entity_field.purgatory')->purgeBatch(10);
   
   @@ -515,7 +515,7 @@ public function testBundleFieldDeleteWithExistingData() {
   +    \Drupal::service('entity_field.purgatory')->purgeBatch(10);
   

   It would be nice if we instantiated the service just once :)

Or, is it more like a Recycle bin? Where you place data before you decide it should be deleted for good.

Therefore, is this the beginning of a Trash bin in core for content?

Edit:
Well, not the beginning, as @amateescu will surely point out: https://www.drupal.org/project/trash

Yay, #2282119: Make the Entity Field API handle field purging just landed!

According to #2, this is still postponed on #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info().

The thing here is that we have #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted which is postponed on this one and #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted is critical (and blocking Migrate Drupal from coming stable).

So the chain is #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted depends on #2907780: Add a field purger service which depends on #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info().

@amateescu or anyone else, can you confirm that I've understood the dependencies correctly? If yes, should we bump this and #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info() to critical as well?

Cheers,
Markus

If I'm understanding you correctly, this "purgatory" service is not like a trash bin because some process to delete the items in the bin is actively processing. Despite that active purging process, it is still possible to save the items, if you act strategically and quickly.

Like: https://youtu.be/QtQPmDjuA5s

If that's the case, why would we allow that saving of items that have been marked for purging and are actively being purged?

And that description does not match a recycle bin because the directive to finally delete the data has been given and a process is actively deleting data that has already passed that system-driven check of whether something needs that data or not?

Because without that context of having a processing actively deleting data underway, what you've just described is a trash bin / recycle bin.

Will this service delete fields marked with deleted flag when base entity for this field does't exist anymore? I have 3 ghost fields marked with deleted flag, but field_cron() can't delete them because commerce changed entity name without update its fields on alpha->beta stage.

This kind change needs to go into the development branch first. Backport is considered afterwards.

@plach what should we do with ghost fields?

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-rolled patch against 10.1.x. Please review

Tried to fix CCF on #33
attached interdiff file

added wrong patch file on #34
Tried to fix CCF on #33
attached interdiff file

Fix CCF on #33 & Attached Interdiff file

This could use an update now that OOP hooks are in.

I have re-rolled the patch and used OOP hooks to purge the field definitions. I did it as a part of contribution sprint at DrupalCon Singapore 2024.

Hi @anmolgoyal74

The reroll seems to be good.

Fix PHPCS Violations:
Go through the generated PHPCS report and resolve the coding standard violations. This may involve reformatting the code, adding missing docblocks, or updating variable names.

Address PHPStan Issues:
Review the PHPStan analysis and resolve any potential type mismatches, missing variables, or logical errors.

Went through it a couple of times.

Just one question in the mr.

Comment seems valid.

Also will need test coverage for the update hook around the config updates.

I think this is ready for review again!

I rebased on main
I updated deprecation versions
I added a deprecation to the file itself

I'm not sure on the system update number

The only bit from the rebase was moving where the field_purge_batch was called from since that test was refactored to handle update multiple types

There is some discussion about this being postponed, but I don't see any reason to postpone this.
I took care of my comment from the other MR but it was trivial, just copying a log.

I'm not sure what test coverage updates like that usually get, do you have an example?

I also removed the require once for the purger and the outdated comment.

I don't see any discussion about moving that config, why was it moved to system, I'm not sure we want to do that do we?

Edit: nevermind, it's because the purger service was moved to the core field subsystem.

Discussed this on slack a bit, there really doesn't seem to be a need for the interface since it doesn't look like it needs to be fully swapped out.

If someone needs more they can subclass it and still override it.

This needs another rebase.

Let's try to bring it to finish

summary needs to be polished and config settings needs to be mentioned in CR and schema deprecation possibly provided

#3571593-19: Deprecate translation.path config in favor of settings details how we should handle the config to settings I think.

https://git.drupalcode.org/project/drupal/-/merge_requests/14644#note_72...

Pushed this a bit further.

I removed the new config added the old back with the deprecation.

I added the new settings.

I need to remove the update hook and replace it with the correct post update hook.

I updated the Issue summary and Change record.

I finished converting this to follow the pattern from the translation path issue.

I'll keep an eye on tests, but this should be ready.

Meant to set needs review!

Did a review, see the MR

I have addressed all feedback but deprecating field_purge_field and field_purge_field_storage without replacement.

I asked @amateescu on slack and he has it on his list to review.

Tagging for subsystem maintainer review.

Thank you, @nicxvan. There's still one thing left related to the visibility of FieldPurger::purgeFieldDefinition() and FieldPurger::purgeFieldStorageDefinition(). Let's wait for a 3rd opinion

Yes, sorry that is what I meant, if we change the visibility then we need to deprecate without replacement.

I now agree with you we should, but it's really a decision for the field maintainer so I'll hold off for now.

I made them protected, updated the deprecations and addressed all feedback.

Ready to RTBC this but I've just noticed we need to update the CR https://www.drupal.org/node/3494023 because of the methods made protected.

Thank you. CR is reflecting now the changes. I cannot see anything else, this is ready.

Reviewed this and left a couple of minor questions/nits

This is really close - would be good to resolve if

Note quite happy with putting more stuff in system cron.

was meant to be

Not quite happy with putting more stuff in system cron.

Because a one letter typo gives it a whole different meaning :)

I addressed all of your feedback thanks!

I didn't change the logger channel for the reason that @berdir stated.

I did see that potential typo, I only didn't reach out for clarification because this:

We could make it a separate cron hook, but ultimate_cron doesn't really support that yet right now due to invoke() only supporting a single hook for the given module

@larowlan said I could self rtbc, one is addressed the feedback.

Nice work everyone - great to see this finally nearly done. Can we get a follow-up to remove the field.settings config object from Drupal 13.

One thing we need to address... we remove the config from config/install but we leave it lying around empty in the post update. See comment on MR.

#3587460: Remove the field.settings config object from Drupal 13 Here we go!

Should we add this as a comment somewhere?

The suggestion was applied and looks correct. I've created the follow up and @alexpott confirmed we do not need a todo anywhere so I think all feedback has been addressed.

Just noticed another thing in the update... sorry.

I've applied the suggestion.

Committed and pushed 867318c3996 to main and bfd87c3dda1 to 11.x. Thanks!

The config name and its key were misnamed in the commit and the change record. The config name field.settings became field.field_settings, and its key purge_batch_size - field_purge_batch_size for some reason.

I opened a follow-up issue - #3622469: Fix field.settings:purge_batch_size config name and key and updated the CR.

Also, shouldn't the new setting be documented in default.settings.php?