Moodle
  1. Moodle
  2. MDL-37048

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

    Details

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

      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"))) {                       
      

        Activity

        Hide
        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 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 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 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 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 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
        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
        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 Agarwal added a comment -

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

        Show
        Ankit Agarwal added a comment - Thanks Rosie for the quick review. Submitting for integration. Thanks
        Hide
        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
        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
        Sam Hemelryk added a comment -

        Thanks Ankit, this has been integrated now.

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

        Passing this test, as no testing is required.

        Show
        Rajesh Taneja added a comment - Passing this test, as no testing is required.
        Hide
        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
        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: