Description

Initialising a new instance of secondary navigation can cause navigation node intersects. It should be entirely possible to instantiate a new navigation view at any point in code; since (as per the "Navigation views" section in the parent epic) these are supposed to be entirely independent collections of nodes. However, when actually doing so, debugging messages will display complaining that children that already exist are being re-added to nodes. To see this, first turn debugging mode on. Then add the following script to your site root:

<?php

require_once('config.php');

use core\context\course;

use core\navigation\views\secondary;

class my_secondary extends secondary {

    protected function get_default_course_mapping(): array {

        $defaultmaping = parent::get_default_course_mapping();

        $defaultmapping['settings'][self::TYPE_SETTING]['filtermanagement'] = 10;

        $defaultmapping['settings'][self::TYPE_SETTING]['unenrolself'] = 9;

        return $defaultmaping;

    }

}

$PAGE->set_context(course::instance(2));

$PAGE->set_url('/whatever');

echo $OUTPUT->header();

$mysecondary = new my_secondary($PAGE);

$mysecondary->initialise();

echo $OUTPUT->footer();

Then just browse to http://yourmoodle/test.php.

What this should do, is construct an entirely new navigation tree, with the filtermanagement and unenrolself nodes in a different order to the original secondary navigation view (note it will not actually change the ordering of visible secondary navigation in the UI, because we don't add this new secondary navigation tree to the $PAGE object at any point). But instead we see that Moodle thinks that some of the nodes already have the children we wish to add - but this shouldn't happen because a navigation view is supposed to be its own separate collection of nodes.

Analysis

NB: A file, intersect.zip, has been attached to this issue with contains a dump of the navigation tree state when the example script above is viewed. Please see the "Supporting utilities" section of the parent epic to understand how to interact with these files.

1. To see why the intersects happen, start the navigation node player script using the files from intersect.zip
2. Press and hold the right arrow, you'll see the global navigation tree start populating
3. At frame 61, the settings navigation tree will start populating
4. At frame 249 secondary navigation starts to populate, this is the point where it should start grabbing nodes from the existing trees and making a new tree
5. Up until frame 252 everything is fine, you can see that editsettings and participants have been added, and they have unique ids (hint: use 'h' to highlight their ids and verify this is true)
   • Side note: you can start to see something is wrong already at this point, because their ncid's are the same ncids as nodes in the original trees
6. Press the right arrow again to get to frame 253
7. The review node was added as a child node of participants, and it also modified the original global navigation tree because the participants node in the global navigation tree shares the same node collection as the one in secondary navigation this voilates the original requirements as laid out in the "Navigation overhaul" section of the parent epic
8. At frame 274, the secondary navigation view has finished populating and global navigation has been thoroughly mutated, which it should not have been
9. The next bunch of frames deal with node activation and aren't relevant here, you can skip to frame 698 where our new secondary navigation view is constructed
10. Slowly step through until frame 703 where you will see it's about to add review to participants - have a look at the ids in the debugging message (the line like: navigation_node::add_node> Adding review...)
11. What's happened is the new secondary navigation view from our script (which is not printed in the output of the navigation node player) is about to add review to a new participants node, but that node also has the same node collection as the one in the original secondary navigation view and in global navigation, so it effectively adds the node to those trees again, press the right arrow and at frame 705 you'll now see two review nodes in global navigation and secondary navigation - hence the intersect error

The code responsible for constructing the new trees was added in MDL-70198 and it seems like the requirement that views should be their own independent thing and not mess with the other navigation trees was not properly understood, and there was never a test written for it. Looking at the code as it is today, and it's clear why it doesn't work. There are two main issues:

1. A simple clone is not enough, as the cloned nodes will keep the same reference to the child node collection
2. When it looks for parent nodes, the node it will add the child to is the original node from the source tree (the array of nodes passed in is an array of the original nodes we wish to reorder)

We put together a "toy" example on 3v4l to demonstrate the issue with the naive clone approach. You can see that using a simple clone is not enough, and messing with "Node D" affects "Node B".

This is how we originally realised there were issues in the navigation API. We created our own secondary navigation view in a custom theme, then used $PAGE->set_secondarynav assuming it would become the new secondary navigation view for all pages. However we found in certain situations we would see the navigation node intersect. This is because there are a handful of pages in Moodle that call $PAGE->secondarynav BEFORE calling $OUTPUT->header(). For example, backup/restore.php. This means that the initial call to $PAGE->secondarynav in backup/restore.php is using core's secondary navigation view, then when $OUTPUT->header() is called, our custom secondary navigation would be initialised and run in to the same navigation node intersect issue as the test script.