Currently there is way too much boilerplate for each target callback.

This patch allows every target callback to assume that incoming values are an array so that each target doesn't have to check. This should be backwards compatible unless there are targets out in the wild that assume the value is singular. While that's possible, it assumes knowledge of the source and that's technically incorrect.

For most targets, cardinality checking is removed. Cardinality is fixed by field.module automatically, so it's useless for the most part. The exceptions to this are taxonomy and file. Those targets are particularly expensive, so they shouldn't to more processing than necessary.

This patch also adds a new callback in the mapping array called 'post_process'. This allows us to remove empty value checking from field targets and move it to a separate step. That way, we can easily re-use _field_filter_items() that magically filters empty values.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

twistor’s picture

Status: Active » Needs review
FileSize
12.72 KB

Let's see what breaks.

Status: Needs review » Needs work

The last submitted patch, feeds-simplify-target-callbacks-2093651-1.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
17.31 KB

Status: Needs review » Needs work

The last submitted patch, feeds-simplify-target-callbacks-2093651-3.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
17.45 KB
twistor’s picture

Move cardinality checking to post-process.

twistor’s picture

Added back cardinality checking for taxonomy and file targets since those are expensive.

Added a test case.

Updated docs.

This should be very close to being ready, but needs manual testing.

twistor’s picture

Issue summary: View changes

More details.

MegaChriz’s picture

@twistor
I haven't tried the patch nor studied the code deeply yet, but are you saying that a target value will now always be an array? Or does this only count for targets that are a field from the Drupal core Field API?
(I have written a module that supplies targets for entity properties that are not Field API fields and these properties are not multivaluable, so the targets currently expect to get "single" values.)

I hope to test the patch in a few days.

hcethatsme’s picture

Installed and tested the patch--no trouble, and it fixes the issue in #1107522: Framework for expected behavior when importing empty/blank values + text field fix. Thanks!

twistor’s picture

@MegaChriz, Yes, the target value will always be an array. That's why I'm testing the waters with this. While mapping to a property, rather than a field, is perfectly valid, the target callback should still be able to handle multiple values. Because of the generic nature of Feeds, any source can be mapped to any target. See path.inc.

Currently, Feeds targets do a good job of this, but processor's setTargetElement() methods are all broken with the same assumption. I've seen many SQL issues where someone mapped a multi-valued source to a node title for example.

In the 8.x-3.x I've fixed this by assuming that all values are an array. It greatly simplifies things, since targets that expect one value can simply $value = reset($values); Feeds doesn't currently offer any guarantee about about the number of values being passed in, so expecting multiple is the only sane option.

What is the module?

twistor’s picture

@hcethatsme, what issue specifically does it solve?

ditcheva’s picture

Works for me too! Fixes the issue I described in https://drupal.org/node/1107522#comment-7880947

MegaChriz’s picture

@twistor, #10

In the 8.x-3.x I've fixed this by assuming that all values are an array. It greatly simplifies things, since targets that expect one value can simply $value = reset($values); Feeds doesn't currently offer any guarantee about about the number of values being passed in, so expecting multiple is the only sane option.

Makes sense. Great idea to standardise this. I was just trying to figure out what the impact of this API change would possibly be.

What is the module?

Ubercart Addresses 7.x-1.x (used as an address book solution for Ubercart) integrates with Feeds: it provides a processor for an Ubercart Addresses address and mapping targets for each address field of this entity type. It has a basic automated test for integration with Feeds. The mapping target implementation is a bit different compared to other mapping target implementations in that it hands that job over to the implementation of the address field itself (or better said: it's handler).
Anyway (don't want to go too much in detail here as it's a bit off-topic), it should be not much of a problem for me to adjust the implementation and be aware of the fact that all values will be provided as arrays.

twistor’s picture

I looked through Ubercart Addresses, and I don't think this change will affect it. This change would only be for target callback functions. Processors that implement setTargetElement() aren't affected.

I don't really consider this an API change, more of an API clarification, since target callbacks *should* be able to handle multiple values. Fixing setTargetElement() would be another issue.

That said, we can get most of this in without the API fix, and just add the post_process callback, as that's what is fixing the issue.

I still would like to know if there are any target callback functions in the wild that expect a single value.

twistor’s picture

This changes the post_process callback to be an array of callbacks so that we can add more if needed.

twistor’s picture

Add mapping as an argument to post_process.

MegaChriz’s picture

I've done some basic manual tests with the patch provided in #16. I also looked for modules that supply mapping targets for Feeds. I haven't studied the changed code itself yet.

In short, this is how I tested:

  • For all tests, the HTTP Fetcher, CSV parser and Node processor were used.
  • All tests were done with the setting "Update existing nodes" enabled.
  • I tested importing single values, two values and empty values.
  • I mapped to a bunch of different target types: guid, title, user_name, created, boolean, date, email, entity reference, image, link, number, text, name, address, path alias.
  • I used Feeds Tamper's explode function to test importing two values per target.

Modules providing mapping targets for Feeds

Test results (so far)

There were no fatal errors during the tests. All imports went smoothly most of the time (see further for details).
Summary:

  • Single values: all fields got the correct value.
  • Two values: most fields got the correct value. When supplying an array for the target "title", the title of the node became "Array".
  • Empty values (or non-existing): all Feeds core targets get cleared out. Some targets from other modules did not completely clear out (their labels stayed on the node page): address (from Address Field), email (from Email Field) and name (from Name Field). From the Name module the first value became an empty string, but the second value stayed there.

I hope to do some more tests later. I see I forgot to include taxonomy and weight in this test. I also didn't try the mapping targets from Ubercart Feed Mappers and the Data module yet.

Attached my configuration in two feature modules:

  • One feature module with node type and feeds importer (called "fields_packed").
  • One feature module with Feeds Tamper configuration (called "fields_packed_tamper").
MegaChriz’s picture

E-mail field: empty values are not cleared out.
See also #718414: Feeds mapper for email field.

Image: when using a not valid URL in the source:

  • Without the patch applied: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'fid' cannot be null and Failed importing 1 node.
  • With the patch applied: Updated 1 node.

Taxonomy: empty value created new term (with mapping option "Auto create" enabled).
I didn't notice any difference in behaviour with or without the patch applied. Issue from #1668186: Taxonomy terms auto-created with empty name. still applies.

Weight module: expects a single value.

  • Without the patch applied: weight value was imported with success. Empty values were ignored.
  • With the patch applied: no weight values were imported. Nodes with existing weight values were not overwritten.

See #2095189: Fix Feeds integration. for a proposed fix for this module.

Here is an overview of targets where something failed with, with the patch applied (so targets where something failed with without the patch applied but not with the patch applied are left out from this overview).

Without patch With patch from #16
Target Single value Two values Empty value Single value Two values Empty value
Title Success Not tested Success Success Failed Success
Address field Success Not tested Failed Success Success Failed
Email field Success Not tested Failed Success Success Failed
Name field Success Not tested Failed Success Success Failed
Taxonomy reference Success Not tested Failed Success Success Failed
Weight Success N/A Failed Failed N/A Failed
ditcheva’s picture

Hi all,

I came to this issue queue from over here, and just want to log the exact problem this solves for me and for the user from comment #9. The bug these patches solve isn't listed here, so I want to move it over (the patch in comment #161, which is where I first reported this bug, does not solve it).

Here is the issue I reported when I was sent to this queue
================================
With a node importer set to replace nodes upon future feed imports and the simplest of import .csv files, I created three nodes with three text fields.
---------------
Name;Job title;Bio
Boriana Ditcheva;Web developer;Lorem ipsum dolor sit amet, consectetur adipiscing elit.
Someone Else;Other job;Lorem ipsum dolor sit amet, consectetur adipiscing elit.
Test person;Data Analyst;
---------------

The three nodes are created beautifully! Then I deleted two of the fields from the import, however, those fields REMAIN in the original nodes - i.e they are not updated or replaced or removed: (Note below that Boriana's Job title is deleted and 'Someone Else's' Bio is deleted):

---------------
Name;Job title;Bio
Boriana Ditcheva;;Lorem ipsum dolor sit amet, consectetur adipiscing elit.
Someone Else;Other job;
Test person;Data Analyst;
---------------

That's the bug I'm experiencing. Once a text field has been imported, no further imports can remove a field. I can update it to other text, but I can't just change it to empty or blank...
================================

In any case, what's the status for this issue? Is there anything else I can help with or anything else I can test. I'd love to help any way if I can.

Thanks!

ditcheva’s picture

Issue summary: View changes

Updated issue summary.

twistor’s picture

MegaChriz++

Getting back to this issue.

I'm going to break this up into 2 parts, so that it's more manageable. The first part, will just be converting $values to always be an array.

Status: Needs review » Needs work

The last submitted patch, 20: feeds-simplify-target-callbacks-2093651-20.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 20: feeds-simplify-target-callbacks-2093651-20.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
16.71 KB

One measly s.

Status: Needs review » Needs work

The last submitted patch, 24: feeds-simplify-target-callbacks-2093651-24.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
15.82 KB

I suppose I should take out my debug code.

twistor’s picture

Assigned: twistor » Unassigned
Status: Needs review » Fixed
ditcheva’s picture

Yay! Thanks, @twistor!

MegaChriz’s picture

Alright, now let's see if my test in #1107522: Framework for expected behavior when importing empty/blank values + text field fix passes (which failed because a zero was treated as empty).

Status: Fixed » Closed (fixed)

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