This issue is part of #2027583: [meta] Behavior when importing blank values
Original Report
content_taxonomy_feeds_set_target() had a bug whereby empty cells were importing as taxonomy tids with empty names. to fix, change FROM
// Return if there are no or empty terms.
if (!is_array($terms) || empty($terms)) {
return;
}
TO
// Return if there are no or empty terms.
if (!is_array($terms) || (count($terms) == 1 && empty($terms[0]))) {
return;
}
this relates to issues like:
http://drupal.org/node/1047894
http://drupal.org/node/1107522
Comment | File | Size | Author |
---|---|---|---|
#23 | feeds-empty-term-name-1668186-23.patch | 2.83 KB | drclaw |
#19 | feeds-empty-term-name-1668186-19.patch | 639 bytes | drclaw |
#16 | feeds-empty-term-name-1668186-16.patch | 633 bytes | twistor |
#11 | feeds_taxonomy_skip_empty_values-1668186-11.patch | 340 bytes | Douglas Marsh |
#8 | feeds-taxonomy-empty-values-1668186-8.patch | 485 bytes | drclaw |
Comments
Comment #1
tsmulugeta CreditAttribution: tsmulugeta commentedWhere is this line of code you are referring to? In which file?
Comment #2
heacu CreditAttribution: heacu commentedright there in content_taxonomy_feeds_set_target(), as i said
Comment #3
Sinan Erdem CreditAttribution: Sinan Erdem commentedI think he is asking in which file...
Comment #4
Sinan Erdem CreditAttribution: Sinan Erdem commentedThe problem persists on D7 also.
Comment #5
InternetDevels CreditAttribution: InternetDevels commentedhope this will help:
http://drupal.org/node/1107522#comment-5224310
Comment #6
kingandy CreditAttribution: kingandy commentedThe patch in that comment (#13) doesn't appear to apply to the current releases (7.x-2.0-alpha8 or 7.x-2.0-dev). It is from 2011, so hardly surprising...
The latest patch (in #81) does seem to apply, but doesn't handle taxonomy fields, only text ones. It sounds like the plan is to create separate issues for each data type. I'll see if they want to use this issue as a base.
Comment #6.0
a.ross CreditAttribution: a.ross commentedLinking to meta-issue
Comment #6.1
a.ross CreditAttribution: a.ross commentedUpdated issue summary.
Comment #7
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 #8
drclaw CreditAttribution: drclaw commentedI think the problem is that the check for Zero needs to use an "identical" operator instead of just an equals operator. Patches attached for dev and alpha8.
Comment #9
drclaw CreditAttribution: drclaw commentedOops forgot to change status
Comment #10
johnvCheck out #1107522-63: Framework for expected behavior when importing empty/blank values + text field fix
Comment #11
Douglas Marsh CreditAttribution: Douglas Marsh commentedI can't believe this is still an issue. Can apply the following patch to current (7.x-2.0-alpha8) or -dev (as of this date):
Just apply this to current/dev and get this taken care of. Taxonomy terms with zero length string(s) are a pain when importing and this fixes. Is there a plan to fix this someplace else in the code.
Comment #12
MegaChriz CreditAttribution: MegaChriz commented@Douglas Marsh
"patch (to be ported)" means there is a patch committed to a branch that needs to modified to commit it to an other branch. I think you meant that you supplied a new patch.
Setting status to "needs review".
Comment #13
twistor CreditAttribution: twistor commentedI'm guessing this is what the problem is.
Comment #14
sgabe CreditAttribution: sgabe commentedThe patch in #8 seems to do the trick.
Comment #15
zserno CreditAttribution: zserno commentedPatch in #11 worked for me. #8 did not make any difference. Tried with alpha8 only.
Comment #16
twistor CreditAttribution: twistor commentedWe should avoid using empty() when checking since it will fail for the string '0', and that is a valid term name, or GUID. Even an empty string is a valid GUID.
I'd rather have the check here, since it changes existing behavior the least. Not convinced though.
Comment #17
Douglas Marsh CreditAttribution: Douglas Marsh commentedI believe twistor is correct. I have not looked at the patch in a production environment, but I believe his/her patch (checking string length) is a better direction. Mind you I think a trim() is in order:
+ elseif ($mapping['autocreate'] && strlen(trim($term))) {
I will look into trying this method as I agree my hack didn't account for legit conditions ("0" or number 0 for example).
Thanks for catching that and finding the line. I was looking for a "better" place to catch the autocreate logic and add the test for a (trimmed) zero length string on the term name.
UPDATE: I implemented this patch into production now and I am much happier!
--Doug
Comment #18
twistor CreditAttribution: twistor commentedYes, there should be a trim in there.
Comment #19
drclaw CreditAttribution: drclaw commentedNew patch based on comment #17
So... how should we test this? Does it even need it's own test? I don't think the other mappers have tests for empty values (maybe they need them too?)
Comment #20
MegaChriz CreditAttribution: MegaChriz commentedHow I think this should be tested:
Yes, other mappers should have them too, though I think that is out of scope for this issue. Tests for other mappers should be done in, for example, #1107522: Framework for expected behavior when importing empty/blank values + text field fix.
Comment #21
drclaw CreditAttribution: drclaw commentedCool. Sounds good. I'm not sure we need the second test though. If a field is missing from the source, I think it's ignored entirely.
Otherwise, I think this sounds like a good plan. I'll see what I can do. =)
Comment #22
MegaChriz CreditAttribution: MegaChriz commentedI agree that the second test is not needed
Now that I think of it, I think you (drclaw) are right the second test isn't really needed in the way I described it, as it is completely covered by the first test. If the first test already proves that no empty terms are created with an empty value, then I guess there is no need to prove that it will happen also with no value at all provided.
I guess I was confused with some other issues (#2093651: Simplify target callbacks. and #1107522: Framework for expected behavior when importing empty/blank values + text field fix) where a change was proposed that when a field is defined as mapping, but does not exists in the source, the same thing should happen as when it is in the source, but with a empty value. The reason behind this is that for some parsers (like the XPath-parser, if I'm not wrong) the difference between empty value and non-existent value can not be made.
A case for the second test, but out of scope for this issue
Anyway, a case where the second test would be useful is to test if values are wiped out in the following two situations:
But that's beyond the scope of this issue, that is just about making sure that no empty terms are created. It's not about wiping out values.
That currently depends on the implementation of the parser. This may count for the CSV-parser, but not for the XPath-parser. That inconsistency is subject of change.
New test plan
To keep things clear, here is a new test plan based on the plan in #20 but without the 'second test'. I also added that a record with a term called '0' should be added, as in that case a term should be created.
0
(should create a term).Comment #23
drclaw CreditAttribution: drclaw commentedThanks for the feedback. Here's a re-roll with tests!
Comment #23.0
drclaw CreditAttribution: drclaw commentedUpdated issue summary.
Comment #24
Mschudders CreditAttribution: Mschudders commentedCan confirm that the latest patch is working #23
Note: Delete the empty taxonomy terms before re importing.
Comment #25
cmseasy CreditAttribution: cmseasy commented#23 worked for me
Comment #26
BD3 CreditAttribution: BD3 commented#23 also worked for me!
Comment #27
twistor CreditAttribution: twistor commentedThanks everybody!
I screwed up the commit message. Doing too many things at once.
http://drupalcode.org/project/feeds.git/commit/5fe509c
Does this need a backport?
Comment #29
MegaChriz CreditAttribution: MegaChriz commentedComment #30
cmseasy CreditAttribution: cmseasy commentedSorry for re-opening.
Patch #23 is commited to current dev jul-2-2015, not in 7.x-2.0-alpha9 release, can somebody confirm this?
Comment #31
MegaChriz CreditAttribution: MegaChriz commentedYes, the commit was done after the 7.x-2.0-alpha8 release and the 7.x-2.0-alpha9 release is a security-only release. This means that any non-security bug fixes that were added after 7.x-2.0-alpha8 only exist in dev.
Comment #32
cmseasy CreditAttribution: cmseasy commentedAfter updating to 7.x-2.0-beta1 my upload did not work. I did not see this issue solved in the issue list https://www.drupal.org/node/2531428. So changed back to 7.x-2.0-alpha8.
For both: 7.x-2.0-beta1 and the latest dev I get:
SQLSTATE[22007]: Invalid datetime format: 1366 Incorrect decimal value: '' for column 'list_price' at row 1
Upload works great with the patched and unsafe alpha8 version (#23) .
Reopening because it is still an issue in latest dev and beta1.
Comment #33
MegaChriz CreditAttribution: MegaChriz commented@cmseasy
That error message seems to be unrelated to this issue. I recently answered a support request that had the same error message. See #2540960: General error: 1366 Incorrect decimal value:. It is likely that the source value for "list_price" is incorrect. Maybe it contains a valuta sign.
The fix for this issue is in fact mentioned on the release notes:
Also the test FeedsMapperTaxonomyTestCase::testBlankSourceValues() is still passing, so that should prove that this issue is fixed. Note that
0
is considered to be a valid term name.Closing this issue again for the above reasons. Reopen this issue if you still notice that empty terms get created. If you do, it would be very helpful if you could tell the steps to reproduce the issue. Thanks.
Comment #34
SevyX CreditAttribution: SevyX commentedIs this issue still alive, I am getting the same error code when importing products into Ubercart through feeds, doing the same thing I have done before which worked but now getting the error
Invalid datetime format: 1366 Incorrect decimal value: '' for column 'list_price' at row 1
Using latest stable version of Feeds 7.x-2.0-beta2.
Anyone know how to fix this?
Comment #35
SevyX CreditAttribution: SevyX commentedHi,
would anyone have the patched files available for download?
Much appreciated.
Comment #36
SevyX CreditAttribution: SevyX commentedComment #37
MegaChriz CreditAttribution: MegaChriz commented'list_price' has nothing to do with empty terms.