According to this RFC http://tools.ietf.org/rfc/rfc4180.txt, a field should be fully double quoted, when something uncommon happens in it, doubling other DQUOTES inside (if there are any). For example:
;A string with a "double" quote in it;
should break parsing process. The correct variant would be:
;"A string with a ""double"" quote in it";
Current implementation will accept the former and will feed the fields values with subsequent lines until next double quote.
This should throw an exception with corresponding message about it, stopping parsing.
===
Splitting #1428272: Added support of encoding conversions to the CSV Parser, part 2.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | feeds-n1720724-10.patch | 2.9 KB | damienmckenna |
| #8 | feeds-fix-dquotes-handling-1720724-8.patch | 3.01 KB | Niremizov |
| #7 | feeds-fix-dquotes-handling-1720724-7.patch | 2.68 KB | Niremizov |
| #1 | feeds-fix-dquotes-handling-1720724-0.patch | 3.52 KB | OnkelTem |
Comments
Comment #1
OnkelTem commentedComment #2
twistor commentedI'm reluctant to change the behavior of the parser. It's been going strong now for a long time.
What we'll need are solid tests. I know there's an RFC and all, but people won't care if their Excel exports don't work, so we need a bunch of test data from common sources of CSV data to work with. Also, some at least one test that fails with the current parser that this one fixes.
Comment #3
twistor commentedIf we had all of those tests, then #1369874: Don't roll own CSV parser; use PHP's native one could start to look like a more valid solution.
Comment #4
OnkelTem commented@twistor
Parser
I agree that existing stuff must continue functioning as before and of course Excel products too.
From the other hand, algorithms should evolve, as the current implementation is unable to correctly parse DQUOTED data for example.
Test cases
I'm not a big fan of M$ apps, and have no good test cases ready at hand, but, of course, we need more tests (now there is only couple of simple, correct and
latin-1-encoded .CSV files in the test folder).fgetcsv()
I didn't know about fgetcsv(), but now after reading it's "man page" I found it's pretty good. I vote for giving it a chance, but to try it would require to introduce changes in ParserCSVIterator class which make it incompatible with current implementation.
Proposition
I propose to create three (or two - current and fgetcsv()) different implementations of CSVParser making appropriate one selectable for user on the parser configuration form.
What do you think?
Comment #5
twistor commentedWhat I meant about Excel files was that we need to assemble an assortment of CSV files, since there really no "one true way" to create a CSV file. I don't mean to put a huge burden on this issue, but I put the same burden on anyone who wants to drastically change the current CSV parser.
I'm not too concerned with changing ParserCSVIterator. If it turns out to be an issue, we can leave it, and move the fgets implementation to something else. That said, I would not want to have multiple CSV parsers included with Feeds. That would turn into all kinds of headaches. I think the ideal thing would be to fully develop the fgets parser someplace else, and move it in when it's mature.
Back to the issue.
Absolutely, we should support DQUOTED CSV files. I don't really use CSV files too much myself, and I would like to know if strict adherence to the spec would break things for current users. Specifically, requiring double quotes for quotes inside a column.
Comment #5.0
twistor commentedUpdated summary
Comment #6
Niremizov commentedSuppose if could get ParserCSV to be more configurable, it could solve such issues. For example, at the moment such line (with ; as separator):
22B02 Arena_S Actau, mkr. 16, "Actau";testIs be parsed as 1 column, becouse dquotes are not escaped. Would be nice if such behaviour could be configured.
Comment #7
Niremizov commentedOk, here is patch that adds inside CSV Parser config form checkbox that allows to turn off Detecting of DQuotes inside file, probably his would be usefull for someone.
Comment #8
Niremizov commentedAdded few fixes to prev. patch.
Comment #9
twistor commentedThis shouldn't be an option, it should just work. Either the field is quoted or not, if not, ignore quotes inside.
I wonder how much things would break if we ported the CSV parser from 8.x-3.x.
Comment #10
damienmckennaRerolled, and simplified so that it uses the $source_config instead of $config.
Incidentally, it might be worth also looking at #3004499: Drush feeds-import command ignores stored config for CSV parser as I found other bugs in the importer.
Comment #12
bluegeek9 commentedDrupal 7 reached end of life and the D7 version of Feeds is no longer being developed. To keep the issue queue focused on supported versions, we’re closing older D7 issues.
If you still have questions about using Feeds on Drupal 7, feel free to ask. While we won’t fix D7 bugs anymore, we’re happy to offer guidance to help you move forward. You can do so by opening (or reopening) a D7 issue, or by reaching out in the #feeds channel on Drupal Slack.
If this issue is still relevant for Drupal 10+, please open a follow-up issue or merge request with proposed changes. Contributions are always welcome!