Moodle
  1. Moodle
  2. MDL-35655

Generalize navigation object assignment and renderer assignment in block navigation to allow more code reuse in specialization of the block

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.4
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide

      These a basic, backend changes. There should be no visible difference.

      1. Without logging in check the navigation block is displayed without errors.
      2. Log in as an administrator.
      3. Use the navigation block a little and just make sure it is still working as expected.
      Show
      These a basic, backend changes. There should be no visible difference. Without logging in check the navigation block is displayed without errors. Log in as an administrator. Use the navigation block a little and just make sure it is still working as expected.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      44390

      Description

      The global navigation object is generated in the get_content method of the navigation block. This could be extracted to a protected method get_method that could be overridden in specialized navigation block plugins to generate alternate navigation from the global navigation or from scratch. One example is a block to display in a course only the branch of the global navigation that pertains to that course.

          /**
           * Returns the navigation
           *
           * @return navigation_node The navigation object to display
           */
          protected function get_navigation() {
              // Initialise (only actually happens if it hasn't already been done yet
              $this->page->navigation->initialise();
              return clone($this->page->navigation);
          }
      

      The renderer of the navigation block is fetched by a string

      $renderer = $this->page->get_renderer('block_navigation');

      but could be fetched by reference to the block name

      $renderer = $this->page->get_renderer($this->blockname);

      to allow specialization of the block by redefining the renderer.

        Activity

        Hide
        Itamar Tzadok added a comment -

        Sam, could you please comment?

        Show
        Itamar Tzadok added a comment - Sam, could you please comment?
        Hide
        Sam Hemelryk added a comment -

        Hi Itamar,

        Having read through the description and looked at the proposed patch this gets a +1 from me.
        The end change isn't a big one, it's 100% safe, and obviously it's a useful change (as you're written the code yourself, thanks!).
        Feel free to put this up for integration, or if you're unable to let me know and I'll put it up for you

        Many thanks for both the issue and the patch.
        Sam

        Show
        Sam Hemelryk added a comment - Hi Itamar, Having read through the description and looked at the proposed patch this gets a +1 from me. The end change isn't a big one, it's 100% safe, and obviously it's a useful change (as you're written the code yourself, thanks!). Feel free to put this up for integration, or if you're unable to let me know and I'll put it up for you Many thanks for both the issue and the patch. Sam
        Hide
        Itamar Tzadok added a comment -

        Thanks Sam! Please send to integration for me (I should write the patch that fixes the workflow issue and allows me to send patches to integration ).

        Show
        Itamar Tzadok added a comment - Thanks Sam! Please send to integration for me (I should write the patch that fixes the workflow issue and allows me to send patches to integration ).
        Hide
        Sam Hemelryk added a comment -

        Cool thanks Itamar, I've tidied up the details in the issue and have put up for integration. Hopefully it'll be in master next week

        Show
        Sam Hemelryk added a comment - Cool thanks Itamar, I've tidied up the details in the issue and have put up for integration. Hopefully it'll be in master next week
        Hide
        Dan Poltawski 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
        Dan Poltawski 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
        Itamar Tzadok added a comment -

        I added a condition on the navigation object retrieved in get_content since specialized blocks may have nothing to show in which case get_content needs to return null. For the global navigation this condition is transparent because the the object must exist.

                // Get the navigation object or don't display the block if none provided.
                if (!$navigation = $this->get_navigation()) {
                    return null;
                }
        

        The change to the fix is minimal and hopefully could be reviewed in no time (sorry for the hassle).

        Show
        Itamar Tzadok added a comment - I added a condition on the navigation object retrieved in get_content since specialized blocks may have nothing to show in which case get_content needs to return null. For the global navigation this condition is transparent because the the object must exist. // Get the navigation object or don't display the block if none provided. if (!$navigation = $ this ->get_navigation()) { return null ; } The change to the fix is minimal and hopefully could be reviewed in no time (sorry for the hassle).
        Hide
        Sam Hemelryk added a comment -

        Looks good thanks Itamar, putting this back up for integration now.

        Show
        Sam Hemelryk added a comment - Looks good thanks Itamar, putting this back up for integration now.
        Hide
        Dan Poltawski added a comment -

        Integrated to master. Thanks Itamar!

        Show
        Dan Poltawski added a comment - Integrated to master. Thanks Itamar!
        Hide
        Rajesh Taneja added a comment -

        Thanks Itamar,

        Works well for me, no problem found in navigation.

        Show
        Rajesh Taneja added a comment - Thanks Itamar, Works well for me, no problem found in navigation.
        Hide
        Aparup Banerjee added a comment -

        Your issue has dug up some gold.
        It works great i've been told.
        Go forth, be brave, be bold.

        yay! "All your thoughts are belong to everyone."

        Thanks and ciao!

        Show
        Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!

          People

          • Votes:
            4 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: