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
    • Rank:
      44879

      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()
      

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          This relates to the CSV class.

          I wonder if the CSV standard allows blank lines.

          Show
          Michael de Raadt added a comment - This relates to the CSV class. I wonder if the CSV standard allows blank lines.
          Hide
          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
          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
          Jason Fowler added a comment -

          Good to go Adrian

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

          Thanks for the review Jason.

          Submitting for integration.

          Show
          Adrian Greeve added a comment - Thanks for the review Jason. Submitting for integration.
          Hide
          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
          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
          Adrian Greeve added a comment -

          Thanks Dan,

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

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

          Thanks Adrian, i've integrated this now

          Show
          Dan Poltawski added a comment - Thanks Adrian, i've integrated this now
          Hide
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

          (not really)

          Closing, thanks!

          Show
          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: