Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-32709 Feature request: move the Book module from contrib to the Moodle core
  3. MDL-33121

Review notes from book module integration review and make fixes as required



    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Book
    • Labels:
    • Testing Instructions:

      There's no visible changes on the front-end.

      #. add book resource
      #. create some chapters
      #. test the functionality of the book module and make sure nothing is broken.

      There's no visible changes on the front-end. #. add book resource #. create some chapters #. test the functionality of the book module and make sure nothing is broken.
    • Affected Branches:
    • Fixed Branches:
    • Pull from Repository:
    • Pull Master Branch:


      During my integration review I noted several things about the new book module which require thought/fixing/clarification.
      As none of them were blockers it was integrated however we should still have a look over this list and act upon each point accordingly:

      Requiring fixes/changes:

      • README.md needs updating to reflect its integration into core. Not urgent but before release probably (defintely "no more developement planned" ).
      • The opening line of the boilerplate for each file is inconsistent. Some say part of Moodle, others Book module for Moodle, and others again Book plugin for Moodle.
      • $PAGE->set_title calls format_string internally (aweful) but mean no need to wrap things in format_string when setting page title[several files affected].
      • $PAGE->set_heading is the same as above.
      • booktool_***_extend_settings_navigation no need to check modname !== book that has happened within the calling function book_extend_settings_navigation. (Lots of unused globals in those functions as well).
      • book_preload_chapters: next and prev values are never set correctly. Not a bug as they are also not used presently (but should be fixed or removed none the less).
      • tool/exportimscp/index.php unused lang string vars.

      Requiring clarification:

      • I couldn't find or determine a use for book_log function in locallib.php
      • PARAM_RAW for the chapter title, is that correct will we allow HTML + friends there?
      • Is there a need to add mod_book as a body class. You can be sure the body class path-mod-book has been added for all pages within the book module.
      • I couldn't find the use of the chapterscount string. If it is used should that have a {$a}.

      Improvements, usability, accessibility, ideas:

      • It would be nice if when editing the first chapter (such as when you've just created the activity) if the would freeze the "subchapter" option and add a note about it being frozen because the first page cannot be a subchapter. Just a smidge better than ignoring it for friendliness.
      • book_get_toc would be great to convert output to use either a renderer preferably or at least standard output mechanics.
      • Simple observation renderers arn't being used, it would be great if we could convert things to make use of a renderer, simple as it may be.




            rwijaya Rossiani Wijaya
            samhemelryk Sam Hemelryk
            Peer reviewer:
            Sam Hemelryk
            Dan Poltawski
            Tim Barker
            Component watchers:
            David Jones, Adrian Greeve, Jake Dallimore, Mathew May, Mihail Geshoski, Peter Dias, Sujith Haridasan
            0 Vote for this issue
            2 Start watching this issue


              Fix Release Date: