Moodle
  1. Moodle
  2. MDL-27901

admin_externalpage constructor should check the name is unique

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.4.1
    • Fix Version/s: 2.5
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide

      Test steps

      1. Duplicate the line $ADMIN->add() in report/security/settings.php
      2. Login as an admin
      3. Make sure you see a notice about the duplicated entry
      4. Make sure the duplicate does not appear in the navigation
      Show
      Test steps Duplicate the line $ADMIN->add() in report/security/settings.php Login as an admin Make sure you see a notice about the duplicated entry Make sure the duplicate does not appear in the navigation
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-27901-master
    • Rank:
      17550

      Description

      Instantiating this class (say in a plugin's settings.php) requires that the name is unique. Unfortunately, it doesn't check and you can add a non-unique name anyway.

      When the actual page is displayed and admin_externalpage_setup( 'name' ) is called, oddness can then ensue (typically a Section Error).

      It would save time if a check was made.

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this.

        I've put it on our backlog and we'll try to get to it as soon as we can.

        In the meantime adding more information, such as replication instructions, fix test instructions, a workaround or even a code solution, will help us and other users.

        Show
        Michael de Raadt added a comment - Thanks for reporting this. I've put it on our backlog and we'll try to get to it as soon as we can. In the meantime adding more information, such as replication instructions, fix test instructions, a workaround or even a code solution, will help us and other users.
        Hide
        Frédéric Massart added a comment -

        Here is a patch. Unfortunately, I don't think that admin_externalpage should know about the other objects and raise an error when one already exist. But we could consider admin_category::add() to make that check as it does for the categories. I am not entirely convinced that this issues has to be fixed, especially as the navigation system will raise a developer notice on duplicated entries.

        If this is not an overkill for such a minor improvement, then it gets my +0.5. If we agree that the developer can use longer and more descriptive admin pages, then this issue gets my -1.

        (Will backport to 2.2, 2.3 if agreed)

        Show
        Frédéric Massart added a comment - Here is a patch. Unfortunately, I don't think that admin_externalpage should know about the other objects and raise an error when one already exist. But we could consider admin_category::add() to make that check as it does for the categories. I am not entirely convinced that this issues has to be fixed, especially as the navigation system will raise a developer notice on duplicated entries. If this is not an overkill for such a minor improvement, then it gets my +0.5. If we agree that the developer can use longer and more descriptive admin pages, then this issue gets my -1. (Will backport to 2.2, 2.3 if agreed)
        Hide
        Rajesh Taneja added a comment -

        Hello Fred,

        Patch looks good, but seems to be an overkill for normal user as well.

        If we plan to add this then it might be nice to have developer debug mode check before locating node.

        else if (!empty($CFG->debug) && ($CFG->debug >= DEBUG_DEVELOPER) && !is_null($this->locate($something->name))) {
        
        Show
        Rajesh Taneja added a comment - Hello Fred, Patch looks good, but seems to be an overkill for normal user as well. If we plan to add this then it might be nice to have developer debug mode check before locating node. else if (!empty($CFG->debug) && ($CFG->debug >= DEBUG_DEVELOPER) && !is_null($this->locate($something->name))) {
        Hide
        Frédéric Massart added a comment -

        Pushing for peer review again, I'll let the reviewers decide of the future of this issue. Thanks!

        Show
        Frédéric Massart added a comment - Pushing for peer review again, I'll let the reviewers decide of the future of this issue. Thanks!
        Hide
        Rajesh Taneja added a comment -

        I have added integrators to this issue to get more feedback.
        IMO, we should improve docs to educate developers, then try detect there coding errors.

        Show
        Rajesh Taneja added a comment - I have added integrators to this issue to get more feedback. IMO, we should improve docs to educate developers, then try detect there coding errors.
        Hide
        Rajesh Taneja added a comment - - edited

        Jumping the process to get feedback from integrators.

        Show
        Rajesh Taneja added a comment - - edited Jumping the process to get feedback from integrators.
        Hide
        Dan Poltawski added a comment -

        In principle, adding the duplicate check seems sensible to me. But Freds reservations also make sense - but I don't think I fully understand the problem domain, perhaps you could explain in more detail the pros and cons.

        1. Please don't 'jump the process'. That is not the purpose of intergration review (and sorry, I know we are crap and bad at responding)
        2. This code, executes a different path (returns false), when debugging is enabled. This isn't great, its fine to not do the check and only output debug with debugging switched on, but executing different codepaths could lead to bugs when debugging is enabled or not.
           } else if ($CFG->debug >= DEBUG_NORMAL && !is_null($this->locate($something->name))) {
           	debugging('Duplicate admin page name: ' . $something->name);
           	return false;
          }
          
        3. The 'moodle pattern' for '$CFG->debug >= DEBUG_NORMAL' would be to do:
          if (debugging('', DEBUG_NORMAL)) {
          

          If you don't believe me, git grep for:

          git grep "debugging('',"
          
        4. Surely this should be DEBUG_DEVELOPER. Only a developer can fix it, right?
        Show
        Dan Poltawski added a comment - In principle, adding the duplicate check seems sensible to me. But Freds reservations also make sense - but I don't think I fully understand the problem domain, perhaps you could explain in more detail the pros and cons. Please don't 'jump the process'. That is not the purpose of intergration review (and sorry, I know we are crap and bad at responding) This code, executes a different path (returns false), when debugging is enabled. This isn't great, its fine to not do the check and only output debug with debugging switched on, but executing different codepaths could lead to bugs when debugging is enabled or not. } else if ($CFG->debug >= DEBUG_NORMAL && !is_null($ this ->locate($something->name))) { debugging('Duplicate admin page name: ' . $something->name); return false ; } The 'moodle pattern' for '$CFG->debug >= DEBUG_NORMAL' would be to do: if (debugging('', DEBUG_NORMAL)) { If you don't believe me, git grep for: git grep "debugging(''," Surely this should be DEBUG_DEVELOPER. Only a developer can fix it, right?
        Hide
        Frédéric Massart added a comment -

        Hi Dan,

        sorry for jumping the process, I often think that it's the best way to get a final feedback, but I understand this is not how the work flow should be.
        Thanks for pointing out the way to check the debug level. I had already seen this call in some other places, but it sounded a bit obscure to me.

        So, why is the logic different when debugging is on?

        • To display a debugging message;
        • To prevent the check for existing node to be performed all the time.

        You are right, the logic should not differ for developers (that could really be confusing), so the alternatives are:

        1. Performing the check for existing node regardless of the debug level;
          • Returning false;
          • OR not returning false;
        2. Only performing the check when minimal debug level is met, and just displaying a debugging message without altering the logic.

        My personal choice goes for the second one. Performing the check on all debug levels would eventually mean doing that on each page load (not considering caching) which we don't want to do. I don't think we should handling developers' mistakes/inattention here. And that even if the newly created node would point to the wrong place.

        I think the first solution, if better, should not be implemented as it's a bit of an overkill comparing to the benefit of having it there.

        I have updated my patch to simply display a message to the developer, pushing it for peer review again.

        Show
        Frédéric Massart added a comment - Hi Dan, sorry for jumping the process, I often think that it's the best way to get a final feedback, but I understand this is not how the work flow should be. Thanks for pointing out the way to check the debug level. I had already seen this call in some other places, but it sounded a bit obscure to me. So, why is the logic different when debugging is on? To display a debugging message; To prevent the check for existing node to be performed all the time. You are right, the logic should not differ for developers (that could really be confusing), so the alternatives are: Performing the check for existing node regardless of the debug level; Returning false; OR not returning false; Only performing the check when minimal debug level is met, and just displaying a debugging message without altering the logic. My personal choice goes for the second one. Performing the check on all debug levels would eventually mean doing that on each page load (not considering caching) which we don't want to do. I don't think we should handling developers' mistakes/inattention here. And that even if the newly created node would point to the wrong place. I think the first solution, if better, should not be implemented as it's a bit of an overkill comparing to the benefit of having it there. I have updated my patch to simply display a message to the developer, pushing it for peer review again.
        Hide
        Rajesh Taneja added a comment -

        Thanks Fred,
        Patch looks good, feel free to push it for integration.

        FYI: I personally would not pass first parameter as it should take default value from api.

        Show
        Rajesh Taneja added a comment - Thanks Fred, Patch looks good, feel free to push it for integration. FYI: I personally would not pass first parameter as it should take default value from api.
        Hide
        Frédéric Massart added a comment -

        Thanks Raj, I think I need to send the first parameter in that case otherwise I can't set the second argument which is not the default one either. Pushing for integration now.

        Show
        Frédéric Massart added a comment - Thanks Raj, I think I need to send the first parameter in that case otherwise I can't set the second argument which is not the default one either. Pushing for integration now.
        Hide
        Rajesh Taneja added a comment -

        Aha my bad...
        Yeah you are right Fred.

        Show
        Rajesh Taneja added a comment - Aha my bad... Yeah you are right Fred.
        Hide
        Dan Poltawski added a comment -

        Looks perfect now, but adding integration_held since its a master only issue.

        Show
        Dan Poltawski added a comment - Looks perfect now, but adding integration_held since its a master only issue.
        Hide
        Dan Poltawski added a comment -

        Integrated, thanks Fred

        Show
        Dan Poltawski added a comment - Integrated, thanks Fred
        Hide
        Rossiani Wijaya added a comment -

        This works as expected.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This works as expected. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        And your fantastic code has met core, hope they become good friends for a long period.

        Closing, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: