Incorrect checks applied to CSV files

[timhunt]

https://docs.fileformat.com/spreadsheet/csv/ says "Each record is located on a separate line, delimited by a line break (CRLF)."

CodeChecker is telling me

"Windows (CRLF) line ending instead of just LF (reporting only first occurrence)"

on my CSV file. I think CodeChecker is wrong.

[timhunt]

Seems to only appy when running CodeChcker form the Moodle UI, not in github actions.

[stronk7]

I bet that is one of those "special" (apart from PHP_Codesniffer) checks that were added to the plugin. Lately have found myself fixing some of them. Really they don't add much, but confusion, heh.

Will take a look to this case soon...

[stronk7]

Yeah, confirmed. That check is there since the night of times:

1d808e0e#diff-3494b45596f0f648d894a5f22070eaae01f3e0ea664be0430b8d23eb7d93e35aR220-R224

And I think it just means that no file within Moodle should be CRLF ended ever. Including CSV files.

Looking to load_csv_content(), we always normalise everything (CR, LF and CRLF) to LF, and then use fgetcsv() with those, already converted, LF.

So I think it's ok to require all CSV files in codebase (some fixture, I bet?) to be LF and not CRLF. Hence, maybe this should be closed?

[timhunt]

I think that the point of testing is to verify that your system will work for users.

Users will create CSV files by saving from Excel, so we should check that those import correctly. Therefore, test fixture CVS files are better tests if they have CR LF. Therefore we should allow it.

At least, that is what I think.

[stronk7]

Would be ok enough to make an exception for anything under a "tests/fixtures" dir and allow everything there?

Allow everything = I mean, don't verify EOLs in CSV files under "tetst/fixtures" dirs.

Ciao :-)

[timhunt]

It might be.

Or, that might just be a confusing inconsistency. My feeling is the same rules should apply to a file type everywhere.

[stronk7]

Yeah, the rule is / has been pretty consistent since the beginning, LOL. Don't allow CRLF files ever, point!.

It's not me the one proposing to add exceptions, just wanted to restrict them to the real cases where we can effectively accept CRLF CSV files. And the only case is fixtures.

If for some (strange) reason we want a CSV file elsewhere, it should be a correct LF one (for example: admin/tool/uploaduser/example.csv).

[timhunt]

We don't allow CRLF in .php, .css, .txt,.. Obviously that is right.

But, CRLF is in the definition of .csv. (https://www.rfc-editor.org/rfc/rfc4180.html). I know .csv is not massively standardised, but requiring all our example .csv files to break the standard seems a strange decision.

[stronk7]

I've created #205 that enabled the use of CRLF CSV files under the fixtures directory.

I really don't see much benefit allowing them everywhere. The reality (the facto "standard") is that both work the same and we aren't going to add CRLF examples elsewhere (haven't in 20 years).

I understand that we should be able to test that CRLF imports work and that's exactly what the PR provides.

So, in practice, with that PR, you will be able to use those CRLF CSV fixtures and done. I think nobody is going to convince the other, LOL.

Ciao :-)

[timhunt]

Fair enoguh. Thanks for doing it Eloy.