It appears that when Feeds provides a downloadable template for TSV, it inserts the word "TAB" in place of actual tabs, which is obviously less than ideal.

Steps to Reproduce
1. Configure a feed importer that uses CSV with "TAB" as the default delimiter.
2. Open the import page for the importer.
3. Click on the link to get a sample template.

Expected Results
The template is delimited with tabs between the columns.

Actual Results
The template is delimited with the word "TAB" between the columns.

CommentFileSizeAuthor
#9 feeds-tsv_delimiter_handling-2629620-9-7.x.patch5.29 KBguypaddock
#8 feeds-tsv_delimiter_handling-2629620-8-7.x.patch5.29 KBguypaddock
#4 feeds-tsv_delimiter_handling-2629620-4-7.x.patch4.63 KBguypaddock
#3 feeds-tsv_delimiter_handling-2629620-3-7.x.patch4.63 KBguypaddock
#2 feeds-csv-template-tab-delimiter-2629620-2.patch536 bytesmegachriz

Comments

GuyPaddock created an issue. See original summary.

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Active » Needs review
StatusFileSize
new feeds-csv-template-tab-delimiter-2629620-2.patch536 bytes

Easy enough to fix. Thanks for the clear bug report!

guypaddock’s picture

guypaddock commented
StatusFileSize
new feeds-tsv_delimiter_handling-2629620-3-7.x.patch4.63 KB

Attached is a better patch that:
- Removes special case handling for "TAB" in all of the places we have it in the CSV parser, moving it to a utility method.
- Also fixes the file extension so that tab-separated CSVs end in TSV so programs like LibreOffice and Excel properly determine the delimiter.
- Moves the list of delimiters used in the Admin UI to one utility method so we have only one place to update if we need to change the delimiters.

guypaddock’s picture

guypaddock commented
StatusFileSize
new feeds-tsv_delimiter_handling-2629620-4-7.x.patch4.63 KB

Noticed a slight style typo, so here's a new version of the previous patch.

megachriz’s picture

megachriz
Primary language Dutch
commented

Cool! Thanks for improving this.

Code review:

  1. +++ b/plugins/FeedsCSVParser.inc
@@ -234,18 +223,100 @@ class FeedsCSVParser extends FeedsParser {
+    drupal_add_http_header(
+      'Cache-Control',
+      'max-age=60, must-revalidate');
+
+    drupal_add_http_header(
+      'Content-Disposition',
+      'attachment; filename="' . $filename . '"');

    I think that the code is more readable if we leave these function calls at one line (I see that there are no coding standards for this, except discussion: #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines). 

  2. +++ b/plugins/FeedsCSVParser.inc
@@ -234,18 +223,100 @@ class FeedsCSVParser extends FeedsParser {
     drupal_add_http_header('Content-type', 'text/csv; charset=utf-8');

    Maybe we should change the media type as well for tsv files? According to the wikipedia article about tab-separated values, the media type should be text/tab-separated-values

megachriz’s picture

megachriz
Primary language Dutch
commented

3.

+++ b/plugins/FeedsCSVParser.inc
@@ -234,18 +223,100 @@ class FeedsCSVParser extends FeedsParser {
+    $configDelimiter = $config['delimiter'];
+
+    switch ($configDelimiter) {
+      case 'TAB':
+        $delimiter = "\t";
+        break;
+
+      default:
+        $delimiter = $configDelimiter;
+        break;
+    }
+
+    return $delimiter;
...
+    switch ($config['delimiter']) {
+      case 'TAB':
+        $extension = 'tsv';
+        break;
+
+      default:
+        $extension = 'csv';
+        break;
+    }
+
+    return $extension;

Instead of assigning the value to a variable first, we might as well directly return the value, as nothing else is done with the variable in the rest of the function.

guypaddock’s picture

guypaddock commented

@MegaChriz: In response to point 1 in #5, this was to address the line length of those calls as the lines were longer than 80 chars:
https://www.drupal.org/coding-standards#linelength

I agree with point 2 in #5. I can make that change.

In response to point 3, this is to enforce a "one entry, one exit" style of coding so it's easier to add debug print statements, breakpoints, etc, as well as to clarify where the function returns:
http://stackoverflow.com/questions/4838828/why-should-a-function-have-on...

guypaddock’s picture

guypaddock commented
StatusFileSize
new feeds-tsv_delimiter_handling-2629620-8-7.x.patch5.29 KB

Attached is a new patch that addresses the following:
- Splits up what was originally a wrapped function call into variables and calls so that we don't have to wrap.
- Replaces getTemplateExtension() with getTemplateFileDetails(), which now provides both the file extension and mime type.

guypaddock’s picture

guypaddock commented
StatusFileSize
new feeds-tsv_delimiter_handling-2629620-9-7.x.patch5.29 KB

Small fix for local variable naming style.

guypaddock’s picture

guypaddock commented

  • MegaChriz committed ef02afd on 7.x-2.x authored by GuyPaddock 
    Issue #2629620 by GuyPaddock, MegaChriz: Fixed template for TSV contains...
megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed

@GuyPaddock, #7
Thanks for the clarification on point 3. It seem to be a matter of personal preference. I'm okay with it.

I tested the patch in #9 by downloading a template for every delimiter. I also checked the output difference in the code for "TAB" and ",". I looked through the code again and I did not see anything suspicious. All is good. Great function documentation, by the way!

Committed #9.

Status: Fixed » Closed (fixed)

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