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

          Attachments

            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