Moodle
  1. Moodle
  2. MDL-30459

Better error message in webservice/upload.php, create file handling web service demo client, update doc

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Component/s: Files API, Web Services
    • Labels:

      Description

      a) Create File upload + File download web service demo client
      b) update the documentation: http://docs.moodle.org/dev/Web_services_files_handling
      c) webservice/upload.php should return better exception when filename already exist

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Jérôme Mouneyrac added a comment -

            We need to return better erro exception in webservice/upload.php script.

            Show
            Jérôme Mouneyrac added a comment - We need to return better erro exception in webservice/upload.php script.
            Hide
            Jérôme Mouneyrac added a comment -

            I wrote some piece of code to make the webservice/upload.php script automatically rename file if occurence is found:

            $renameoccurencewithdate = optional_param('renameoccurencewithdate', false, PARAM_BOOL); //avoid to extra call
             
            //Check if the file already exist
                $existingfile = $this->file_exists($file_record->contextid, $file_record->component, $file_record->filearea, 
                            $file_record->itemid, $file_record->filepath, $file_record->filename);
                if ($existingfile) {
                    //if allow automatic rename (avoid)
                    if ($renameoccurencewithdate) {
                        //get file name extension
                        $filenameinfo = pathinfo($file->filename);
                        //add a . to the extension
                        $filenameinfo['extension'] = empty($filenameinfo['extension'])?'':'.'.$filenameinfo['extension']; 
             
                        $file->filename = $filenameinfo['filename'] . '-' . userdate(time(), "%Y%m%d-%H%M%S") . $filenameinfo['extension'];
                    } else {
                        throw new moodle_exception('storedfileproblem', 'File already exist');
                    }
                }
            

            Show
            Jérôme Mouneyrac added a comment - I wrote some piece of code to make the webservice/upload.php script automatically rename file if occurence is found: $renameoccurencewithdate = optional_param('renameoccurencewithdate', false, PARAM_BOOL); //avoid to extra call   //Check if the file already exist $existingfile = $this->file_exists($file_record->contextid, $file_record->component, $file_record->filearea, $file_record->itemid, $file_record->filepath, $file_record->filename); if ($existingfile) { //if allow automatic rename (avoid) if ($renameoccurencewithdate) { //get file name extension $filenameinfo = pathinfo($file->filename); //add a . to the extension $filenameinfo['extension'] = empty($filenameinfo['extension'])?'':'.'.$filenameinfo['extension'];   $file->filename = $filenameinfo['filename'] . '-' . userdate(time(), "%Y%m%d-%H%M%S") . $filenameinfo['extension']; } else { throw new moodle_exception('storedfileproblem', 'File already exist'); } }
            Show
            Jérôme Mouneyrac added a comment - Demo clients are there: https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-HTTP-filehandling
            Hide
            Jérôme Mouneyrac added a comment - - edited
            Show
            Jérôme Mouneyrac added a comment - - edited Documentation updated: http://docs.moodle.org/dev/Web_services_files_handling
            Hide
            Jérôme Mouneyrac added a comment -

            Little improvement for upload script when filename already exist (avoid to display a DDL INSERT error)

            Show
            Jérôme Mouneyrac added a comment - Little improvement for upload script when filename already exist (avoid to display a DDL INSERT error)
            Hide
            Sam Hemelryk added a comment -

            Jerome - how necessary is this?
            I had just had pressed go to integrate this and then noticed a massive massive problem.

            If a user uploads 3 files, and number 2 fails, then number 1 has been successfully uploaded and the end client will never know as only the exception will stand.
            The code as it is cannot stay as far as I can see. We have two options:

            1. Revert this code and deal with it after release.
            2. Fix the issue very quickly in one of two fashions:
              1. All files are checked before creating them and if any single file exists return the exception (this way no files are created)
              2. Ignore files that already exist and simply don't return them in the result (the application will be smart enough to compare what was sent against what was received as a result)

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Jerome - how necessary is this? I had just had pressed go to integrate this and then noticed a massive massive problem. If a user uploads 3 files, and number 2 fails, then number 1 has been successfully uploaded and the end client will never know as only the exception will stand. The code as it is cannot stay as far as I can see. We have two options: Revert this code and deal with it after release. Fix the issue very quickly in one of two fashions: All files are checked before creating them and if any single file exists return the exception (this way no files are created) Ignore files that already exist and simply don't return them in the result (the application will be smart enough to compare what was sent against what was received as a result) Cheers Sam
            Hide
            Sam Hemelryk added a comment -

            I should add if you choose to fix it immediately then you will need to work off the integration branch as I had to clean up whitespace issues when integrating your changes.

            Show
            Sam Hemelryk added a comment - I should add if you choose to fix it immediately then you will need to work off the integration branch as I had to clean up whitespace issues when integrating your changes.
            Hide
            Jérôme Mouneyrac added a comment -

            I have a patch that return this (1st file fails, 2nd success, 3rd fails):

            [

            {"filename":"image_to_upload.jpg","errortype":"filenameexist","errormsg":"File name already exists: image_to_upload.jpg"}

            ,

            {"component":"user","contextid":"13","userid":"2","filearea":"private","filename":"image_to_upload2.jpg","filepath":"\/","itemid":0,"license":"allrightsreserved","author":"Admin User","source":""}

            ,

            {"filename":"image_to_upload3.jpg","errortype":"filenameexist","errormsg":"File name already exists: image_to_upload3.jpg"}

            ]

            Show
            Jérôme Mouneyrac added a comment - I have a patch that return this (1st file fails, 2nd success, 3rd fails): [ {"filename":"image_to_upload.jpg","errortype":"filenameexist","errormsg":"File name already exists: image_to_upload.jpg"} , {"component":"user","contextid":"13","userid":"2","filearea":"private","filename":"image_to_upload2.jpg","filepath":"\/","itemid":0,"license":"allrightsreserved","author":"Admin User","source":""} , {"filename":"image_to_upload3.jpg","errortype":"filenameexist","errormsg":"File name already exists: image_to_upload3.jpg"} ]
            Hide
            Sam Hemelryk added a comment -

            Thanks for getting a fix up promptly Jerome, it has been integrated now

            Show
            Sam Hemelryk added a comment - Thanks for getting a fix up promptly Jerome, it has been integrated now
            Hide
            Rossiani Wijaya added a comment -

            This is working great.

            Test passed.

            Show
            Rossiani Wijaya added a comment - This is working great. Test passed.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Sent upstream! Just in time for Moodle 2.2rc1 (if related), yay!

            Closing and big thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Sent upstream! Just in time for Moodle 2.2rc1 (if related), yay! Closing and big thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: