Problem/Motivation
In Feeds 7.x-2.0-alpha8 and earlier there was some inconsistency with importing "blank" values (0
, NULL
or ""
). Some mappers ignored empty values, some overwrote the target value.
The patch from #161 was committed to fix this inconsistency and always overwrite the target value, even if the field was not in the source.
There are some different kinds of expectations around importing blank values:
- Some expect blank values should always be ignored.
- Some expect blank values should always overwrite the target.
The first behaviour is how it was for most mappers in 7.x-2.0-alpha8 and earlier, the second behaviour is how it is now in dev. Providing the best of both worlds was not possible for the following reasons:
- With the current architecture, the distinction of non-existent vs. empty can not be made for a generic solution. (See #147, #160)
- Making every field configurable as to how it handles empty values is outside the scope of Feeds. (See #162)
Proposed resolution
Always overwrite the target value. Patch for this was committed. For the behaviour "ignore empty values" there is now the experimental module Feeds empty (suggestions for a better name are welcome, see #164).
Remaining tasks
- Write tests for the new behaviour. The tests should check if target values are overwritten when importing blank values for the following mappers:
Date fieldsDONEFile fieldsDONELink fieldsDONENumeric fieldsDONETaxonomy reference fieldsDONEText fieldsDONE- #2415283: Some tests are not executed by testbot should be fixed in order for the testbot to run all the added tests.
A start for this test was made in #173, next patch was in #194.
Original report by jptl
So if the csv for field_summary is blank (hello,,1234), when the node is displayed, it still shows as...
Something: hello
Summery:
Value: 1234
I can fix it by editing the node and, without making any changes, just pressing save but that can be pain when uploading 500+ nodes.
From what I can tell, it only happens for text fields. I was looking through the code and in mappers/field.inc, it has the following but that doesn't seem to work.
if (empty($value)) {
return;
}
Comments
Comment #1
ownage CreditAttribution: ownage commentedI'm having the same exact problem. The labels of empty fields should be hidden, but in this instance Feeds seems to keep them there. I need to find a way to fix this ASAP because I have ~6k nodes to import.
Anyone have some help to lend?
Comment #2
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedPlease try this untested, hacky, naive patch.
Comment #3
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedNo that won't work, because it's neither FALSE nor NULL but ''. empty or == can't be used, because 0 as an integer value might be valid.
Next try attached.
Comment #4
ownage CreditAttribution: ownage commentedFIXED, thank you very much Niklas!
Comment #5
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedGreat, thank you for testing.
I am setting this back the needs review because the patch isn't comitted, yet. To discuss: Anything to improve coding style wise? Check for empty fields at a different point in code?
Comment #6
froboyAfter my first import had many empty fields showing up, I tested this with an CSV import of 320 rows with about 20 fields each including many blanks. Everything seems to have worked as advertised. The code looks OK from my quick inspection, but I'm not much of an authority on that.
Comment #7
franzThis is not for CSV only, but any source where you have mappings but empty input values for fields.
Comment #8
franzAnd it works! (using XPath Parser on a XML)
Comment #9
johnbarclay CreditAttribution: johnbarclay commented#3 didn't break anything for me, but didn't solve my problem.
My issue seems completely different, but has the same symptoms: Somehow all my text fields being imported have a single blank space in them. I used the trim function in feeds_tamper and everything is fine.
Comment #10
dman CreditAttribution: dman commented+1 this is helpful
I think building in a trim() would be most intuative in most cases. Thinking of user expectations rather than as a coder.
Comment #11
smitty CreditAttribution: smitty commentedThis patch works fine, thank you!
But unfortunately it only affects numerical and string fields but not taxonomy fields.
To prevent empty taxonomy fields from showing up you must change function taxonomy_feeds_set_target in line 67 in taxonomy.inc from
if (empty($terms)) {
to
if (empty($terms) || empty($terms[0])) {
This might not be very smart but it does the trick. So my question: has anybody a better solution for this?
Comment #12
franzI think all official target handlers should get similar fixes, if one can post a patch using #3 as a base
Comment #13
nielsonm CreditAttribution: nielsonm commentedHere's a patch, based off of #11 that checks for empty taxonomy terms in the foreach loop.
Comment #14
franzComment #15
nielsonm CreditAttribution: nielsonm commentedPatched the link and field mappers to ignore empty fields.
Comment #16
rtdean93 CreditAttribution: rtdean93 commentedI have manually applied the patch in #3 for the latest dev version - and it worked fine. I have opened a new thread for that release at http://drupal.org/node/1372064#comment-5368824
Comment #17
colanFix in the latest dev branch first, then backport.
Comment #18
xlyz CreditAttribution: xlyz commentedpatch at #3 works here as well on latest dev. please commit.
Comment #19
emackn CreditAttribution: emackn commentedone suggestion for #3, use $entity->language instead of 'und'
Comment #20
Niremizov CreditAttribution: Niremizov commentedmappers/field.inc
Inside function field_feeds_set_target_text (line 71), there is a check if $value is an array:
______________
Inside "_field_feeds_set_target" function (line 96), there is a check if $value is empty... But
I suppose that the check if the $value is empty should be done inside "field_feeds_set_target_text" before "is_array" check, for example:
...?
Comment #21
Shiraz Dindarpatch in #3 worked for me too. It applied cleanly to alpha4.
Comment #22
colan@emackn: That's better, but what's best is using field_get_items().
Comment #23
madeby CreditAttribution: madeby commentedWill these patches help the following problem:
Empty fields (in the file) overwrite fields with value (in Drupal.)
Also, are these patches part of the latest -DEV?
Comment #24
madeby CreditAttribution: madeby commentedApplied both patches and they work which is awesome! We need one for taxonomy as well. @nielsonm is this something you can do? Or do we have someone that understands this module enough to do it?
Thanks!
Comment #25
franzSo we need to combine patches from #3 , #13 and #15. I feel the issue metioned in #19 or #22 do not belong to this issue, as 'und' was being used before it, so better spin-off (if there isn't an issue about it already).
I'm currently testing this, but please test this and review it so we can get it committed soon.
Comment #26
madeby CreditAttribution: madeby commentedThanks for doing this.
Will #25 prevent the following:
The CSV I'm importing contains a empty value but the drupal node that I'm updating contains a manually entered value. Now, it will not overwrite this with a empty value? Correct?
Comment #27
franzCorrect, it's supposed to be that way. You can test this on local backup and report if it works well.
Comment #28
madeby CreditAttribution: madeby commentedI have been testing this for a while and I does not seem to work properly.
If the field has content on the drupal node but nothing in the CSV. The content in the field on the drupal node will be replaced and also be empty.
It is very important that it do not replace content with empty and only overwrite the field with empty if it is empty.
Comment #29
cdmo CreditAttribution: cdmo commentedI've been trying to figure out what's going on with a problem I'm having related to the Feeds XPath Parser module that seems related to this thread. Basically I'm trying to import a value of 0 and any time I try to do that it just quietly ignores the import. I applied this patch and it didn't have any impact unfortunately. Any advice would be appreciated. My original post is here: http://drupal.org/node/1463232
Thanks.
Comment #30
franz@madeby, this was exactly what the patch did, and what you describe is the un-patched behavior. Have you applied the patch properly? What versions are you using?
@cdmo, I don't think this is related, as this thread is discussing the very opposite, unless you applied this patch in the past and it's now causing it. Otherwise, this is another issue.
Comment #31
franzFWIW, questions in #19 and #22 can be discussed #1232836: Use proper language instead of 'und'..
Comment #32
madeby CreditAttribution: madeby commentedI use Alpha 4. Can I only use DEV?
Comment #33
nielsonm CreditAttribution: nielsonm commentedIt might not be a bad idea, as long as you're not on a production site or it doesn't break functionality. You could also reroll the patch for alpha.
Comment #34
bnine CreditAttribution: bnine commentedWhat about file field?
Comment #35
madeby CreditAttribution: madeby commentedDownloaded a fresh copy of feeds 2.0 DEV. (From March 1)
Tried to apply the patch in #25 with git apply -v feeds-empty_values.patch in the feeds folder.
However, it do not work. The patch do no alter any files.
Can you clarify which version it works on?
Comment #36
franzdouble posting.
Comment #37
franz@madeby. Did you download the tarball and used "git apply" to apply the patch?
I just applied the same logic to the file mapper too.
Now, I'm not certain if this is the best way of doing it. As I saw in another issue for Feeds 6.x, using empty() purges "0" as a source value. Sometimes people will want to use these values, even NULL or something, so maybe there should be an option for that in the source config: "Don't skip empty values"/"Feeds usually doesn't update or create data with empty or "0" values. Check this box to override this behavior."
What do you say?
Comment #38
franzAnother desired behavior here is to have empty values reset the target: #1528784: Behavior when importing empty/blank values for numeric field.
I'm renaming the issue to widen the scope. I'll add information to the issue summary.
Comment #39
franzSet #1042384: Support for handling NULL values and #1092652: Possible to allow for blank fields and not overwriting existing data? as duplicates.
Comment #40
madeby CreditAttribution: madeby commented@franz turns out the patch only works on the Alpha 4 version and not DEV.
Comment #41
franzPer-mapper config is closer than I believed: #860748: Config for mappers?
Comment #42
franzMarked #1047894: Behavior when fields/columns are not in import file: do not clear field, but leave field untouched. as duplicate.
Comment #42.0
franzUpdated issue summary.
Comment #43
lunk rat CreditAttribution: lunk rat commentedI have many "0" values in my csv: I use them to set the key of select lists, or of boolean fields, for example. Clearing "0" csv values would be a disaster.
Comment #44
gaele CreditAttribution: gaele commentedWhat's meant here? (#37)
This will check whether the first character of a string value is empty, so "003452" will be considered empty.
Comment #45
franzIt is certainly not right, we need to pull in all the cases and provide the proper treatment. Using empty($string) also fails when string is "0" or "00". Please check summary, I'm sure we can start sketching those conditions and also what to do for each behavior.
Comment #45.0
franzUpdating proposed resolution.
Comment #46
lunk rat CreditAttribution: lunk rat commentedI'm not up to writing patches for this, but I will share my use-case, as it points to the need for a config in the mapper to allow the user to decide how empty fields should be handled:
I will update this as I think of more examples.
Comment #47
franzWe also seem to need a "0" setting like:
"Consider 0 (zero) values as empty".
Let's see a first sketch on those 3 behaviors from summary:
Comment #47.0
franzchanged "expectated" to expected
Comment #48
franzI've updated the summary and re-added related issues as sub-tasks. Just need to finish config for Feeds so we can work on each of them.
Comment #49
madeby CreditAttribution: madeby commentedAny news on this in Alpha 5 and the DEV from May 29th?
Comment #50
liquidcms CreditAttribution: liquidcms commenteda little surprised this is still open but maybe i am missing the finer points of this. i have a CSV with numerous 0's in it and these will not import.
i have 7.x-2.0-alpha5+24-dev (2012-Jul-12)
Comment #51
liquidcms CreditAttribution: liquidcms commentedtried patch from #3 and on test of 10 records i now get 2 failing with this error:
SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'node-92' for key 'PRIMARY'
and these 2 records do not import
without patch all 10 imported; i just don't get any of the 0's imported
Comment #52
liquidcms CreditAttribution: liquidcms commentedbut must be a different issue as all my records have 0 values.. hmm.. odd
Comment #53
liquidcms CreditAttribution: liquidcms commentedi think my issue with not importing 0's in is field_collection_feeds_set_target() (and fixed it)
Comment #54
jjclint CreditAttribution: jjclint commentedI've tried applying the patch on #37 but I get a patch failed error message I'm guessing it's because I'm using feeds latest version, should I try and apply the patch manually? Where does this issue stands currently? It's been open for a while now and it seems to really hinder a lot of the progress made on feeds and it's ability to update nodes from a number of different importers.
Comment #55
millaraj CreditAttribution: millaraj commentedMy specific use case is pulling content in from one xml feed, storing it and then trying to pull extra information in from another feed to enhance the existing data. Unfortunately because I'm not mapping some of the fields it is removing content existing in the already populated fields when importing the second feed.
Comment #56
twistor CreditAttribution: twistor commented#55, That use case is already supported, you just need to set "Update existing" in the processor settings.
Following.
Comment #57
mksweet CreditAttribution: mksweet commentedConfirming that patch #3 worked for me on 7.x-2.0-alpha5.
Comment #58
mErilainen CreditAttribution: mErilainen commentedFor me not, I have 7.x-2.0-alpha5+43-dev. I get the following error:
Investigating further...
EDIT: My bad, it was some feature which doesn't work with the patch for some reason.
Comment #59
cthiebault CreditAttribution: cthiebault commentedHere is patch #3 for release 2.0-alpha7.
Comment #60
cthiebault CreditAttribution: cthiebault commentedHere is patch #15 for release 2.0-alpha7.
Comment #61
akosipax CreditAttribution: akosipax commentedMay I add that empty textfields in the import file should sometimes update the node/any entity's field into an empty value? Like in my case, I have product(entity) with a textfield called 'note'. In the CSV file, the note column can be empty or not. If it's empty, I want the field in the updated product to be empty.
Comment #61.0
akosipax CreditAttribution: akosipax commentedUpdating tasks.
Comment #61.1
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedOption 3: Reset field value: #1528784 proposes empty values should reset the field is same as second option.
Comment #61.2
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedUpdated issue summary.
Comment #61.3
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedUpdated issue summary.
Comment #62
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedAs it in the issue summary:
Let's put options in feed map. By doing this, default behavior can be set for each field, not just at data type level by applying patches.
Give at least 2 options for each field:
Road map:
1) change UI of feeds map. So we can have those options available.
2) Implementation in each data type (Date, Numeric, File, Text etc.)
3) Have other default behavior added for different data types.( Such as wrong date format for date field)
Comment #63
guillaumev CreditAttribution: guillaumev commentedHere is patch #13 for release 2.0-alpha8
Comment #64
mikran CreditAttribution: mikran commentedIssue summary needs update as #860748: Config for mappers? is in.
Comment #65
Uhkis CreditAttribution: Uhkis commentedPatch to #62 for latest git version. Includes UI for allowing empty values and implements the change in Text datatype.
Comment #67
dman CreditAttribution: dman commentedThis failed because of 0 failures? WTF testbot?
Comment #68
dman CreditAttribution: dman commented#65: feeds-empty-behavior-1107522-65.patch queued for re-testing.
Comment #70
a.ross CreditAttribution: a.ross commentedNot because of 0 test failures, but because of 1833 PHP errors.
Comment #71
dman CreditAttribution: dman commentedWell, really just one. Database was unavailable - it seems.
Comment #72
a.ross CreditAttribution: a.ross commentedErr, what? The database wasn't unavailable, as that would result in critical failures when setting up the tests. But the tests ran normally...
Comment #73
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedHave done a manual test.
Applied patch in version 7.x-2.0-alpha7, tested and found it is working as I had expected.
After test, I have checked PHP log, Apache log and Drupal log. No error was found.
So set to need further review.
Comment #74
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commented#65: feeds-empty-behavior-1107522-65.patch queued for re-testing.
Comment #76
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedI do not think failure of the test should block our way. Since the test result is not giving any useful info.
Comment #77
mikran CreditAttribution: mikran commentedUhkis started to work on improved version of the patch, I think PHP errors are solved already but some other improvements are not finished yet.
Comment #78
a.ross CreditAttribution: a.ross commentedThe patch is for 7.x-2.0-alpha7?? That's half a year old! Patch needs to be re-rolled against 7.x-2.x-dev, or at the very least tested against the correct version of Feeds! It's currently testing against 7.x-2.x-dev.
Comment #79
a.ross CreditAttribution: a.ross commentedFirst of all, this sort of new functionality probably needs tests.
Needless whitespace removal.
Idem
Why put this in an IF statement if the function call will always return a string, evaluating to true?
Function documentation must start with a single line.
This causes lots of PHP notices, where the 'allow_empty' element is missing from the array. This needs to either include a call to isset() or you must be able to assure that the element is always set.
Too much whitespace.
This is redundant.
Redundant.
This callback has a fifth argument, the mapper configuration. Use that instead of this awkward loop.
http://drupalcontrib.org/api/drupal/contributions!feeds!feeds.api.php/function/my_module_set_target/7
Comment #80
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commented@a.ross
This patch is not for 7.x-2.0-alpha7, but it is still applicable to 7.x-2.0-alpha7 version. Here is git message I caught when applying the patch.
Comment #81
Uhkis CreditAttribution: Uhkis at Mediamaisteri Oy commentedUpdated patch for #65, includes 2 simple tests.
Comment #82
a.ross CreditAttribution: a.ross commentedThat looks a lot better. But you removed the call to the below function. I suggest to restore that bit.
As for the function itself, I suggest changing this to the following (so the if statement I referred to in the previous patch can remain):
Comment #83
mikran CreditAttribution: mikran commentedIt's still called in form of:
+ 'summary_callback' => 'feeds_ui_mapping_settings_allow_empty_summary',
As the summary callback is now invoked only when it's defined in the target there is no need to check if it's defined.
Comment #84
a.ross CreditAttribution: a.ross commentedWell caught. I see that the settings form is now also a callback, which is more appropriate. This looks good to me, as far as I'm concerned this is RTBC
Comment #85
vaccinemedia CreditAttribution: vaccinemedia commentedHas this been resolved yet? I've just imported a load of products into a commerce installation and have blank terms in my taxonomy vocabularies (see attachment) which are being selected and therefore creating empty dropdown selectors (see other attachment) on the product display and I can't delete them! (see final attachment)
Comment #86
johnvIt will not help your import, but you CAN delete the term by first filling in the title and save, then delete.
(Which is strange behaviour, since you CAN delte driectly from a delete link)
Comment #87
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commented@vaccinemedia Patch at #81 did not fix this. I think we will put it to a separate issue.
We need implementation of this in each data type. Taxonomy is one of them.
refer to #62
Comment #88
kingandy CreditAttribution: kingandy commentedThere's already a separate issue asking about blank Taxonomy fields - #1668186: Taxonomy terms auto-created with empty name. - I think that should be used to develop this feature for the Taxonomy Term data type. Otherwise, that issue will need to be marked as duplicate of whatever issue gets created to build it...
(Changing title to reflect data type specificity)
Comment #89
a.ross CreditAttribution: a.ross commentedThe scope of this issue is a bit unclear. It appears to be a meta issue when you look at the description, but it contains a patch for text fields.
Perhaps a new meta issue can be created and the scope of this issue can just be the text field.
Comment #89.0
a.ross CreditAttribution: a.ross commentedUpdated issue summary.
Comment #90
a.ross CreditAttribution: a.ross commentedI went ahead and took the liberty to create a meta issue, separating it from this field-specific one.
Comment #91
ownage CreditAttribution: ownage commentedI applaud everyone's efforts in this to date, but how are we sitting with this "Text Field" issue right now?
Have any of these "empty field" patches for text fields been committed to the most recent 7.x-2.0-alpha8 (yet alone dev)?
I need to update my ~6k nodes ASAP and have upgraded to the newest alpha8 (in hopes that there is a current version of what originally worked for me in patch #3).
Comment #92
gapa CreditAttribution: gapa commentedI have sucessfully applied patch from #63 to latest dev version (7.x-2.x-dev 2013-Jul-06) and this did solve my problems.
Comment #93
ownage CreditAttribution: ownage commentedSame with me! Thanks for the update.
Comment #94
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedSince patch at #81 is good for the frame work of all the data type, I think we should not change the switch the original issue title. So, I change the title and description of the issue back.
Following issue can be solved afterward.
#1528784: Behavior when importing empty/blank values for numeric field
#1668186: empty content taxonomy cells imported as taxonomy terms with empty names
Comment #94.0
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedUpdated issue summary.
Comment #94.1
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedSince we have patch already, let's keep this issue solved and committed.
Comment #95
ditcheva CreditAttribution: ditcheva commentedHi everyone,
I also really need the ability to update some of my fields (text fields) from containing text to being emptied out through imports. I just tested patch #81 with several imports of 2,000+ records each, and it works beautifully.
It provides an option to adjust each of the fields you're importing in the mapping tab. By default, if the import field is blank, the field in your database is not affected. However, you can specify that an empty field in the import *should* overwrite your fields to be empty. How cool!
Here is a screenshot with the new and improved options:
I'd like to encourage any module maintainers to apply this patch when possible. Since it leaves the default option just as it has always been, it should not disrupt users' current workflow.
Comment #96
Michsk CreditAttribution: Michsk commentedFor the files (https://drupal.org/node/2049341 & https://drupal.org/node/1047894) , we should have a option, 'If empty, keep existing value'.
Comment #97
Michsk CreditAttribution: Michsk commentedHere's a start on the file section, tough i'm not sure it's te way to go. We do a node_load to get the node with the revision, that way we can get the currently used field. The alt en title still get removed, haven't done that yet.
I'd have to start working with patches, so sorry that this is not a patch.
Comment #98
johnv@ditcheva, #95, thanks for the screenshot!
@Lasac, #96, The option in patch #81 should do just that: "leave current value untouched, if new value is empty".
The wording of the checkbox and/or the summary should be clearer. I wouldn't know what means "empty value alowed".
Here is a proposal, using radiobuttons (since I could not make it very clear in one line, and this allows easier expansion to more options) The same wording can be used in the summary:
Comment #99
Michsk CreditAttribution: Michsk commented@johnv: thats what the new file.inc does.
Comment #99.0
Michsk CreditAttribution: Michsk commentedUpdated issue summary.
Comment #100
johnv@lasac, OK. The summary and #94 point to separate issues for other field types, that can use this 'framework', keeping this issue only for textfield as an starting example. For Files you can use #2049341: Images are deleted when node is updated but mapped field is empty to post your patch.
Comment #101
johnvBelow suggestion probablye does not belong to this issue, but regarding to multi-value fields (files, text/numeric listfield), the following options are needed:
Comment #102
johnvSee below code: the callbacks are overwritten. When trying to implement a new option, only the summary/form of the last option is used. This does not happen for the text-field in the patch, but it does happen in the taxonomy-patch from #63.
I see some alternative options: 1) The name of the callback must be more generic, or 2) the form_callback/summary_callback must accept an array of functions, of 3) the new callback must call the original callback.
Comment #103
johnvAlso, in hook_feeds_set_target($source, $entity, $target, $value, $mapping), the new/last parameter $mapping is not passed. I tested this for both text and taxonomy_term.
Comment #104
ditcheva CreditAttribution: ditcheva commentedHi everyone,
I'm attaching a brand new patch, that should apply to the latest 7.x-2.x dev branch, addressing some of the issues raised above.
I completely agree that applying this behavior to additional fields types should be a separate issue. That way we won't allow scope creep of this ticket to keep these changes from being applied.
(As a side note, I've been using feeds (with a .csv file importer) for over a year, not realizing that my fields were not being overwritten by the blank values in my import file. We just recently found out and were shocked. These options will be very welcome and - as I mentioned in my first comment - will not disrupt any current users, since the default value will be what it's always been (blank text field values do NOT overwrite, but retain the current value you have in the database).
Here are my changes, per @johnv's comments
1. The admin UI settings for the new empty-field option are now radio buttons (not a single checkbox) with more descriptive names
2. I tested to make sure the $mapping parameter was passed, and it actually was passed for all my text field tests
I didn't quite get the issue in comment #103 because the naming convention for the new 'empty field' options mirrors the current 'unique id' field setting pretty closely. So I wasn't sure what a better name may be. I left that out, but would be happy to tweak more if needed.
Again - I've tested this several times, and it works just as expected. If others could test and confirm that it works, that would be great!
Comment #105
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedWhat is "[32m+[m[32m" ? It appears in front of each added line?
Comment #106
a.ross CreditAttribution: a.ross commentedlook like spaces? Odd, must be a strange diff format
Comment #107
ditcheva CreditAttribution: ditcheva commentedThat does look strange... must be the diff format, but if you look at the patch all the way through, it appears just on the lines that are the *newly added test file* in the patch. The modifications of existing files further down don't look that way...
In any case, it passed testing, so I'm thinking it's OK.
Do you all have any other feedback or did you get a chance to test?
Comment #108
dman CreditAttribution: dman commentedThose are ANSII escapes for color codes. If you examine it (try deleting characters) there is an unprintable character hidden before each [.
So that looks like a result of a syntax highlighter popping up where it should not.
What I thought was surprising was that the patch applied cleanly and tested OK!
What's actually happening is that changes to both feeds_ui.admin.inc, text.inc appear TWICE in the file. Once colorized, once not.
It seems that the second chunk was used.
Also, based on the patch manifest, it seems that tests/feeds/content_changed.csv was added twice.
Altogether a very fishy file.
Edit: Heh, copy&pasting the escape characters messed with my forum post.
Comment #109
ditcheva CreditAttribution: ditcheva commentedOoops. Well I definitely don't want to be contributing any 'altogether very fishy files'. :-)
I'll redo the patch tomorrow if someone doesn't beat me to it! And hopefully that will encourage others to test this awesome new functionality out too...
Comment #110
ditcheva CreditAttribution: ditcheva commentedOK, here it is! Updated patch that doesn't look strange. I tested this one too, let me know what you think.
Again, just as I said in comment #104, this patch adds the ability to specify what to do with blank text field values in your import
- a blank value in the import file can leave current values as are (default behavior)
- a blank value in the import file can clear out the current values from your db.
Thoughts? Improvements? Agreement that this has been reviewed and works?
Thanks, everyone!
Comment #112
ditcheva CreditAttribution: ditcheva commentedOoops, I see the issue. Let's try this again.
Comment #113
a.ross CreditAttribution: a.ross commentedI haven't thoroughly reviewed this, but I did notice that the patch contains tabs.
Comment #114
ditcheva CreditAttribution: ditcheva commentedCool. I can definitely re-roll the patch, but would love to see if there is any other feedback on the functionality/code before I do that.
Comment #115
mbatterton CreditAttribution: mbatterton commentedThis patch is great although it only work for text fields. Could it be extended to work for other field types, e.g Image, Integer, Decimal, Date, Float and Price?
Comment #116
kingandy CreditAttribution: kingandy commentedDate, Files, Numeric fields and Taxonomy are already being developed in different tasks - see the "Remaining tasks" section of the post header. Looks like Date is completed, the others are in various states of needing work/review.
Comment #117
a.ross CreditAttribution: a.ross commentedWhy was the link to #2027583: [meta] Behavior when importing blank values removed? This issue is suffering from scope creep IMO.
Comment #118
ditcheva CreditAttribution: ditcheva commentedIt's cool - we're not suffering from scope creep too badly as long as we keep the other field types separate! Glad you guys are on the ball about that! :-)
So it looks like the patch worked for @mbatterton as well. Any other testers who'd be willing to try it out and mark as tested by the community or give feedback? I'd love to keep the momentum going here and to make a commit! We've been using feeds for years without realizing our blank fields weren't getting written, so this would be a huge improvement.
Again the patch should provide you options in the admin interface to change the overwriting behavior for blank fields for each mapped text field (and of course - it sticks with whatever option you've chosen during the import):
Comment #119
ArtActivator.com CreditAttribution: ArtActivator.com commentedHello. I have a bug with duplicate terms on importing (https://drupal.org/node/2033519)
Lasac said, that I need to wait until framework issue is commited.
Will some patch from this issue help me on resolving this?
Comment #120
littledynamo CreditAttribution: littledynamo commentedJust tested this patch and can confirm that it cleared the field!
Nice work!
Comment #121
ditcheva CreditAttribution: ditcheva commentedGreat. Three separate tests. Let's mark RTBC, and see if this first part can be committed!
Comment #122
manuelBS CreditAttribution: manuelBS commentedPatch works perfect for me, thanks!
Comment #123
mparker17Fixed whitespace...
Comment #124
blueblot CreditAttribution: blueblot commentedpatch works for me too, thx
blueblot
Comment #125
ditcheva CreditAttribution: ditcheva commentedYay! Thanks for all the patch testing, you folks are great. And thanks for clearing up the spaces issue @mparker17, I meant to do that and forgot!
Marking RTBC again.
Comment #126
mparker17Thanks @ditcheva! :)
Comment #127
ownage CreditAttribution: ownage commentedJust a question--
I currently have the #59 patch installed (based on patch #3 for release 2.0-alpha7) and there seems to be one little issue. I was wondering if this issue would be present in the newest release of this patch.
The feeds behavior of the issue is, when "updating" a text field that used to have content but is now null, feeds does not rewrite the field as null-- the content stays there!
Please let me know if that issue would be present in the newest release of this patch.
Comment #128
franzI think this kind of behavior is now configurable, but you can always set your importer to replace nodes, if you don't care at all about losing modifications...
Comment #129
twistor CreditAttribution: twistor commented"Empty new value leaves current value untouched." -- this is not happening in this patch. Feeds resets each entity field long before any mapping is called. And frankly, that's not a feature I'm willing to support.
I agree that some of the targets need more specific value checking, empty() in numeric doesn't work for instance. But, these are all trivial issues. What this patch is trying to accomplish is just strange. What should happen is that each field target ignores emptiness. They just validate the field value, or set it to NULL. Then, in entityValidate(), or someplace before save(), we should iterate over the fields calling the field's is_empty() callback and just remove empty fields.
Comment #130
ditcheva CreditAttribution: ditcheva commentedHey @twistor, I want to make sure I understand your comment properly to know what to address. I think this issue is very old, very active and *sorely* needed, so I want to make sure it isn't left in 'Needs work' for a long time just because of lack of clarity. :-)
You mention that 'Empty new value leaves current value untouched" is not happening in this patch. Could you expand on that? Are you saying that the patch doesn't provide the functionality for one of its options? (It only solves the issue for text fields at the moment - better to approve that first before solving terms and other field types etc).
Furthermore, when you explain how to solve this issue ("...in entityValidate(), or someplace before save(), we should iterate over the fields calling the field's is_empty() callback and just remove empty fields.") are you trying to say that, in your opinion, any future work on this issue shouldn't give folks the option for how to handle empty fields? The reason I ask is that there seems to have been a discussion throughout this issue, that was more philosophical than technical about how to treat empty fields. (This was before I found this issue myself in trying to deal with the problem of empty values being ignored, so I never chimed in then). Some seemed to think empty fields in the import file should be ignored, and others (I'm in this camp, as are the majority who have found this issue through the bug queue because this is the part that is missing) believe an empty value in the import file (or equivalent) should empty out the corresponding drupal field. Currently there is absolutely no way to empty out a field you've ever set before because all empties are ignored. :-( In any case, it was decided that both options should be provided because there were voices on both sides of the debate. With this comment, are you saying that you do not want to support the availability of two options? That how empty values are treated should not be an option in the admin UI, and instead, empty values should *always* empty out the corresponding drupal fields?
If that's the case, I understand, I just want to make sure it is clear so we can move this issue forward and actually fix this. I'm afraid that without clarity on what you'd be willing to commit, we may continue working in a futile direction!
Thanks again for getting back so quickly, and if you can clarify the above two questions too, we can keep going. :-)
Comment #131
twistor CreditAttribution: twistor commentedMy apologies for letting this run on for so long.
"Empty new value leaves current value untouched":
If you look in FeedsProcessor::map() at the top. You'll see that each field is emptied before the mapping is even started. So, yes, currently there is no way to leave a field untouched that has a mapping. Any fields that don't have a mapping are left alone, as long as you select "Update existing" rather than replace.
are you trying to say that, in your opinion, any future work on this issue shouldn't give folks the option for how to handle empty fields?
Yes. Empty fields should always be empty. I can understand the other side. But, it is a minority. Feeds can't be all things to all people. This could also be solved as an add-on module.
To elaborate on what I was saying before, targets shouldn't be so concerned about emptiness. They should always set a value. Either the correct, validated value, or an empty string. Then in another step, after mapping has occurred, we should loop through the fields and remove the empty values from them. The reason for this is that some targets handle mapping multiple columns of a field. Think of the link mapper. It handles the title, and url fields. In order for this to work properly, both fields need to have a value, even if it's empty, in order to keep the values aligned.
Comment #132
a.ross CreditAttribution: a.ross commentedThat's unacceptable as I'm sure many people rely on the current functionality. There must be an option to preserve the old way of handling empty values. By the way, is there currently a difference between how missing fields and empty fields are handled?
Sorry missed the second comment. A solution based on feeds tamper would be fine I guess.
Comment #133
twistor CreditAttribution: twistor commented@a.ross Which part is unacceptable? You'll have to be more specific. I am only suggesting an improvement to the status quo.
Comment #134
a.ross CreditAttribution: a.ross commentedThe part where the old behavior, where people already rely on, is done away with. The old behavior being that an empty import field does not clear the field in Drupal it's mapped to. Or at least, I experienced that with numeric fields, not sure if it was the same for text fields.
Edit: right, sorry. For text fields the field is emptied, but not nullified. I find it unlikely that people rely on that behavior, but maybe people still do. :/
Comment #135
twistor CreditAttribution: twistor commenteda.ross, if you experienced that, then that would be a bug. I'm assuming that by people, you mean yourself.
Comment #136
a.ross CreditAttribution: a.ross commentedI'm sorry, I was kind of confused. I came here because I had an issue with an empty numeric field; the field would be left untouched. This issue (the original report) is different and in fact incompatible with Drupal's validation for text fields, so this is actually a rather straightforward bug. Adding an option for how to handle an empty text field should IMO be a separate issue.
Comment #137
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedIt is a little bit confusing. But I believe we are still on the right track.
The patch provide 2 options in dealing with empty field.
1) Empty new value leaves current value untouched.
2) Empty new value clears current value.
What if we change the words like following? Will it be more easy to understand.
1) Do not deal with empty value (or: Not enforcing empty value)
2) Enforce empty value
This is on the Feeds mapping, each data type still need to implement the behavior when enforce empty value. (eg. text can be "", number can be 0 etc.) This is what #119 is waiting for.
Comment #138
a.ross CreditAttribution: a.ross commentedYes, that's what we've been talking about in this issue, but it isn't directly related to the original issue. I noted before that this issue is suffering from scope creep :) maybe I'm the only one who's confused but still better to keep things separate.
Comment #139
johnvI think what twistor is trying to say is this: "Since all values are cleared from the target before we enter the mapping, the current value will ALWAYS be empty."
I (used to) have a use case for this patch, and have implemented it, but now I am puzzled (too).
Comment #140
ditcheva CreditAttribution: ditcheva commented@johnv, I'm confused because I've tested the patch multiple times myself - with imports of between 6 nodes all the way to 2,700 nodes at a time. It always works - both both admin options of how to treat empty values. We have several other manual tests all saying it works too. The patch does what it says for text fields. So I'm confused - until now nobody had mentioned a failed test. Could it be that you or @twistor are testing it for other types of fields (non-text fields)? If not, could you expand on which part doesn't work? I feel like I'm misunderstanding something or don't know what is meant by 'current value' - the current drupal field or the field being imported that is trying to replace it?
Thank you very much again!
As I said earlier - I am not sure what the use case for ignoring empty fields would be - that's the current behavior folks are saying we should preserve (could anybody explain a good use case for this?). In my view, the most important part is to be able to clear out the values of drupal fields with empty import fields when you need to.
Comment #141
dman CreditAttribution: dman commented@ditcheva - since you ask ... a simple use-case is for merging incomplete data into the CMS, where it may be corrected or curated, and then updated later. I've had a number of cases like this.
An almost-real-world example (the truth is worse) :
We pull staffing information from a CRM sort of source. Names, contact info etc that then gets published in a staff directory.
Information we get from the CRM is patchy and incomplete, but important bits (like are they still employed) are the most up-to-date.
Other bits, like the freetext 'biography' blurb or the 'headshot' are irregular and mostly missing, but better than nothing most of the time.
NOW:
On the website, the content editor can go and edit the profiles, fix up formatting in the bios or upload a better headshot suitable for the team pages.
THEN:
Updates from the CRM get pulled in periodically. New staff members appear, phone numbers change, people leave, our CMS data gets updated.
*except for the stuff we have started maintaining locally*
That is the case for
We've done similar with pure node content - Pulling an RSS from an existing site a client was adding content to while building a full new site in the background. Sorta continuous-integration scenario. We get their data, we *may* curate it a little (tag to the new IA) whilst at the same time importing their live content and tag edits as they happen. On updates, the merge rules included:
content=overwrite,
tags=merge,
menu=don't touch,
attached images=add if new, don't touch if existing (we sometimes added new or better images on the new site where they did not exist on the old)
... etc.
Thus, the configurable options that let me define just a few of these rules would help out a lot.
Comment #142
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedregarding #141, I think this patch is handling the empty/blank values. It is done before it starts merging. Only empty text is implemented in this patch. Others data types are waiting for the commit of this patch. They are listed at top.
This patch is to find out the whether the empty/blank values should be used or now. If user decide to use the empty/blankvalue, then go to next stage which is merging with existing data.
Comment #143
ditcheva CreditAttribution: ditcheva commentedThank you all very much for explaining the use case for keeping an option to ignore empty values... That's probably why the decision was originally made to include both options...
I think to move forward, and either get the current patch committed or improved to be committed, it would be useful to get the other part of my question from comment #140 answered - the part about which part of it isn't currently working. Anyone have any insight into that?
Comment #144
johnv@ditcheva, here is some update from some others issues (which are all mentioned in the summary):
#1668186: Taxonomy terms auto-created with empty name. has the same problem as OP (adding an new empty value instead of discarding it) + a simple solution. In that respect, this issue is way out of control ! Instead of a simple fix, a whole new system is created. (But i'm glad it's there)
Such a simple change would be sufficient for OP.
The other issues are a result of the fact that Feeds clears all values that are in the Definition of the Importer.
#2049341: Images are deleted when node is updated but mapped field is empty has an implementation to add new values to a multivalue file field. I adapted that patch for multivalue taxonomy terms.
It also contains a hack to preserve the old values, without reloading the Entity.
#1047894: Behavior when fields/columns are not in import file: do not clear field, but leave field untouched. has a change that only clears all values that are in the actual import file (that may have less columns as the definition.)
IMO Feeds does not need to clear the values, and may leave that up to the mapper, but I understand that such a change will break all custom mappers :-(
Comment #145
MegaChriz CreditAttribution: MegaChriz commentedSo, if I understand the issue well, with the patch applied the following happens:
Target field is emptied.
Target field is emptied.
And the exepected behaviour should be:
Leave target field untouched.
Target field is emptied.
Following twistor's comments in #129 and #131 I translate that as:
"Whether or not the target field should be left untouched, shouldn't be configurable. The target field should always be emptied when the value in the source is empty and always be left untouched when the field in the source does not exists."
If this is the way to go, then the next patch should do the following:
When the story above is confirmed, I'm willing to work on a new patch with tests.
Comment #146
Summit CreditAttribution: Summit commentedHi @MegaChriz, spot on! That should, in my opinion, be the wanted behavior! Not there untouched, There is emptied.
Greetings, Martijn
Comment #147
twistor CreditAttribution: twistor commented#145,
This is not going to happen.
I understand the use case, but it's minimal, and it will break current assumptions. The assumption being that the state of the feed is exactly reflected in the state of the entity. If it doesn't exist in the feed, it doesn't exist in the entity. Whether or not the feed has a value, vs. contains an empty value is too delicate a distinction to make, and it's completely left up to the implementation of the parser.
Comment #148
twistor CreditAttribution: twistor commentedIf you can make the distinction of non-existent vs. empty then you probably have control of the feed and can just put the value in.
Comment #149
MegaChriz CreditAttribution: MegaChriz commentedOkay, making the distinction of non-existent vs. empty is not possible for a generic solution, because it completely depends on the implementation of the parser.
@twistor
The status of this issue is set to "needs work". If the proposal in #145 is not achievable, then what's the next step for fixing this issue?
Comment #150
Michsk CreditAttribution: Michsk commentedPersonally, would like to suggest to do something similar as in: https://drupal.org/node/2049341.
There i send a copy of the current field to the mapper so if needed it can fallback to the current value. That way it's not a problem that feeds unsets the field in the entity and the mapper does not have to do any kind of magic to get the current field value since it's already been passed to the mapper.
Comment #151
loker CreditAttribution: loker commentedThe patch in #63 worked like a charm for me...
Comment #152
ditcheva CreditAttribution: ditcheva commentedI'd just like to poll again what the status of this issue is. It's been over 2 years that the bug was first reported.
If we use this feeds module to import nodes with text fields and later want to clear out any of those text fields in future imports, we can't.
Some of my web users have resorted to putting in the words "none" to remove text, because there is absolutely no way, with the current module, to clear out a text value.
It just looks so silly and unprofessional on our site to have node fields that say 'none' simply because we cannot import an empty value!
The patch I had in #125 has been tested numerous times and works for me. I feel like I'm left with two options: use my own patch and have a feeds module that has been hacked/does not match the official module here (so I can't do updates on it) or keep putting the words 'none' in my text fields that should be empty, because otherwise, their value that is no longer appropriate remains. Both of these options seem ridiculous, but this issue is also not moving forward in any meaningful way.
Does anyone else have any ideas? Is there a response to @MegaChriz's question in #149? Or does someone have any other work-around we could use in the meantime?
Comment #153
twistor CreditAttribution: twistor commented@ditcheva, The patch in #123 should not solve your problem at all, and if it does, it's not the point of the patch. As far as I can tell, you should not be seeing the behavior that you are. You always have the option of creating a custom mapper, rather than patching Feeds.
That said, I think all of this is way too complicated. Field mappers shouldn't have to worry about what kinds of values are empty. Fields already know how to check this. On that note, people might be interested in
#2093651: Simplify target callbacks. which integrates a fields empty callback and remove all of the custom checking.
Comment #154
ditcheva CreditAttribution: ditcheva commentedHey @twistor, I agree that I should not be seeing the behavior, but I certainly am, and I thought that's the entire purpose of this bug report. Perhaps I'm confused and the folks here are seeing another type of problem...
In any case, I just went and re-checked on a simple drupal install and Feeds 7.x-2.0-alpha8 (latest) and tested everything again. The bug is there.
See if you can reproduce it: 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, and indeed, 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;
---------------
So - that's the bug I'm experiencing, and it's certainly a bug. Those fields should no longer be there. I can reproduce this behavior every time, and on multiples of my sites, content types and importers. That behavior is definitely fixed with the #123 patch, though I understand it may not implement things properly, in which case I would definitely not push for it. So - would it be better for me to open up a brand new bug issue if I'm seeing this? I'm getting the sense that I misunderstood what this thread is about..
Comment #155
twistor CreditAttribution: twistor commented@ditcheva, Now that's a bug report! I haven't tested it yet, but yes please open a new issue.
Comment #156
MegaChriz CreditAttribution: MegaChriz commentedIf the report in #154 should be a new issue, then what is this issue about? I may be wrong, but I thought this issue was about fixing the behaviour that target fields were not overwritten when the source for that field was blank (or maybe this issue wants to go further then that?). And doesn't the patch in #2093651: Simplify target callbacks. proposes a fix for this issue? (O.K. maybe you could call it a side effect that the patch in #2093651: Simplify target callbacks. possibly fixes this issue.)
Comment #157
twistor CreditAttribution: twistor commented@ditcheva, would you please test the patch in #2093651: Simplify target callbacks.?
@MegaChriz, I feel for you. I'm not entirely sure what the goal of this issue is. I'm trying to break it up into bugs that exist and features that people want. It seems to me that this issue spans too many things. Re: "fixing the behaviour that target fields were not overwritten when the source for that field was blank". #2093651: Simplify target callbacks. should fix that for most/all cases. It's basically me taking a different approach to solving it generically.
It basically comes down to two different expectations about how empty fields should be handled that are getting confused into one.
Comment #158
hcethatsme CreditAttribution: hcethatsme commentedI'm having the same issue as ditcheva with XML imports, and the Simplify target callbacks patch does fix it--just tested. Thanks, @twistor!
Comment #159
ditcheva CreditAttribution: ditcheva commentedYaaaaaaaay! So glad you pointed us in that direction @twistor.
I also just tested the patch at #2093651: Simplify target callbacks and it works! It totally solves the issue I described above. I'll add my comments there too and follow that issue to see when it will be committed (instead of this one). I think many of the folks here may simply need that fix.
Comment #160
MegaChriz CreditAttribution: MegaChriz commented@twistor, #157
I think it's getting more clear to me what is happening in this issue (too bad I stepped into it quite late). Let me summarize:
The problem is that for some target fields the field doesn't get overwritten when that value for that field is blank in the source. There are multiple solutions proposed for that problem. One of them is simply and straight forward, the others go further then just fixing the problem by also introducing new features or API changes. None of the solutions proposed seem to be perfect, they all have cons.
Proposed solutions (so far)
Is by just removing the
empty()
check at the beginning of the target implementation. Similar to the patch in #63.Pros
Cons
This approach was introduced in #37 and that seemed the way to go for a long time. If I'm not wrong, this solution was proposed to fix the negative side effect of solution #1, namely that some people currently using Feeds in production expect that empty values are always skipped. The latest patch following this approach was posted in #123.
The patch was marked as "needs work" by twistor in #129, because a part of it didn't exactly do what it said.
Pros
Cons
Proposed by me in #145. When the field doesn't exists in the source, leave target untouched, otherwise empty the target. This solution is not achievable, because the distinction of non-existent vs. empty can not be made for a generic solution.
Pros
Cons
Proposed in #2093651: Simplify target callbacks.. This solution moves the decision whether something is empty or not one step earlier in the process, out of the target implementations. The implementations of the targets will get simpler and more standarised and will produce less overhead.
Pros
Cons
It seems to me that solution #4 is nicest one, though I have yet to check out how that exactly works and if the negative side effect that solution #1 has, is addressed (though I would also be fine if the change in behavior is clearly documented). It seem like it does by providing an additional mapping callback called 'post_process'.
If we would go for that solution, we should not forget to document the API change. I suggest to start using change records for this (as Drupal core does). (Side note: we then should also add a change record for the change in #1711648: Abstract bundle handling., as documenting that change was forgotten as reported in #2021087: Add a big notice about "abstract bundle handling" in release notes of 7.x-2.0-alpha8).
Comment #161
twistor CreditAttribution: twistor commentedHe, well I feel like an idiot. Figured out the real problem.
Comment #162
twistor CreditAttribution: twistor commented@MegaChriz, thanks for the synopsis.
The patch in #161 *should* actually fix the issue. Unsetting the field doesn't work as field API purposefully ignores fields if they don't exist, rather than deleting them.
I can't believe I haven't seen this before.
Making every field configurable as to how it handles empty values is outside the scope of Feeds. I do have an idea about how we could do it in a contributed module. #2093651: Simplify target callbacks. Should make it a bit easier as well.
Comment #163
twistor CreditAttribution: twistor commentedWe should definitely add a test-case for this.
Comment #164
twistor CreditAttribution: twistor commentedSandbox for configurable emptiness handling.
https://drupal.org/sandbox/twistor/2094551
Anyone have any ideas for a good name?
Comment #165
Michsk CreditAttribution: Michsk commentedtwistor: i think you actually nailed it with #161, this also imports images correctly, i had a ton of duplicate images, and now it does not seem to duplicate them any more.
Comment #166
hcethatsme CreditAttribution: hcethatsme commented@twistor, thanks for the patch in #161 - tested and works on 7.x-2.0-alpha7! Any plans on when this might get released? We use Git submodules so want to avoid local patches if at all possible. Any other type of testing that could be done to help?
Thank you again!
Comment #167
Summit CreditAttribution: Summit commentedHi, This issue is a "mother" issue for other issues. Patch #161 works great!
Is it ok to set it to RTBC?
Greetings, Martijn
Comment #167.0
Summit CreditAttribution: Summit commentedAdded new file mapper in list : #2049341
Comment #168
killua99 CreditAttribution: killua99 commentedPatch #161 is working.
When some field (source) come empty it fill empty! yey!
Comment #169
killua99 CreditAttribution: killua99 commentedComment #170
sin CreditAttribution: sin commented#161 fixed issue with incorrect attached images removal during node updates if source field is empty. 7.x-2.0-alpha8. Thanks guys!
Comment #171
twistor CreditAttribution: twistor commentedI've committed the patch from #161.
http://drupalcode.org/project/feeds.git/commit/95cc744
What else is left to do on this?
Comment #172
MegaChriz CreditAttribution: MegaChriz commentedWriting a test case as noted in #163.
I started with writing a test for this issue a few months ago, but due to other things on my mind I forgot to finish it. Attached is what I currently got.
Comment #173
MegaChriz CreditAttribution: MegaChriz commentedI forgot to do a
git add
before creating the patch, so the patch in #172 misses two files.Attached a new patch.
Comment #176
Mykolas CreditAttribution: Mykolas commentedFeeds tamper "Filter empty values" plugin solves this problem.
Comment #177
MegaChriz CreditAttribution: MegaChriz commented173: feeds.test-case-fix-empty-fields-1107522-173.patch queued for re-testing.
The test failed because a zero was treated as empty for a text field. Now that #2093651: Simplify target callbacks. is committed, let's see if the test still fails. The one single test method should be split up in two, by the way.
Comment #178
chrowe CreditAttribution: chrowe commented@Mykolas
I don't see a "Filter empty values" plugin
Do you mean "Required field"? I assume this should do the same thing. (correction: this prevents the entire record from being imported, not just the one field.)
Comment #179
chrowe CreditAttribution: chrowe commentedThe description says "Default: Do not touch. (This is current behavior.)" but I don't think that is the case any more. I think it used to be but has been changed to "Import anyway".
Comment #180
coredumperror CreditAttribution: coredumperror commentedChrowe is right. The default behavior of how Feeds treats empty inputs for fields which did not used to be empty is now different. It was years ago, but I think a toggle option for this behavior was discussed earlier in this issue. I think this would be a good idea to implement that, and to set the default to the original behavior. Without such a change, people who upgrade Feeds will suddenly start getting wildly different import behavior that deletes their fields.
Comment #181
gcbI also need to have imports leave values alone on import. However, in FeedProcessor.inc, the map() function forces nulling without any way to prevent it other than overriding the entire function (which seems both silly and a maintenance nightmare). I'm attaching a patch that adds a simple check for a flag on the mapping configuration called "prevent_null_overwrite". This allows my FeedsProcessor implementation to set the flag when appropriate and handle the writing or nulling itself in setTargetElement().
It seems to me it would be a sensible default behavior to allow setTargetElement to handle this, but I haven't dug too deeply into the feeds module so I may be missing something. Regardless, if this solution might work for you, feel free to have a look at how I've implemented it over at project/redhen_feeds in the development branch.
Comment #182
gcbThe patch in #181 is against Dev. Here is a version against alpha-8
Comment #184
nickmaine CreditAttribution: nickmaine commentedhttps://drupal.org/node/1107522#comment-5955580
Multiple value fields. If I import a csv with source column 'image url' and map it to a multi-value Drupal field, then import a different csv that has a new value for 'image url', the default behavior is to overwrite the original value. But if it is a multi-value field, I might want feeds to just 'add' a second value to the multi-value Drupal field.
How is issue #4 being handled ?
I have being trying to solve this issue myself.
Comment #185
dman CreditAttribution: dman commented@nickmaine
I think the replace/append/merge question needs a different issue.
Basically, the default behavior will always be 'replace', as the data source is expected to be definitive at import time.
The best we can ask for here is "leave it alone if it's not defined".
Asking for new 'merge/append' behavior for *multiples* = a follow up feature request.
(in my experience with this sort of task, it's a lot harder to *delete* old records than to just add new ones. That's the real challenge.)
Comment #186
MegaChriz CreditAttribution: MegaChriz commentedI've updated the issue summary and pointed out that:
Comment #187
MegaChriz CreditAttribution: MegaChriz commentedComment #188
Exploratus CreditAttribution: Exploratus commentedSo should we use #161?
Comment #189
MegaChriz CreditAttribution: MegaChriz commented@Exploratus
The patch in #161 is committed to dev, so if you are using the latest dev there is no need to apply that patch. If you are on Feeds 7.x-2.0-alpha8 you could try to apply the patch, though I would recommend to use the latest dev instead as there was also a related fix committed (in #2093651: Simplify target callbacks.) and I'm not sure how Feeds would operate with only the fix from #161 applied on 7.x-2.0-alpha8.
Comment #190
Exploratus CreditAttribution: Exploratus commentedThanks. works great
Comment #192
christo snyman CreditAttribution: christo snyman commentedOk, so im a bit lost here. I read through all the comments but not sure which path I should follow. When I import blank values I do not want them to import a zero but want them to erase the value. In my scenario I have a price & old price and want new products without an old_price value to have no value and existing products must erase whatever value is in there. Please help out as I got super confused going through the comments (I have very little dev skills)
Thanks people
Comment #193
Nux CreditAttribution: Nux commented@christo snyman seems like you should use dev version. Current alpha version (7.x-2.0-alpha8) doesn't work well with empty values (the value is added as empty to revisions, but not really changed).
Comment #194
MegaChriz CreditAttribution: MegaChriz commentedFinally picking this up again. Note that the fix for this issue was already committed (see #171), it only could use tests.
The attached patch is a follow-up on the patch in #173 and adds three test methods that test the following:
0
is not considered empty.The following test methods are added:
For field types text, number_integer, number_decimal, number_float.
For field types date, datestamp, datetime. Other than in the other tests,
0
is considered an empty value.For field type taxonomy_term_reference.
Also the following assert method is added:
To assert that a field with a certain value does not exists. This method is to used to assert that after a field should be emptied it no longer contains it's originally value. Maybe I remove this method in a future patch and explicitly check the field for an empty value instead.
This is still in work progress. Things left to do:
@gcb, #181, I'm not sure if I understand what your patch does, but I think your proposal could better be handled in a follow-up.
Comment #195
MegaChriz CreditAttribution: MegaChriz commentedAnd here is the test for the link field. Note that the tests will currently fail, because apparently the fix from this issue (posted in #162) did not fix the whole problem in case of the link field. Though the link field gets successfully emptied, the field labels remain displayed on the node display:
This may be a problem in the link module (because it does not happen for text, number, date and taxonomy fields), so it would be better if this is further investigated first, but I think Feeds should try to fix it anyway if it is easy to do.
I'll check the link field further later. Next step for me will be writing a test for emptying file fields. I hope to have this ready next Tuesday (seven days from now).
Comment #196
MegaChriz CreditAttribution: MegaChriz commentedThat's interesting. The tests for link field aren't executed at all. And that is because the test case isn't listed in the info file. Let's add tests/feeds_mapper_link.test to info file.
Tests should fail because of the issue noted in #195.
Comment #197
MegaChriz CreditAttribution: MegaChriz commentedAnd here is the test for the file field. I also included a fix for the issue with the link field reported in #195: in the target callback for the link field a call to
_field_filter_items()
is made, which will calllink_field_is_empty()
and as such filter out empty link field values. I tried first if a fix for #2379631: field_attach_validate() must be called before programmatic entity saves would also solve the problem as_field_filter_items()
is also called byfield_default_validate()
, but this wasn't the case.Note that not all tests are evaluated by the testbot, herefore I opened #2415283: Some tests are not executed by testbot. Thus this issue basically depends on that one now.
Comment #199
MegaChriz CreditAttribution: MegaChriz commentedLet's see if this fixes the failed assertion from the patch in #197.
Note that #2415283: Some tests are not executed by testbot should first be fixed in order for the testbot to run all the tests that are included with this patch.
Comment #200
Leeteq CreditAttribution: Leeteq commented@Twistor: Name suggestion for #164
Comment #201
OnkelTem CreditAttribution: OnkelTem commentedFollow-up on a feature to skip empty (or empty-after-rewrite) sources: #2426613: Add "NOT NULL" option to targets
Comment #203
MegaChriz CreditAttribution: MegaChriz commented#2415283: Some tests are not executed by testbot is in, let's see if the tests from #199 all pass.
Comment #204
sonicthoughts CreditAttribution: sonicthoughts as a volunteer commentedThis blockee looks pretty solid esp for an alpha ... can we now release alpha 9 or are you also waiting for child issues?
Comment #205
twistor CreditAttribution: twistor commentedIt had to be re-rolled because of the extend mapping API issue.
Rather than special-case the link field, what about doing something like this? I think it will help with #2379631: field_attach_validate() must be called before programmatic entity saves as well.
That's a lot of tests!
Comment #207
twistor CreditAttribution: twistor commentedoops.
Comment #208
MegaChriz CreditAttribution: MegaChriz commented_field_filter_items() and cardinality correction applied to all field instances?
The code in mappers/field.inc looks like a decent solution for all field targets. One question though: it loops through all field instances of an entity and applies the
_field_filter_items()
correction and the cardinality correction to all of them. Shouldn't Feeds only apply these corrections to the fields that are targeted and leave the rest alone? For example: if you have an entity with two fields and you only map to the first field, the corrections in mappers/field.inc are also applied to the second field, which you didn't map to.I've looked through the tests once more and I made some tiny corrections to it.
Comment #209
twistor CreditAttribution: twistor commentedThat's probably true about not messing with fields Feeds isn't managing.
Comment #212
twistor CreditAttribution: twistor commentedComment #214
twistor CreditAttribution: twistor commentedThanks all!
Comment #216
Morbus IffAre there any new developments, or new issues related to this? I have been unable to get it to work. Currently using Feeds 7.x-2.x-beta1, with Feeds XML XPath Parser 1.1 (feeds_xpathparser), and I am unable to get normal target text fields to be wiped out when the value is empty.
1. Process XML file with [Credentials]PhD[/Credentials]
2. Watch the value get assigned to an existing node.
3. Process XML file with [Credentials][/Credentials].
4. See the existing node get updated, but with no change to Credentials.
5. Process XML file with [Credentials]ABC[/Credentials].
6. See the existing node get updated, with a change to Credentials.
Comment #217
apmsooner CreditAttribution: apmsooner commentedI also have not been able to get it to work with XML file using xpath parser when value is empty for a numeric target. The schema for field is set to numeric and not null => FALSE but anything that has a blank value gets skipped and reports error in feeds log like example:
SQLSTATE[HY000]: General error: 1366 Incorrect decimal value: '' for column 'n_factor' at row 1
Other values get imported just fine.
Here is detailed output from the log. I've also tried as a .csv importer and same result so numeric fields remain a problem for me....
Comment #218
Leeteq CreditAttribution: Leeteq commentedSo what is the real status here: is the issue fixed or not, in the latest -dev?
Comment #219
MegaChriz CreditAttribution: MegaChriz commentedThis issue is definitely fixed. For every field target that Feeds provides there is an automated test that tests if values are wiped out if an empty or if no value is provided and these tests are still passing. It is possible however that wiping out values does not work for mapping targets provided by other modules. Some modules may still check if the provided value is empty and then decide to do nothing. In such cases, Feeds can nothing do about that, you'll then have to create an issue for the module in question.
The issue that apmsooner had has been resolved in #2572861: Empty Numeric Fields cause SQL errors and was caused by a misunderstanding of the Feeds API.
The issue that Morbus has, sounds like an issue with the name field mapper, which Feeds doesn't provide. This issue should be handled in the Name Field issue queue.
The fix for this issue is included in the 7.x-2.0-beta1 version.
Comment #220
Leeteq CreditAttribution: Leeteq commented@MegaChris; thanks for the thorough clarification.
Comment #221
jomarocas CreditAttribution: jomarocas as a volunteer commentedi'm no sure this working, i have a field date/iso.
and i have default value in today day, and the field is required, with a custom format d/m/Y, this is a field collection item and the importer is configured for a node have infinite fields, the date have a default value and working when add a new field collection item, but when importer dont working, i dont declare in the feed importer the field but i think always take the default value, why? thanks
Comment #222
troybthompson CreditAttribution: troybthompson commentedWhat about boolean fields? When I'm importing nulls it's interpreting them as 0's even when I have the field not required. I'm using Feeds (link is external) 7.x-2.0-beta1+14-dev (2015-Oct-11)
Comment #223
gcbI still need the ability to prevent nulling, and have upgraded to 2.0-beta1. So, here's a patch for that version to enable a config on your feed processor called "prevent_null_overwrite". It's pretty handy. I think it'd be cool to add, but I'll leave this ticket in "fixed" mode and just leave this here for whoever needs it...
Comment #224
mikemccaffreyUpdating @gcb's patch to work against the latest dev branch and beta 2, so you can enable a config on your feed processor called "prevent_null_overwrite".
Comment #225
demonde CreditAttribution: demonde commentedThanks for the patch in #223 and #224, but how do I apply such a config on my feed processor?
Comment #226
vbard CreditAttribution: vbard commentedModule Feeds empty doesn't work with current Feeds version at the moment.
Comment #227
vbard CreditAttribution: vbard commented@mikemccaffrey, I can't find any config called 'prevent_null_overwrite', where should I seek it?
Comment #228
gcbYou can add it to the ConfigForm() function in your FeedsProcessor class. Example: http://cgit.drupalcode.org/redhen_feeds/tree/lib/RedhenFeedsContactProce...
Comment #229
vbard CreditAttribution: vbard commentedThank you @gsb, now I see!
And is there a way to add this setting to existing feeds processor, for example FeedsNodeProcessor, besides hacking it?
Comment #230
gcbYou could just extend feedsnodeprocessor with your own and only override that one function.
Comment #231
vbard CreditAttribution: vbard commented@gcb Thank you for that piece of knowlеge, I appreciate this a lot!
Although, I went my way and did some explorations on feeds_empty module. Here is a patch for it https://www.drupal.org/node/2323785#comment-11305767