Moodle
  1. Moodle
  2. MDL-33322

Unable to download recent file in file picker with JS turned off

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Filepicker
    • Labels:
    • Testing Instructions:
      Hide
      1. Turn off browser Javascript
      2. Add file resource a course
      3. In "select files" section, add a file from 'recent files'
      4. Choose 'download'

      Make sure there's no error display.

      Show
      Turn off browser Javascript Add file resource a course In "select files" section, add a file from 'recent files' Choose 'download' Make sure there's no error display.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      41173

      Description

      Replication steps:

      1. Be sure to have JS turned off at browser and site/user level
      2. Go to course and add "resource > file"
      3. In "Select files" section choose "Add", then "Recent files"
      4. Choose a file listed and click "Select"
      5. select the download button, the following error message occurs:
      Notice: Undefined property: stdClass::$contextid in /moodle/lib/filestorage/file_storage.php on line 953

        Issue Links

          Activity

          Hide
          Marina Glancy added a comment -

          Rossie, I have just received another bug MDL-33335. Can you take a look please, it seems like the similar problem.

          Show
          Marina Glancy added a comment - Rossie, I have just received another bug MDL-33335 . Can you take a look please, it seems like the similar problem.
          Hide
          Rossiani Wijaya added a comment -

          post patch.

          Please make sure MDL-33196 patch has been applied before using this patch.

          Sending for peer review. Ankit?

          Show
          Rossiani Wijaya added a comment - post patch. Please make sure MDL-33196 patch has been applied before using this patch. Sending for peer review. Ankit?
          Hide
          Aparup Banerjee added a comment -

          looks good.
          -replication instructions could be rephrased into proper test instructions.

          • if you're generous, you could update (repository/filepicker.php) this old style
            $user_context = get_context_instance(CONTEXT_USER, $USER->id);

            to the class based call.

          Show
          Aparup Banerjee added a comment - looks good. -replication instructions could be rephrased into proper test instructions. if you're generous, you could update (repository/filepicker.php) this old style $user_context = get_context_instance(CONTEXT_USER, $USER->id); to the class based call.
          Hide
          Marina Glancy added a comment - - edited

          Hi Rossie,
          there was a change in the content of files.source field. It became a serialised object now. Apparently DS forgot to put those changes in non-js filepicker.
          In your patch please remove changes in lib/filelib.php and use in repository/filepicker.php:

          $record->source = serialize((object)array('source' => $thefile['url']));
          

          as it is done in repository/repository_ajax.php.
          Also please set all the other fields as well as repository_ajax.php does it: timecreated, timemodified, userid

          Show
          Marina Glancy added a comment - - edited Hi Rossie, there was a change in the content of files.source field. It became a serialised object now. Apparently DS forgot to put those changes in non-js filepicker. In your patch please remove changes in lib/filelib.php and use in repository/filepicker.php: $record->source = serialize((object)array('source' => $thefile['url'])); as it is done in repository/repository_ajax.php. Also please set all the other fields as well as repository_ajax.php does it: timecreated, timemodified, userid
          Hide
          Marina Glancy added a comment -

          Rossie,
          regarding your comment that when file is picked from recent files, the size is 0:

          I looked at the code and it appears that it calls the wrong function and picking files from any moodle repository (server files, course legacy files, private files and recent files) should not work. And also it does not work in 2.2 either.
          So, can you create another issue please (I did not check in 2.1, most likely it does not work there either).

          As for this issue, can you please add the following, so the strict standards are followed:

          
          diff --git a/repository/draftfiles_manager.php b/repository/draftfiles_manager.php
          index b3f1dde..0bbc3a3 100644
          --- a/repository/draftfiles_manager.php
          +++ b/repository/draftfiles_manager.php
          @@ -333,7 +333,8 @@ default:
           
                           $home_url->param('action', 'browse');
                           $home_url->param('draftpath', $file->filepath);
          -                $foldername = trim(array_pop(explode('/', trim($file->filepath, '/'))), '/');
          +                $filepathchunks = explode('/', trim($file->filepath, '/'));
          +                $foldername = trim(array_pop($filepathchunks), '/');
                           echo html_writer::link($home_url, $foldername);
           
                           $home_url->param('draftpath', $file->filepath);
          
          Show
          Marina Glancy added a comment - Rossie, regarding your comment that when file is picked from recent files, the size is 0: I looked at the code and it appears that it calls the wrong function and picking files from any moodle repository (server files, course legacy files, private files and recent files) should not work. And also it does not work in 2.2 either. So, can you create another issue please (I did not check in 2.1, most likely it does not work there either). As for this issue, can you please add the following, so the strict standards are followed: diff --git a/repository/draftfiles_manager.php b/repository/draftfiles_manager.php index b3f1dde..0bbc3a3 100644 --- a/repository/draftfiles_manager.php +++ b/repository/draftfiles_manager.php @@ -333,7 +333,8 @@ default : $home_url->param('action', 'browse'); $home_url->param('draftpath', $file->filepath); - $foldername = trim(array_pop(explode('/', trim($file->filepath, '/'))), '/'); + $filepathchunks = explode('/', trim($file->filepath, '/')); + $foldername = trim(array_pop($filepathchunks), '/'); echo html_writer::link($home_url, $foldername); $home_url->param('draftpath', $file->filepath);
          Hide
          Rossiani Wijaya added a comment -

          Hi Marina,

          Thank you for your feedback and patch.

          Note:
          There's a regression issue for using/downloading file from Moodle's repositories (local, user, recent, course files). In order to avoid a complex files conflicts, the proper fix to use file from these repositories, will be address on MDL-33473.

          Thanks.

          Show
          Rossiani Wijaya added a comment - Hi Marina, Thank you for your feedback and patch. Note: There's a regression issue for using/downloading file from Moodle's repositories (local, user, recent, course files). In order to avoid a complex files conflicts, the proper fix to use file from these repositories, will be address on MDL-33473 . Thanks.
          Hide
          Aparup Banerjee added a comment -

          its great! sending up for integration

          ps: i've turned my strict standards back on now.

          Show
          Aparup Banerjee added a comment - its great! sending up for integration ps: i've turned my strict standards back on now.
          Hide
          Dan Poltawski added a comment -

          I'm not entirely clear about this source field thing. Linking MDL-33513

          Show
          Dan Poltawski added a comment - I'm not entirely clear about this source field thing. Linking MDL-33513
          Hide
          Dan Poltawski added a comment -

          I've integrated this now. As noted the source field is really confusing - but that is addressed in the other issue.

          Show
          Dan Poltawski added a comment - I've integrated this now. As noted the source field is really confusing - but that is addressed in the other issue.
          Hide
          Dan Poltawski added a comment -

          Tested and passed, thanks!

          Show
          Dan Poltawski added a comment - Tested and passed, thanks!
          Hide
          Martin Dougiamas added a comment -

          Yep this is OK for now (fixing the serialised thing) but in MDL-33513 we are actually going to get rid of the serialised thing everywhere.

          I'll make a note there that this line here that serialises the source field should be UNDONE again.

          Show
          Martin Dougiamas added a comment - Yep this is OK for now (fixing the serialised thing) but in MDL-33513 we are actually going to get rid of the serialised thing everywhere. I'll make a note there that this line here that serialises the source field should be UNDONE again.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

          Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

          Many thanks for your collaboration!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: