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

          Attachments

            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