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.
Comment | File | Size | Author |
---|---|---|---|
#26 | feeds-simplify-target-callbacks-2093651-26.patch | 15.82 KB | twistor |
#24 | feeds-simplify-target-callbacks-2093651-24.patch | 16.71 KB | twistor |
#20 | feeds-simplify-target-callbacks-2093651-20.patch | 15.75 KB | twistor |
#17 | fields_packed.zip | 6.73 KB | MegaChriz |
#17 | fields_packed_tamper.zip | 2.89 KB | MegaChriz |
Comments
Comment #1
twistor CreditAttribution: twistor commentedLet's see what breaks.
Comment #3
twistor CreditAttribution: twistor commentedComment #5
twistor CreditAttribution: twistor commentedComment #6
twistor CreditAttribution: twistor commentedMove cardinality checking to post-process.
Comment #7
twistor CreditAttribution: twistor commentedAdded 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.
Comment #7.0
twistor CreditAttribution: twistor commentedMore details.
Comment #8
MegaChriz CreditAttribution: MegaChriz commented@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.
Comment #9
hcethatsme CreditAttribution: hcethatsme commentedInstalled 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!
Comment #10
twistor CreditAttribution: twistor commented@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?
Comment #11
twistor CreditAttribution: twistor commented@hcethatsme, what issue specifically does it solve?
Comment #12
ditcheva CreditAttribution: ditcheva commentedWorks for me too! Fixes the issue I described in https://drupal.org/node/1107522#comment-7880947
Comment #13
MegaChriz CreditAttribution: MegaChriz commented@twistor, #10
Makes sense. Great idea to standardise this. I was just trying to figure out what the impact of this API change would possibly be.
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.
Comment #14
twistor CreditAttribution: twistor commentedI 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.
Comment #15
twistor CreditAttribution: twistor commentedThis changes the post_process callback to be an array of callbacks so that we can add more if needed.
Comment #16
twistor CreditAttribution: twistor commentedAdd mapping as an argument to post_process.
Comment #17
MegaChriz CreditAttribution: MegaChriz commentedI'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:
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:
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:
Comment #18
MegaChriz CreditAttribution: MegaChriz commentedE-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:
SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'fid' cannot be null
andFailed importing 1 node.
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.
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).
Comment #19
ditcheva CreditAttribution: ditcheva commentedHi 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!
Comment #19.0
ditcheva CreditAttribution: ditcheva commentedUpdated issue summary.
Comment #20
twistor CreditAttribution: twistor commentedMegaChriz++
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.
Comment #22
twistor CreditAttribution: twistor commented20: feeds-simplify-target-callbacks-2093651-20.patch queued for re-testing.
Comment #24
twistor CreditAttribution: twistor commentedOne measly s.
Comment #26
twistor CreditAttribution: twistor commentedI suppose I should take out my debug code.
Comment #27
twistor CreditAttribution: twistor commentedAlright, this is committed.
http://drupalcode.org/project/feeds.git/commit/363c03b
Comment #28
ditcheva CreditAttribution: ditcheva commentedYay! Thanks, @twistor!
Comment #29
MegaChriz CreditAttribution: MegaChriz commentedAlright, 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).