Problem

Sometimes during development fields get removed and the entity that provided gets deleted before the fields are purged via field_purge_batch() leaving field_deleted_* tables in the database that can't be removed.

Proposed resolution

Provide a warning when skipping to tell the administrator that the fields can't be removed because the entity is gone. Hopefully giving them a clue that the entity needs to be re-created for the purge to go through.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Status: Active » Needs review
FileSize
1.36 KB

Ideally I'd prefer if they got deleted but this seems to be a safer patch in my head.

joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Title: Provide more fedback when skipping deleted fields » Provide more feedback when skipping deleted fields during field_purge_batch()

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

Issue tags: +migrate

This happened during field config migration, so tagging for maybe more exposure.

scuba_fly’s picture

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?

joelpittet’s picture

@scuba_fly, yes I removed \Drupal::entityManager() because it's deprecated AND I needed access to $entity_manager anyway later in the code, so avoid calling it twice, though maybe superfluous, it's straightforward.

/**
* @deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0.
* Use \Drupal::entityTypeManager() instead in most cases. If the needed
* method is not on \Drupal\Core\Entity\EntityTypeManagerInterface, see the
* deprecated \Drupal\Core\Entity\EntityManager to find the
* correct interface or service.
*/
public static function entityManager() {

amateescu’s picture

Status: Needs review » Needs work

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 :)

Yogesh Pawar’s picture

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

Yogesh Pawar’s picture

Status: Needs work » Needs review
amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks great now :)

xjm’s picture

Title: Provide more feedback when skipping deleted fields during field_purge_batch() » Log a message when skipping deleted fields during field_purge_batch()
Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice

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!

Yogesh Pawar’s picture

Assigned: Unassigned » Yogesh Pawar
Yogesh Pawar’s picture

Assigned: Yogesh Pawar » Unassigned
Status: Needs work » Needs review
FileSize
661 bytes

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

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

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

  • xjm committed f16783a on 8.5.x
    Issue #2862308 by Yogesh Pawar, joelpittet, amateescu: Log a message...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.