Details

    • Testing Instructions:
      Hide

      Test One

      1. Create a database module.
      2. Go to the presets tab and upload the attached preset.
      3. Go to [Settings->Database activity administration->Import entries.
      4. Load the rfc-4180 compliant csv file attached.

      [TEST] Check that no errors are produced and view the entries inserted into the database. They should be inserted as expected.

      Test Two

      • Run the phpunit test csv_testcase lib/tests/csvtest.php
        command line (phpunit csv_testcase lib/tests/csvtest.php)

      [TEST] No error should occur.

      Show
      Test One Create a database module. Go to the presets tab and upload the attached preset. Go to [Settings->Database activity administration->Import entries. Load the rfc-4180 compliant csv file attached. [TEST] Check that no errors are produced and view the entries inserted into the database. They should be inserted as expected. Test Two Run the phpunit test csv_testcase lib/tests/csvtest.php command line (phpunit csv_testcase lib/tests/csvtest.php) [TEST] No error should occur.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-34075-master
    • Rank:
      42380

      Description

      RFC 4180 compliant csv files can not currently be cleanly imported into moodle.

      RFC 4180 standard: http://tools.ietf.org/html/rfc4180

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          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.

          Show
          Michael de Raadt added a comment - 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.
          Hide
          Adrian Greeve added a comment -

          Issue size: XL

          Show
          Adrian Greeve added a comment - Issue size: XL
          Hide
          Adrian Greeve added a comment -

          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.

          Show
          Adrian Greeve added a comment - 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.
          Hide
          David Monllaó added a comment -

          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.

          Show
          David Monllaó added a comment - 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.
          Hide
          Adrian Greeve added a comment -

          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.

          Show
          Adrian Greeve added a comment - 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.
          Hide
          Dan Poltawski added a comment -

          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

          Show
          Dan Poltawski added a comment - 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
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks guys, this has been integrated now
          Hide
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - Adrian has asked me to pull this out of this weeks integration.
          Hide
          Dan Poltawski added a comment -

          Reopening as requested. I've pushed a revert.

          Show
          Dan Poltawski added a comment - Reopening as requested. I've pushed a revert.
          Hide
          CiBoT added a comment -

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

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Adrian Greeve added a comment - - edited

          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.
          Show
          Adrian Greeve added a comment - - edited 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.
          Hide
          David Monllaó added a comment -

          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...

          Show
          David Monllaó added a comment - 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...
          Hide
          Adrian Greeve added a comment -

          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.

          Show
          Adrian Greeve added a comment - 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.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - doesn't seem minor, setting to major - (also no work around)
          Hide
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - Hi, what is the "out of memory limitation of str_getcsv()" limitation mentioned here? is there any data about the limit?
          Hide
          Adrian Greeve added a comment -

          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.

          Show
          Adrian Greeve added a comment - 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.
          Hide
          Aparup Banerjee added a comment -

          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

          Show
          Aparup Banerjee added a comment - 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
          Hide
          Tim Barker added a comment -

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

          Show
          Tim Barker added a comment - Tested test one and it passed No need to run the unit tests as they run in the nightly build.
          Hide
          Eloy Lafuente (stronk7) added a comment -

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

          http://youtu.be/n64CdfDRnZY

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: