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

          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: