Taxonomy terms auto-created with empty name.

Where is this line of code you are referring to? In which file?

I think he is asking in which file...

The problem persists on D7 also.

hope this will help:
http://drupal.org/node/1107522#comment-5224310

The 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.

Linking to meta-issue

Updated issue summary.

I have sucessfully applied patch from #63 to latest dev version (7.x-2.x-dev 2013-Jul-06) and this did solve my problems.

I 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.

Oops forgot to change status

Check out #1107522-63: Framework for expected behavior when importing empty/blank values + text field fix

I 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):

--- feeds/mappers/taxonomy.inc.orig	2013-09-05 09:10:31.056594222 -0700
+++ feeds/mappers/taxonomy.inc	2013-09-05 09:13:10.158521559 -0700
@@ -124,6 +124,10 @@
   // Iterate over all values.
   foreach ($terms as $term) {
 
+    if (empty($term)) {
+      continue;
+    }
+
     if ($info['cardinality'] == $delta) {
       break;
     }

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.

@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".

I'm guessing this is what the problem is.

The patch in #8 seems to do the trick.

Patch in #11 worked for me. #8 did not make any difference. Tried with alpha8 only.

We 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.

I 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

Yes, there should be a trim in there.

New 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?)

How should we test this?

How I think this should be tested:

1. Add a test method to the FeedsMapperTaxonomyTestCase class.
2. Add two source files (rss or csv), both with content for nodes.
   • In the first source file, add at least fields "guid", "title" and "category". This file will be used to test if terms that only consist of spaces are skipped.
   • In the second source file, add at least fields "guid" and "title" but omit the field "category". This file will be used to test if no terms are created at all.
3. Fill the source files with a couple of records. In the first source file, at least one record should have a valid category, at least one record should have a category with a single space, and at least one record should have a category that has no characters at all.
4. In the test, create a node importer with mappers for at least guid, node title and taxonomy reference field.
5. In the test, import the first source file and assert the number of terms created.
6. In the test, import the second source file and assert the number of terms created.

I don't think the other mappers have tests for empty values (maybe they need them too?)

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.

Cool. 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. =)

I 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:

• When an empty value is provided.
• When no value is provided.

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.

If a field is missing from the source, I think it's ignored entirely.

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.

1. Add a test method to the FeedsMapperTaxonomyTestCase class.
2. Add a source file (rss or csv) with content for nodes. Add at least fields "guid", "title" and "category" to this file.
3. Fill the source file with a couple of records:
   • At least one record with a valid category (should create a term);
   • At least one record with a category that consists of a single space (should NOT create a term);
   • At least one record with a category that has no characters at all (should NOT create a term);
   • At least one record with a category that is 0 (should create a term).
4. In the test, create a node importer with mappers for at least guid, node title and taxonomy reference field.
5. In the test, import the source file and assert the number of terms created. It's probably also good to assert which terms were created instead of asserting only the amount of terms.

Thanks for the feedback. Here's a re-roll with tests!

Updated issue summary.

Can confirm that the latest patch is working #23

Note: Delete the empty taxonomy terms before re importing.

#23 also worked for me!

Thanks 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?

Yes, 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.

@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:

#2149829 by chilic: Import/Update 0 value to text field.

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.

Is 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?

Hi,

would anyone have the patched files available for download?

Much appreciated.

'list_price' has nothing to do with empty terms.