Log a message when skipping deleted fields during field_purge_batch()

Looks good, would be very helpfull to show this logging.

Is calling \Drupal::entityManager() depricated and the reason you replaced it with an $entity_manager variable?

Can we get another review?

Here's a review :)

1. +++ b/core/modules/field/field.purge.inc
   @@ -82,18 +82,19 @@ function field_purge_batch($batch_size, $field_storage_uuid = NULL) {
   +  $entity_manager = \Drupal::entityTypeManager();
   +  $info = $entity_manager->getDefinitions();
   

   The variable should be called $entity_type_manager.

2. +++ b/core/modules/field/field.purge.inc
   @@ -82,18 +82,19 @@ function field_purge_batch($batch_size, $field_storage_uuid = NULL) {
   +      \Drupal::logger('field')->warning("Cannot remove field because the entity type is unknown: %entity_type", ['%entity_type' => $entity_type]);
   

   We should mention the field name in the message.

3. +++ b/core/modules/field/field.purge.inc
   @@ -82,18 +82,19 @@ function field_purge_batch($batch_size, $field_storage_uuid = NULL) {
   +    $count_purged = $entity_manager->getStorage($entity_type)->purgeFieldData($field, $batch_size);
    
   -    $count_purged = \Drupal::entityManager()->getStorage($entity_type)->purgeFieldData($field, $batch_size);
   

   The new line seems to have been detached from the code below it, let's put it back :)

Updated patch as per suggestion from @amateescu on comment #9 & also added interdiff for this patch.

Looks great now :)

Thanks for working on this! A lot of people run into difficulties debugging field purging, so this seems like a good idea.

The entity type manager change isn't really in scope at all... it's not used in the line being added. This is a one-line patch with seven lines in it. :P Usages of the deprecated entity manager should be removed across all of core in a single patch or a set of patches grouped by concept. References: https://www.drupal.org/core/scope#creep and also the section on incomplete scope. This patch is small enough that it's still easy to review, but ideally, we should still remove those changes from the patch.

+++ b/core/modules/field/field.purge.inc
@@ -82,18 +82,19 @@ function field_purge_batch($batch_size, $field_storage_uuid = NULL) {
-    // @todo Revisit after https://www.drupal.org/node/2080823.

I don't think we're actually revisiting it in this issue, just adding a logged message to the same code without revisiting the code itself? So probably we shouldn't be removing this @todo either. The actual issue for it is #2343517: Cleanup @todo referring to the config dependencies API issue. Probably the @todo should have referenced that issue instead of the old one to avoid exactly this sort of confusion, but this is another reason not to make out-of-scope changes. :)

A novice could remove the unneeded hunks from this patch, and change it to just be the single line for the logger.

Thanks!

Updated patch as per suggestion by @xjm in comment #13

A smaller scope for the patch is good as well :)

Cool, that works. Committed and pushed to 8.5.x. Thanks!