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

Passing attributes to advcheckbox form object triggers "undefined index" notice.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.2, 2.8.1, 2.9.4, 3.0.2, 3.1
    • Fix Version/s: 2.9.5, 3.0.3
    • Component/s: Forms Library
    • Labels:

      Description

      On or about line 47 of /lib/form/advcheckbox.php is the line:

      <code>
      if (!is_null($attributes['group'])) {
      <code>

      If the attributes array doesn't contain the 'group' key, an undefined index notice will be triggered. The following line should make the notice go away:

      <code>
      if (!is_null($attributes) && array_key_exists('group', $attributes)) {
      <code>

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting that issue and providing a fix.

            It looks like you won!

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting that issue and providing a fix. It looks like you won!
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Hello. I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

            For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

            If you have any information about this issue or a possible fix please post it here

            Show
            rajeshtaneja Rajesh Taneja added a comment - Hello. I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment If you have any information about this issue or a possible fix please post it here
            Hide
            marina Marina Glancy added a comment - - edited

            This is still an issue in 2.8, it's just nowhere in Moodle we use advcheckbox with attributes others than 'group' so we don't face it.

            To test apply this and open "Edit profile" form:

            --- a/user/editadvanced_form.php
            +++ b/user/editadvanced_form.php
            @@ -101,7 +101,7 @@ class user_editadvanced_form extends moodleform {
                     $mform->addElement('selectgroups', 'auth', get_string('chooseauthmethod', 'auth'), $authoptions);
                     $mform->addHelpButton('auth', 'chooseauthmethod', 'auth');
             
            -        $mform->addElement('advcheckbox', 'suspended', get_string('suspended', 'auth'));
            +        $mform->addElement('advcheckbox', 'suspended', get_string('suspended', 'auth'), null, array('class' => 'myclass'));
                     $mform->addHelpButton('suspended', 'suspended', 'auth');
             
                     $mform->addElement('checkbox', 'createpassword', get_string('createpassword', 'auth'));
            

            Show
            marina Marina Glancy added a comment - - edited This is still an issue in 2.8, it's just nowhere in Moodle we use advcheckbox with attributes others than 'group' so we don't face it. To test apply this and open "Edit profile" form: --- a/user/editadvanced_form.php +++ b/user/editadvanced_form.php @@ -101,7 +101,7 @@ class user_editadvanced_form extends moodleform { $mform->addElement('selectgroups', 'auth', get_string('chooseauthmethod', 'auth'), $authoptions); $mform->addHelpButton('auth', 'chooseauthmethod', 'auth'); - $mform->addElement('advcheckbox', 'suspended', get_string('suspended', 'auth')); + $mform->addElement('advcheckbox', 'suspended', get_string('suspended', 'auth'), null, array('class' => 'myclass')); $mform->addHelpButton('suspended', 'suspended', 'auth'); $mform->addElement('checkbox', 'createpassword', get_string('createpassword', 'auth'));
            Hide
            rushi963 Rushikesh Nalla added a comment -

            Hey I have been working on this and here is the github compare URL - https://github.com/moodle/moodle/compare/master...rushi963:MDL-29817 .I have attached the patch as well so please have a look at it and submit it for "peer review". One way of testing it is the one mentioned by Marina Glancy so that can be taken as the testing instructions.

            Thanks.

            Show
            rushi963 Rushikesh Nalla added a comment - Hey I have been working on this and here is the github compare URL - https://github.com/moodle/moodle/compare/master...rushi963:MDL-29817 .I have attached the patch as well so please have a look at it and submit it for "peer review". One way of testing it is the one mentioned by Marina Glancy so that can be taken as the testing instructions. Thanks.
            Hide
            marina Marina Glancy added a comment -

            Thank you, submitting for peer review for you.
            Can you please provide the testing instructions?

            Show
            marina Marina Glancy added a comment - Thank you, submitting for peer review for you. Can you please provide the testing instructions?
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-29817 using repository: https://github.com/rushi963/moodle

            • Testing instructions are missing.
            • master (0 errors / 0 warnings) [branch: MDL-29817 | CI Job]

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-29817 using repository: https://github.com/rushi963/moodle Testing instructions are missing. master (0 errors / 0 warnings) [branch: MDL-29817 | CI Job ] More information about this report
            Hide
            lameze Simey Lameze added a comment -

            Hi Rushikesh Nalla, thanks for work on this issue!

            Is there any reason for using:

            if (!is_null($attributes) && array_key_exists('group', $attributes)) {
            

            instead of

            if (!empty($attributes['group'])) {
            

            I've did a testing here, doing my suggested change above, and it works exactly the same way. So I guess we can use that, makes the code simple and we just call one function, instead of two. Could you please change that?

            I think the testing Marina suggested is good enough for this change, so I will use that as testing instructions.

            Thanks.

            Show
            lameze Simey Lameze added a comment - Hi Rushikesh Nalla , thanks for work on this issue! Is there any reason for using: if (!is_null($attributes) && array_key_exists('group', $attributes)) { instead of if (!empty($attributes['group'])) { I've did a testing here, doing my suggested change above, and it works exactly the same way. So I guess we can use that, makes the code simple and we just call one function, instead of two. Could you please change that? I think the testing Marina suggested is good enough for this change, so I will use that as testing instructions. Thanks.
            Hide
            rushi963 Rushikesh Nalla added a comment -

            Thanks for a better fix, definitely calling one function is better. I have made the necessary changes. Have a look and yes that can be used as the testing instructions.

            Thanks.

            Show
            rushi963 Rushikesh Nalla added a comment - Thanks for a better fix, definitely calling one function is better. I have made the necessary changes. Have a look and yes that can be used as the testing instructions. Thanks.
            Hide
            lameze Simey Lameze added a comment -

            Looks much better now Rushikesh Nalla, I wonder if you could provide branches for 3.0 and 2.9. As this is a bug, we need to backport it to stable branches.

            Please let me know if you need some assistance with that.

            Thanks.

            Show
            lameze Simey Lameze added a comment - Looks much better now Rushikesh Nalla , I wonder if you could provide branches for 3.0 and 2.9. As this is a bug, we need to backport it to stable branches. Please let me know if you need some assistance with that. Thanks.
            Hide
            rushi963 Rushikesh Nalla added a comment -

            Yes I need some help, do I have to download other stable versions (3.0 and 2.9) and then create branches out of them?

            Thanks.

            Show
            rushi963 Rushikesh Nalla added a comment - Yes I need some help, do I have to download other stable versions (3.0 and 2.9) and then create branches out of them? Thanks.
            Hide
            lameze Simey Lameze added a comment -

            Hi Rushikesh Nalla, yep, download those stables and create branches from them.

            Thanks.

            Show
            lameze Simey Lameze added a comment - Hi Rushikesh Nalla , yep, download those stables and create branches from them. Thanks.
            Hide
            rushi963 Rushikesh Nalla added a comment -

            Thanks SImey, I have created the branches for 2.9 and 3.0 and added them as well. Do i have to do anything else?

            Thanks.

            Show
            rushi963 Rushikesh Nalla added a comment - Thanks SImey, I have created the branches for 2.9 and 3.0 and added them as well. Do i have to do anything else? Thanks.
            Hide
            lameze Simey Lameze added a comment -

            Hi Rushikesh Nalla, thanks for creating branches for stables, well done!

            I had a look on the patch and it looks good now. I'm pushing for integration review.

            Thanks.

            Show
            lameze Simey Lameze added a comment - Hi Rushikesh Nalla , thanks for creating branches for stables, well done! I had a look on the patch and it looks good now. I'm pushing for integration review. Thanks.
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-29817 using repository: https://github.com/rushi963/moodle

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-29817 using repository: https://github.com/rushi963/moodle Testing instructions are missing. MOODLE_29_STABLE (0 errors / 0 warnings) [branch: MDL-29817_29 | CI Job ] MOODLE_30_STABLE (0 errors / 0 warnings) [branch: MDL-29817_30 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-29817 | CI Job ] More information about this report
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-29817 using repository: https://github.com/rushi963/moodle

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-29817 using repository: https://github.com/rushi963/moodle MOODLE_29_STABLE (0 errors / 0 warnings) [branch: MDL-29817_29 | CI Job ] MOODLE_30_STABLE (0 errors / 0 warnings) [branch: MDL-29817_30 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-29817 | CI Job ] More information about this report
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (29, 30 and master), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (29, 30 and master), thanks!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Verified in all branches while integrating it, so passing, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Verified in all branches while integrating it, so passing, thanks!
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

            Walking on water and developing software from a specification are easy if both are frozen.
            — Edward V Berard

            Show
            poltawski Dan Poltawski added a comment - Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org. Walking on water and developing software from a specification are easy if both are frozen. — Edward V Berard

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Mar/16