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

Book module has call to "get_file_by_hash" that is missing sha1

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Book
    • Labels:

      Description

      mod/book/tool/importhtml/locallib.php
      Line: 57

              if ($file = $fs->get_file_by_hash("$context->id/mod_book/importhtmltemp/0/$chapterfile->pathname")) {                       
      

      Will never find the file because it is missing the sha1 function.

      Should be:

              if ($file = $fs->get_file_by_hash(sha1("$context->id/mod_book/importhtmltemp/0/$chapterfile->pathname"))) {                       
      

        Gliffy Diagrams

          Activity

          Hide
          ankit_frenz Ankit Agarwal added a comment -

          That section of code doesnt seem to being used anymore. That context is excuted only if $type = 0
          but in import_form.php we have

                          // '0'=>get_string('typeonefile', 'booktool_importhtml'),

          the whole thing is commented out. Seems like it is not supported anymore. I will dig little more to find out the issues related to this, if it is not supported, will cleanup the code to remove those clauses.
          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - That section of code doesnt seem to being used anymore. That context is excuted only if $type = 0 but in import_form.php we have // '0'=>get_string('typeonefile', 'booktool_importhtml'), the whole thing is commented out. Seems like it is not supported anymore. I will dig little more to find out the issues related to this, if it is not supported, will cleanup the code to remove those clauses. Thanks
          Hide
          ankit_frenz Ankit Agarwal added a comment -

          Looking at the commit 34660c5979863c9b45038853bdb356c66d6d60da, it looks like it was a proposed new feature to support a single file import. But was never actually enabled in the code.

          Petr, if you can share your views on this it will be great.
          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - Looking at the commit 34660c5979863c9b45038853bdb356c66d6d60da, it looks like it was a proposed new feature to support a single file import. But was never actually enabled in the code. Petr, if you can share your views on this it will be great. Thanks
          Hide
          ankit_frenz Ankit Agarwal added a comment -

          I have fixed the code as suggested in the issue, although this code is not used in present, it can be in future, so better have the correct code in place.
          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - I have fixed the code as suggested in the issue, although this code is not used in present, it can be in future, so better have the correct code in place. Thanks
          Hide
          rwijaya Rossiani Wijaya added a comment -

          Hi Ankit,

          The patch looks fine.

          [y] Syntax
          [-] Output
          [y] Whitespace
          [-] Language
          [-] Databases
          [-] Testing
          [-] Security
          [-] Documentation
          [y] Git
          [y] Sanity check

          Feel free to submit it for integration review.

          Show
          rwijaya Rossiani Wijaya added a comment - Hi Ankit, The patch looks fine. [y] Syntax [-] Output [y] Whitespace [-] Language [-] Databases [-] Testing [-] Security [-] Documentation [y] Git [y] Sanity check Feel free to submit it for integration review.
          Hide
          ankit_frenz Ankit Agarwal added a comment -

          Thanks Rosie for the quick review.
          Submitting for integration.
          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - Thanks Rosie for the quick review. Submitting for integration. Thanks
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Ankit, this has been integrated now.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Ankit, this has been integrated now.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Passing this test, as no testing is required.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Passing this test, as no testing is required.
          Hide
          poltawski Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          poltawski Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                11/Mar/13