Deleted Target Field Throws Fatal Error

Do you want to provide a patch to fix the issue? I think the Feeds configuration should somehow create a dependency on the field in question. This way you'll be notified that a Feeds configuration is dependent on the field the moment you delete the field in the UI.

Rolled a quick patch that handles the whitescreen. It doesn't take care of the field dependency so leaving this as needs work. this should at least let the module work for now.

Alright, while the above patch does allow a user to fix the mappings manually it doesn't actually fix the problem when you try to run a feed with an obsolete mapping (it whitescreen...). I'm thinking the dependency would fix that as long as it kept the user from deleting the field if it was referenced in a mapping.

Would it be better to delete the mapping if the user goes through with deleting the field after being warned that it is in use, or would it be better to to keep the user from deleting the field if it is a mapping target? I'm not sure if there is a standard way to create such a dependency or if it really just a case by case basis

From a UX standpoint, I think it is better that the mapping target in question is deleted as well. I think it like this: the user wants to delete a field, then he/she doesn't want another obstacle to tackle first. With the warning displayed, the user can then decide itself if it is worth to check the Feeds mapping configuration first.

In the D7 version of Feeds you could just delete a field without error. The target would then show as 'missing' on the mapping configuration and the import would continue and just skip that target.

I think the solution in D7 would not be enough for the D8 version (though it would be nice if an import doesn't result into a fatal error when a field is missing). You should at least know what impact deleting the field will have. I think the best way to do this is by declaring a dependency, although I'm not sure how to do this.

Thanks for taking the time to investigate this issue. :)

It wouldn't take too much to hotfix this to at least make it useable for now. I ended up doing so on my local for one of the parsers as I couldnt figure out a way to get of the mapping without either messing around in the database/config files or re adding a field with the same machine name for long enough to delete the mapping. It really just boils down to adding a continue statement where ever the getPluginId method fails to return a valid plugin. I'll put up an interim patch with those statements when I have a chance.

This patch allows the mappings to be edited and the feeds to be run. Doesnt add the dependency, still looking into that

Thanks for working on this, @wolffereast!

I think though that throwing an exception in getTargetPlugin() would be better from an architectural standpoint. What do you think?

Also noticed that \Drupal\feeds\FeedTypeInterface doesn't mention the getTargetPlugin() method, even though other code that is receiving a FeedTypeInterface object expect that this method exists. Should probably be an other issue though.

No worries!

Throwing an error in getTargetPlugin definitely makes sense. Should the error kill the import entirely or just get caught and logged? At this point there is no way to get rid of a mapping that targets a deleted field, so I think catching and logging might be the way to go.

I dove into the dependency stuff a bit the other day and found that it goes way deeper than I originally assumed. The output you see when you delete a field comes from a dependency calculation run in \Drupal\Core\Config\Entity\ConfigEntityBase. I'm not sure what trait or class is required for a mapping to add a dependency to a field. Lots of reading to do.

for reference: https://www.drupal.org/docs/8/api/configuration-api/configuration-entity...

If we cant add a dependency and handle it in that process I think it is also possible to remove the mapping by subscribing to the field storage delete event.

I think fields should be listed as dependency in \Drupal\feeds\Feeds\Processor\EntityProcessorBase::calculateDependencies(). There is already a dependency on entity type there.

Hey,

After applying the patch. The Mapping tab content gets loaded with a blank row for the deleted field.

Hi,

I this what we are suppose to do in calculateDependencies method ?

// Add dependency on entity fields.

    $entity_field_provider = \Drupal::entityFieldManager()
      ->getDefinition($this->entityField())
      ->getProvider();
    $dependencies['module'][$entity_field_provider] = $entity_field_provider;

Also, how do we test that the dependencies are being set.

Thanks for helping me here.

Regards
Vijay

@vijay.mayilsamy
Thanks for looking at this. I haven't figured out myself how to do this. A dependency on the module is however not enough. There should be relation between the field and the feed type. Ideally, the feed type is updated when the field is deleted. It would be a good idea to examine how Views handles this: what happens when you try to delete a field that is used in a View? Is the view deleted? If so, updating the feed type may be too hard and sadly the deletion of the field would then mean the deletion of the feed type as well. Does the view get automatically updated? Then that should somehow be possible to do in Feeds as well.

This is a start. It adds dependencies when saving the feed type. But it should also update the feed type when deleting a field. Right now, with this patch it will try to delete the feed type when deleting a field.

This fixes the issue and adds a test! I also found out that a feed type should have a dependency on the field, not the field storage.

Committed #16 with a few small changes in the code comments.