In testing instructions, it might also be good to test that we are still able to create potentially non-standard CSV files created in Excel or OpenOffice.

Issue size: XL

This patch will accept rfc-4180 compliant csv files. A unit test has also been included with it.

For the peer reviewer
The code on line 98:

$csv_encode    = csv_import_reader::get_encoded_delimiter($delimiter_name);

I'm not sure if this is required. Some examples that I could try it against would be most appreciated.

Hi Adrien,

All looks great to me, reading php.net I've found http://www.php.net/manual/en/function.fgetcsv.php#98800 but editing a test case with that example the csv_import_reader passes the test without problems. The only thing I've seen, but not 100% related with this issue, is that there is no option to specify a different enclosure for the records than the double quote, mod_data has a "field enclosure" form element, but it's never used.

Thanks David for the review.

I've added another parameter to load_csv_content. It has a default value so it should not affect any existing code.

Putting up for integration review.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Thanks guys, this has been integrated now

Adrian has asked me to pull this out of this weeks integration.

Reopening as requested. I've pushed a revert.

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

The reason that I pulled this out of integration is because it was causing a regression when uploading users into Moodle. After looking around I found that I was just slightly altering the way that the class behaved. Since then I have re-written the code to follow this funcitonality.

Things to note:

• I know that I am loading the csv data twice, but this seems to be less memory intensive compared to using str_getcsv().
• I have stored the data as a csv file because serialized data seems to have trouble with special characters in the arrays.

Hi Adrian,

As we just discussed, with the out of memory limitation of str_getcsv() I only see two alternatives to avoid the temp file creation; replicate the fgetcsv() line-splitter functionality (lot of work) or change the interface of load_csv_content() or create a new load_csv_file() receiving the file path instead of the file contents...

Thanks David for having another look at this for me.

• I'm not really a huge fan of re-writing the functionality of fgetcsv().
• I don't think that I'm allowed to change the functionality of the class, as it may break plugins that might be using it.

I've put a lot of thought into this solution and I'm not 100% convinced that it's the right solution. I'll put it up for integration and see what other people think of it.

The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

doesn't seem minor, setting to major - (also no work around)

Hi,
what is the "out of memory limitation of str_getcsv()" limitation mentioned here? is there any data about the limit?

Hi Apu,

I was originally getting an out of memory error when I was first testing out the str_getcsv() function in my code. I have gone back and put it back in to see exactly what the message was that I was getting, but I've found that now (and I'm not exactly sure why) I'm not getting that message and it is processing the csv file. So I did some more testing.

While str_getcsv doesn't have any memory problems, it does not parse the csv data properly http://php.net/manual/en/function.str-getcsv.php durik@3ilab.net.
I tried it myself and had the same problems. I tried out the solution that they provided, but it doesn't handle information on multiple lines.

With the current solution I've tested it out with a csv file which contains 3000 users. It loaded that into the system with no troubles. (415 secs)

I'll leave it up to you as to whether using str_getcsv() should be investigated further or not.

Thanks.

well this is definitely a step forward towards rfc compliant csv files here so its been integrated into master only.

Perhaps a side issue can be created (if you can replicate the memory issues you mention). For now this is into master to be tested, and soon it'll be out there for real world feedback

Tested test one and it passed No need to run the unit tests as they run in the nightly build.

I'm so proud...of you, many thanks!

http://youtu.be/n64CdfDRnZY

Closing as fixed, ciao