Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-39801

navigationlib.php navigation_node_collection::remove fails

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.7, 2.4.4, 2.5, 2.6
    • Fix Version/s: 2.3.8, 2.4.5, 2.5.1
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide

      This can not be reproduced in core since function navigation_node::remove() is almost not used. It may occur when themes or other contributed plugins attempt to remove a navigation node.

      Review the commits and run unit tests.

      Show
      This can not be reproduced in core since function navigation_node::remove() is almost not used. It may occur when themes or other contributed plugins attempt to remove a navigation node. Review the commits and run unit tests.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-39801-master

      Description

      navigationlib.php:902 if ($node->key == $key && $node->type == $type)
      This test always returns true for the first element of the $this->collection
      Hard to find out why my theme was not removing the good item !

      Solution :
      if ($node->key === $key && $node->type === $type)
      Maybe the test is not ok also l.2561 and l.4397 .

      Thanks for your work.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            marina Marina Glancy added a comment -

            I have also noticed that after removing a node it is impossible to insert a child (not in the end). Second commit addresses this issue.

            TO INTEGRATORS: I'm submitting the master (== 2.5) fix only but would vote to backport the fix.

            Show
            marina Marina Glancy added a comment - I have also noticed that after removing a node it is impossible to insert a child (not in the end). Second commit addresses this issue. TO INTEGRATORS: I'm submitting the master (== 2.5) fix only but would vote to backport the fix.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (23, 24, 25 & master), good spot!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (23, 24, 25 & master), good spot!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Passing, the array_values() has logic and have tried navigationlib tests with and without the patch. Once applied they pass.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Passing, the array_values() has logic and have tried navigationlib tests with and without the patch. Once applied they pass.
            Hide
            poltawski Dan Poltawski added a comment -

            Feature: Thanks to our superb contributors
              In order to make Moodle better
              As an integrator
              I need to thank all our contributors
             
              Scenario: Dan thanks you all
                Given I log in as "dan"
                And I see "lots of fixed issues"
                When I follow "Close integrated issues"
                Then I should see "Lots of thanks to all our contributors"
            

            Your changes are upstream

            Show
            poltawski Dan Poltawski added a comment - Feature: Thanks to our superb contributors In order to make Moodle better As an integrator I need to thank all our contributors   Scenario: Dan thanks you all Given I log in as "dan" And I see "lots of fixed issues" When I follow "Close integrated issues" Then I should see "Lots of thanks to all our contributors" Your changes are upstream

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  8/Jul/13