I am generating unique ids from some other fields being imported (via csv/spreadsheet upload), which means this unique field isn't meant to be provided by the user directly. So it has a blank label in it's source. However, when users download the template file, they get a blank column (no header) as the first column. This is because the FeedsCSVParser->getTemplate() merges unique fields and sources together, with the uniques as the first argument of array_merge()

foreach (array_merge($uniques, $sources) as $col) {

In other words, the blank column is first regardless of the order it is defined in the processor. Understandably, this may be a bit of a specific use case, and doesn't really cause any serious bugs per se, but having an empty column in the beginning of the document looks weird to external users.

I propose adding a check on the value of $col before appending to the $columns array in FeedsCSVParser->getTemplate() ,

      if (!empty(trim($col))) {
        $columns[] = $col;
      }

Patch coming. Take it or leave it. I just have a policy of filing an issue and patch if I need to modify a contrib module.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bburg created an issue. See original summary.

bburg’s picture

Status: Active » Needs review
FileSize
598 bytes

Patch

Status: Needs review » Needs work

The last submitted patch, 2: feeds-empty_template_columns-2834026-1.patch, failed testing.

MegaChriz’s picture

Tests are failing because prior to PHP 5.5, the function empty() could only be used on variables.
http://php.net/empty
The documentation page of the empty() function suggests to use trim($name) == FALSE instead:

Prior to PHP 5.5, empty() only supports variables; anything else will result in a parse error. In other words, the following will not work: empty(trim($name)). Instead, use trim($name) == false.

bburg’s picture

Good catch, thanks!

Here's the new patch.

bburg’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: feeds-empty_template_columns-2834026-5.patch, failed testing.

MegaChriz’s picture

Sorry for maybe a bit of confusion. The column should be added when it is *not* empty. Now only empty columns are added ;). trim($name) == FALSE == empty.

bburg’s picture

Doh! That's what I get for not testing locally first. I'll push another in a bit.

bburg’s picture

Status: Needs work » Needs review
FileSize
599 bytes
MegaChriz’s picture

I noticed one issue with the patch. If you had a column name called 0 in the source mappings, that zero wouldn't end up in the CSV template either.
Additionally, when using a space as column name and used that as unique, the texts looked a bit weird on the import page:

So this called for additional tests. See attached patch for that. To fix the issue with 0, I used strlen(trim($col)) instead.

  • MegaChriz committed 8c7ebf0 on 7.x-2.x authored by bburg
    Issue #2834026 by bburg, MegaChriz: Fixed do not add blank column...
MegaChriz’s picture

Status: Needs review » Fixed

Committed #11. Thanks, bburg!

Status: Fixed » Closed (fixed)

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