Moodle
  1. Moodle
  2. MDL-30940

multiselect mform element default and returned value are not working as expected

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.3.6, 2.4.3
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide
      1. Find some multiple select fields `git grep setMultiple`, AND some normal selects dropdowns
      2. Confirm that they work as expected
        • Data saved
        • Defaults are correct
        • Defaults don't override data submitted
      3. Download the attached file test_form_data.php
      4. Play with the form settings
      5. Confirm that
        • You can set default values on the single select and multiple select
        • The submitted value is selected whatever default value has been set
        • When none of the elements of the multiple select are selected, nothing is selected
      Show
      Find some multiple select fields `git grep setMultiple`, AND some normal selects dropdowns Confirm that they work as expected Data saved Defaults are correct Defaults don't override data submitted Download the attached file test_form_data.php Play with the form settings Confirm that You can set default values on the single select and multiple select The submitted value is selected whatever default value has been set When none of the elements of the multiple select are selected, nothing is selected
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
      MDL-30940-master
    • Rank:
      33962

      Description

      Attached are two examples of tiny mform using multiselect
      In the first example (copy it to your moodle root (in $CFG->wwwroot)) defaults are not working.

      In the second... defaults are working at 50% but what is worse is that by taking each selection off (shift click) and submitting the form, defualts are still submitted.

      Please help me!!!!

      1. select_one.php
        1 kB
        Daniele Cordella
      2. select_two.php
        1 kB
        Daniele Cordella
      3. test_form_data.php
        1 kB
        Frédéric Massart

        Issue Links

          Activity

          Hide
          Daniele Cordella added a comment - - edited

          this issue has only one detail more than MDL-28077

          Show
          Daniele Cordella added a comment - - edited this issue has only one detail more than MDL-28077
          Hide
          Michael de Raadt added a comment -

          Thanks for investigating this.

          Show
          Michael de Raadt added a comment - Thanks for investigating this.
          Hide
          Daniele Cordella added a comment -

          may I ask why do anyone seems to care about this issue?

          Show
          Daniele Cordella added a comment - may I ask why do anyone seems to care about this issue?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Fred, following conversation @ HQ chat, I'm assigning this to you in case you can verify and polish these multiselect issues.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Fred, following conversation @ HQ chat, I'm assigning this to you in case you can verify and polish these multiselect issues. TIA and ciao
          Hide
          Frédéric Massart added a comment -

          Here a proposed solution. I have added a hidden field above the select field to force the browser to send a dummy data upon form submission. The data will be cleaned up as it's not part of the possible options of the select element.

          The object returned $form->get_submitted_data() won't contain any property for the empty select, but I decided to keep this behaviour as it has been the case for a long time, and forcing an empty array() to be added there could lead to more problems here and there (3rd party plugins also) than it would add value to the code.

          Show
          Frédéric Massart added a comment - Here a proposed solution. I have added a hidden field above the select field to force the browser to send a dummy data upon form submission. The data will be cleaned up as it's not part of the possible options of the select element. The object returned $form->get_submitted_data() won't contain any property for the empty select, but I decided to keep this behaviour as it has been the case for a long time, and forcing an empty array() to be added there could lead to more problems here and there (3rd party plugins also) than it would add value to the code.
          Hide
          Daniele Cordella added a comment -

          Thanks, thanks and thanks even more to you Frédéric and Eloy too.
          I still didn't test it but I am going to extensively use this mform element so I will repeatedly test this work in detail.

          Show
          Daniele Cordella added a comment - Thanks, thanks and thanks even more to you Frédéric and Eloy too. I still didn't test it but I am going to extensively use this mform element so I will repeatedly test this work in detail.
          Hide
          Daniele Cordella added a comment -

          I am not an official QA tester, and I did not follow your Testing Instruction
          BUT
          I included your solution into my local installation and it works like a charm!

          Show
          Daniele Cordella added a comment - I am not an official QA tester, and I did not follow your Testing Instruction BUT I included your solution into my local installation and it works like a charm!
          Hide
          Daniele Cordella added a comment -

          Ok, it is quite 6 hours I am playing with your solution with satisfaction!
          I am starting to believe your intervention is, as usual, a great work!

          In spite of this I still wonder how can I disable the 'animals' select upon the content of the 'colours' multiselect item.
          The set of disabledIf
          $mform->disabledIf('animals', 'colours', 'eq', '2');
          $mform->disabledIf('animals', 'colours', 'eq', '3'); does not work

          and the solution
          $mform->disabledIf('animals', 'colours', 'eq', array(2, 3)); does not work too

          Maybe, now that you still know all the multiselect mform item at hearth, it is a good time to fix this aspect too.
          If you agree, I add one more issue here in the tracker for this new problem.

          Show
          Daniele Cordella added a comment - Ok, it is quite 6 hours I am playing with your solution with satisfaction! I am starting to believe your intervention is, as usual, a great work! In spite of this I still wonder how can I disable the 'animals' select upon the content of the 'colours' multiselect item. The set of disabledIf $mform->disabledIf('animals', 'colours', 'eq', '2'); $mform->disabledIf('animals', 'colours', 'eq', '3'); does not work and the solution $mform->disabledIf('animals', 'colours', 'eq', array(2, 3)); does not work too Maybe, now that you still know all the multiselect mform item at hearth, it is a good time to fix this aspect too. If you agree, I add one more issue here in the tracker for this new problem.
          Hide
          Frédéric Massart added a comment -

          Hi Daniele,

          thanks for your feedback! I think you are making us realising how badly we support the multiple selects. There is half a solution that you might be able to make use of:

          $mform->disabledIf('animals', 'colours[]', 'eq', '2');
          

          That is a bit hacky, and that will only work if you only select one value in the multiple select "colours", but that's start.

          As you offered, I'd suggest you to raise an issue to extend the support of disableIf when mutiple values are selected, and removing the [] from the disabledIf call.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Daniele, thanks for your feedback! I think you are making us realising how badly we support the multiple selects. There is half a solution that you might be able to make use of: $mform->disabledIf('animals', 'colours[]', 'eq', '2'); That is a bit hacky, and that will only work if you only select one value in the multiple select "colours", but that's start. As you offered, I'd suggest you to raise an issue to extend the support of disableIf when mutiple values are selected, and removing the [] from the disabledIf call. Cheers, Fred
          Hide
          Daniele Cordella added a comment -

          Hi Frédéric,
          thanks for your partial solution.
          As of your suggestion I opened MDL-39280.

          Show
          Daniele Cordella added a comment - Hi Frédéric, thanks for your partial solution. As of your suggestion I opened MDL-39280 .
          Hide
          Daniele Cordella added a comment -

          I tested the patch following instructions from point 3 to point 5.
          I was missing the first two points.
          All the test I performed were successful.
          50% of the test has been done.

          Show
          Daniele Cordella added a comment - I tested the patch following instructions from point 3 to point 5. I was missing the first two points. All the test I performed were successful. 50% of the test has been done.
          Hide
          Frédéric Massart added a comment -

          Thanks Daniele, pushing this for integration!

          Show
          Frédéric Massart added a comment - Thanks Daniele, pushing this for integration!
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just 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
          Dan Poltawski added a comment - The main moodle.git repository has just 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
          Damyon Wiese added a comment -

          Thanks Fred,

          Notes:

          1. I did a quick web search and this is the same solution that rails uses for multi selects.
          2. I also tested in firefox, chrome and ie9 and they all submitted the data reliably.
          3. I con't see an easy way to add unit tests specifically for this - I also couldn't find examples in core where we were using multi-selects and setDefault (which could explain why we hadn't noticed this).
          4. I tested the mnet user profile page and the enrolment screens multi selects - but the more testing on this the better.

          No issues found so this is integrated to master, 24 and 23 branches.

          Thanks!

          Show
          Damyon Wiese added a comment - Thanks Fred, Notes: I did a quick web search and this is the same solution that rails uses for multi selects. I also tested in firefox, chrome and ie9 and they all submitted the data reliably. I con't see an easy way to add unit tests specifically for this - I also couldn't find examples in core where we were using multi-selects and setDefault (which could explain why we hadn't noticed this). I tested the mnet user profile page and the enrolment screens multi selects - but the more testing on this the better. No issues found so this is integrated to master, 24 and 23 branches. Thanks!
          Hide
          Frédéric Massart added a comment -

          (That is also the way CakePHP proceeds to enforce a value to be sent in some cases.)

          Show
          Frédéric Massart added a comment - (That is also the way CakePHP proceeds to enforce a value to be sent in some cases.)
          Hide
          Rossiani Wijaya added a comment -

          This is working as expected.

          Tested for 2.3, 2.4 and master.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working as expected. Tested for 2.3, 2.4 and master. Test passed.
          Hide
          Dan Poltawski added a comment -

          Thanks! You're changes are now spread to the world through this git and our source control repositories.

          No time to rest though, we've got days to make 2.5 the best yet!

          ciao

          Show
          Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: