Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-36113

Importing entries into a database activity from a csv file containing empty lines results in an error

    Details

    • Testing Instructions:
      Hide

      Test steps

      1. Create a database activity.
      2. Create two fields.
      3. Create a csv file similar to the one mentioned in the description (It must have blank lines in the middle of the data).
      4. Go to [Settings ► Database activity administration ► Import entries].
      5. Upload the file.
        • Ensure that no errors are displayed and that the entries are imported correctly.
      Show
      Test steps Create a database activity. Create two fields. Create a csv file similar to the one mentioned in the description (It must have blank lines in the middle of the data). Go to [Settings ► Database activity administration ► Import entries] . Upload the file. Ensure that no errors are displayed and that the entries are imported correctly.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-36113-master

      Description

      We're handling empty lines at the end of the file but if an empty line is found in the middle a fatal error occurs.

      The database activity has a URL and text area field.

      myblog, about me
      http://bob.com,I am bob. Fear me.
      http://joe.com, I am joe. I like Joe. Joe good.
       
      http://blamo.com,blamo i am.
       
       

      Unable to read the raw data from the CSV file
       
      More information about this error
      Debug info:
      Error code: csvfailed
      Stack trace:
       
          line 467 of /lib/setuplib.php: moodle_exception thrown
          line 107 of /mod/data/import.php: call to print_error()

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              salvetore Michael de Raadt added a comment -

              This relates to the CSV class.

              I wonder if the CSV standard allows blank lines.

              Show
              salvetore Michael de Raadt added a comment - This relates to the CSV class. I wonder if the CSV standard allows blank lines.
              Hide
              phalacee Jason Fowler added a comment -

              [ ] Syntax
              [ ] Output
              [ ] Whitespace
              [ ] Language
              [ ] Databases
              [ ] Testing
              [ ] Security
              [ ] Documentation
              [ ] Git
              [ ] Sanity check – Heath Forscyth must be crazy ... no one likes Fosters beer right?

              Show
              phalacee Jason Fowler added a comment - [ ] Syntax [ ] Output [ ] Whitespace [ ] Language [ ] Databases [ ] Testing [ ] Security [ ] Documentation [ ] Git [ ] Sanity check – Heath Forscyth must be crazy ... no one likes Fosters beer right?
              Hide
              phalacee Jason Fowler added a comment -

              Good to go Adrian

              Show
              phalacee Jason Fowler added a comment - Good to go Adrian
              Hide
              abgreeve Adrian Greeve added a comment -

              Thanks for the review Jason.

              Submitting for integration.

              Show
              abgreeve Adrian Greeve added a comment - Thanks for the review Jason. Submitting for integration.
              Hide
              poltawski Dan Poltawski added a comment -

              Hi Adrian,

              Looking at the php manual (http://php.net/manual/en/function.fgetcsv.php), it says:

              "Note: A blank line in a CSV file will be returned as an array comprising a single null field, and will not be treated as an error.".

              So I wonder if it would be better to explicitly testing for null in $fgetdata[0]. (i.e. $fgetdata[0] !== null).

              Show
              poltawski Dan Poltawski added a comment - Hi Adrian, Looking at the php manual ( http://php.net/manual/en/function.fgetcsv.php ), it says: "Note: A blank line in a CSV file will be returned as an array comprising a single null field, and will not be treated as an error.". So I wonder if it would be better to explicitly testing for null in $fgetdata [0] . (i.e. $fgetdata [0] !== null).
              Hide
              abgreeve Adrian Greeve added a comment -

              Thanks Dan,

              I've made the change as you suggested. I've uploaded the change.

              Show
              abgreeve Adrian Greeve added a comment - Thanks Dan, I've made the change as you suggested. I've uploaded the change.
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks Adrian, i've integrated this now

              Show
              poltawski Dan Poltawski added a comment - Thanks Adrian, i've integrated this now
              Hide
              fred Frédéric Massart added a comment -

              Test passed on master. Thanks!

              However, please note that using the CSV sample provided will not work. The column name 'about me' contains a space at the beginning which prevents it from being matched with the database field. I don't think this is an issue, and is definitely not linked to this issue (hence not failing it). I have discussed about it with Adrian which will decide if we should, or not, raise an issue to trim column names.

              Show
              fred Frédéric Massart added a comment - Test passed on master. Thanks! However, please note that using the CSV sample provided will not work. The column name 'about me' contains a space at the beginning which prevents it from being matched with the database field. I don't think this is an issue, and is definitely not linked to this issue (hence not failing it). I have discussed about it with Adrian which will decide if we should, or not, raise an issue to trim column names.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

              (not really)

              Closing, thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    3/Dec/12