Moodle
  1. Moodle
  2. MDL-29147

QuickForm insertElementBefore shows a warning if there are multiple elements without a name

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.13, 2.0.4, 2.1.1
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      At the end of course/edit_form.php function 'definition()' add the following line:
      $mform->insertElementBefore($mform->createElement('static', 'testing', 'This is a test', 'The test text'), 'idnumber');

      The element appears as expected, but there is also the following error message:
      Notice: Undefined index: in [folder-removed]/lib/pear/HTML/QuickForm.php on line 679
      Warning: array_search() expects parameter 2 to be array, null given in[folder-removed]/lib/pear/HTML/QuickForm.php on line 679

      Show
      At the end of course/edit_form.php function 'definition()' add the following line: $mform->insertElementBefore($mform->createElement('static', 'testing', 'This is a test', 'The test text'), 'idnumber'); The element appears as expected, but there is also the following error message: Notice: Undefined index: in [folder-removed] /lib/pear/HTML/QuickForm.php on line 679 Warning: array_search() expects parameter 2 to be array, null given in [folder-removed] /lib/pear/HTML/QuickForm.php on line 679
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-29147_insertElementBefore_warning
    • Rank:
      18688

      Description

      When adding an element, the duplicates array is not updated if the element has no name.
      However, when using the 'insertElementBefore' function, it is assumed that all duplicate elements are contained in the duplicates array.

      I've attached the one-line patch.

      Note the test below uses the course edit form for convenience, as it is already set up, but this will affect any forms with multiple unnamed entries.

        Activity

        Hide
        Michael de Raadt added a comment -

        Hi, Davo.

        Thanks for reporting that and providing a solution.

        Show
        Michael de Raadt added a comment - Hi, Davo. Thanks for reporting that and providing a solution.
        Hide
        Dan Poltawski added a comment -

        Hi Davo,

        Is this still an issue? If so perhaps you could rebase and put up for peer review? Thanks

        Show
        Dan Poltawski added a comment - Hi Davo, Is this still an issue? If so perhaps you could rebase and put up for peer review? Thanks
        Hide
        Davo Smith added a comment -

        Just checked - certainly is still an issue and the 1-line fix is unchanged (although I've cherry-picked it into a new branch on top of master).

        Show
        Davo Smith added a comment - Just checked - certainly is still an issue and the 1-line fix is unchanged (although I've cherry-picked it into a new branch on top of master).
        Hide
        Dan Poltawski added a comment -

        Looks good to me.

        Show
        Dan Poltawski added a comment - Looks good to me.
        Hide
        Davo Smith added a comment -

        If all OK, could it be put forward for integration review?

        Show
        Davo Smith added a comment - If all OK, could it be put forward for integration review?
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Some hours ago...

        the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Some hours ago... the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Sam Hemelryk added a comment -

        Thanks Davo, this has been integrated now.
        I'd only been integrated on master, 22, and 21 though sorry. 20 and 19 branches are now only receiving security patches or patches that have been agreed upon and are considered necessary.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks Davo, this has been integrated now. I'd only been integrated on master, 22, and 21 though sorry. 20 and 19 branches are now only receiving security patches or patches that have been agreed upon and are considered necessary. Cheers Sam
        Hide
        Rossiani Wijaya added a comment -

        This is looking great.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This is looking great. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        FCT (fixed, closing, thanks). Ciao

        "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
        ~ Benjamin Disraeli

        Show
        Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli

          People

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

            Dates

            • Created:
              Updated:
              Resolved: