Moodle
  1. Moodle
  2. MDL-30168

Horrible mess with formslib and generated ids

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.14, 2.0.5, 2.1.2
    • Fix Version/s: 2.1.4, 2.2.1
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      1. Test the edit question forms to verify that the problems described in the issue Description are gone.

      2. Test a variety of other forms in Moodle, to verify that they are not broken.

      3. Run the unit tests in lib/simpletest/testformslib.php

      Show
      1. Test the edit question forms to verify that the problems described in the issue Description are gone. 2. Test a variety of other forms in Moodle, to verify that they are not broken. 3. Run the unit tests in lib/simpletest/testformslib.php
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30168_tim2
    • Rank:
      26052

      Description

      To see the symptoms of the problem, try this:

      1. Go to the question bank, and create a match question.
      2. Scroll down to the Hints section of the form.
      3. Click on the label "Clear incorrect responses" next to Hint 2.
      4. Observe that the checkbox in Hint 1 toggles, not the one in Hint 2. (I am testing this on Firefox, in case it matters.)

      For comparison, create a numerical question. Note that in the repeated 'Answers' section of the form, note that clicking on all the labels work as inspected. A bit of poking around with Firebug should let you see that the problem is to do with generated ids.

      Looking in git history, this seems to have been a problem ever since we started using HTML quickforms.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment - - edited

          There are several problems here:

          1. The implementation of the _generateId function is bad.

          The original formslib code in the base class is slighly odd (hint, if you are using microtime to generate what you think is a unique id, you are deluded) but will actually work, because of the static $idx.

              function _generateId()
              {
                  static $idx = 1;
          
                  if (!$this->getAttribute('id')) {
                      $this->updateAttributes(array('id' => 'qf_' . substr(md5(microtime() . $idx++), 0, 6)));
                  }
              } // end func _generateId
          

          The problem is that some Moodle person decided that they did not like the qf_ prefix, and wanted to used id_ instead. They 'achieved' this by copying and pasting the _generateId function into all subclasses. The problem with that is that there are now lots of different static $idx counters, so we are highly likely to generate equal ids.

          The correct solution is to parent::generateId() (which uses the one canonical counter) and then change the qf to id_. I have implemented this.

          2. The other problem was that (adv)checkbox and radio were calling _generateId in the constructor. _generateId is supposed to supply an id if one has not been provided elsewhere. Therefore, we should call it as late as possible, namely in the toHTML method.

          Note that (adv)checkbox and radio do need to call _generateId somewhere, because they assume that there is an id when outputting the <label>.

          In particular, the call to _generateId in the constructor was breaking (adv)checkbox and radio used in repeat_elements.

          moodleform::repeat_elements was relying on code in MoodleQuickForm_Renderer::renderElement (in lib/formslib.php) that builds a nice id based on the name, if the id has not already been set. Since repeat_elements clones the repeated elements, the fact that a generated id was being set in the constructor was causing all the repeated checkboxes to have the same id.

          Show
          Tim Hunt added a comment - - edited There are several problems here: 1. The implementation of the _generateId function is bad. The original formslib code in the base class is slighly odd (hint, if you are using microtime to generate what you think is a unique id, you are deluded) but will actually work, because of the static $idx. function _generateId() { static $idx = 1; if (!$ this ->getAttribute('id')) { $ this ->updateAttributes(array('id' => 'qf_' . substr(md5(microtime() . $idx++), 0, 6))); } } // end func _generateId The problem is that some Moodle person decided that they did not like the qf_ prefix, and wanted to used id_ instead. They 'achieved' this by copying and pasting the _generateId function into all subclasses. The problem with that is that there are now lots of different static $idx counters, so we are highly likely to generate equal ids. The correct solution is to parent:: generateId() (which uses the one canonical counter) and then change the qf to id_. I have implemented this. 2. The other problem was that (adv)checkbox and radio were calling _generateId in the constructor. _generateId is supposed to supply an id if one has not been provided elsewhere. Therefore, we should call it as late as possible, namely in the toHTML method. Note that (adv)checkbox and radio do need to call _generateId somewhere, because they assume that there is an id when outputting the <label>. In particular, the call to _generateId in the constructor was breaking (adv)checkbox and radio used in repeat_elements. moodleform::repeat_elements was relying on code in MoodleQuickForm_Renderer::renderElement (in lib/formslib.php) that builds a nice id based on the name, if the id has not already been set. Since repeat_elements clones the repeated elements, the fact that a generated id was being set in the constructor was causing all the repeated checkboxes to have the same id.
          Hide
          Tim Hunt added a comment -

          Grrr! so, Problem 1 is purely theoretical because PHP is so bloody slow, it takes more than one microsecond to call microtime, so that is a sufficient unique-id generator, at least on Linux. I think on Windows there may still be problems. Anyway, relying on it is crappy, so perhaps we should apply my changes anyway.

          Show
          Tim Hunt added a comment - Grrr! so, Problem 1 is purely theoretical because PHP is so bloody slow, it takes more than one microsecond to call microtime, so that is a sufficient unique-id generator, at least on Linux. I think on Windows there may still be problems. Anyway, relying on it is crappy, so perhaps we should apply my changes anyway.
          Hide
          Tim Hunt added a comment -

          So, we have to decide how to handle this. These is potentially risky changes. My suggestion would be:

          1. get this peer-reviewed and integrated before the Moodle 2.2. QA cycle starts.

          2. fix any problems during 2.2 QA cycle (I don't expect any problems.)

          3. After the 2.2 release, back-port this to Moodle 2.x.

          Show
          Tim Hunt added a comment - So, we have to decide how to handle this. These is potentially risky changes. My suggestion would be: 1. get this peer-reviewed and integrated before the Moodle 2.2. QA cycle starts. 2. fix any problems during 2.2 QA cycle (I don't expect any problems.) 3. After the 2.2 release, back-port this to Moodle 2.x.
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this, Tim.

          I'll see if we can find you a peer reviewer during today's Scum.

          Show
          Michael de Raadt added a comment - Thanks for working on this, Tim. I'll see if we can find you a peer reviewer during today's Scum.
          Hide
          Rajesh Taneja added a comment -

          Hello Tim,

          I looked at the code and it seems the main problem you faced is because of _generateId in checkbox constructor. Moving that in tohtml will solve the issue.

          As per re-organizing generateId and static idx, I would personally prefer to remove all the _generateId and leave it in base class (HTML_QuickForm_element) and replace qf with id_. In my understanding this is safe to do as moodleform expects id to start with id_ and strips qf_ in renderElement.

          I have created a branch for your reference which performs the similar job https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-30168

          FYI:
          Only concern in moving _generateid from constructor to tohtml is the duplication of id's for checkbox and radio buttons. This can be seen in your patch as well, by creating a wiki and check id of wiki format radio buttons.

          Show
          Rajesh Taneja added a comment - Hello Tim, I looked at the code and it seems the main problem you faced is because of _generateId in checkbox constructor. Moving that in tohtml will solve the issue. As per re-organizing generateId and static idx, I would personally prefer to remove all the _generateId and leave it in base class (HTML_QuickForm_element) and replace qf with id_. In my understanding this is safe to do as moodleform expects id to start with id_ and strips qf_ in renderElement. I have created a branch for your reference which performs the similar job https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-30168 FYI: Only concern in moving _generateid from constructor to tohtml is the duplication of id's for checkbox and radio buttons. This can be seen in your patch as well, by creating a wiki and check id of wiki format radio buttons.
          Hide
          Tim Hunt added a comment -

          You branch seems to have only made some of the https://github.com/timhunt/moodle/commit/919c3560ae828892a52194ea3f771ee209b66559, and split them between the two different commits.

          Although we can hack core formslib code, surely it is better to make Moodle-specific changes where we can, since we already have our own subclasses. I know the duplication of the _generateId is bloodly ugly, so I quite like your approach too. You really need to discuss it with one of the senior HQ developers.

          I am not really familiar with the wiki module. I assume that you are talking about the radio buttons on mod/wiki/create.php, that you see after selecting Add activity... -> Wiki on the course page, and then sumbitting that form. For me, those three radio buttons do not have duplicate ids (using latest master, not this patch.)

          Show
          Tim Hunt added a comment - You branch seems to have only made some of the https://github.com/timhunt/moodle/commit/919c3560ae828892a52194ea3f771ee209b66559 , and split them between the two different commits. Although we can hack core formslib code, surely it is better to make Moodle-specific changes where we can, since we already have our own subclasses. I know the duplication of the _generateId is bloodly ugly, so I quite like your approach too. You really need to discuss it with one of the senior HQ developers. I am not really familiar with the wiki module. I assume that you are talking about the radio buttons on mod/wiki/create.php, that you see after selecting Add activity... -> Wiki on the course page, and then sumbitting that form. For me, those three radio buttons do not have duplicate ids (using latest master, not this patch.)
          Hide
          Rajesh Taneja added a comment -

          Hello Tim,
          Your second commit is actually resolving the real issue mentioned in this bug, but causing duplicate id issue.
          Yes, I am taking about radio buttons on mod/wiki/create.php. Have you tried it with your patch? Moving generateId away from constructor is duplicating radio button id's on mod/wiki/create.php. I am still looking at how this can be resolved and will post updates ASAP

          Show
          Rajesh Taneja added a comment - Hello Tim, Your second commit is actually resolving the real issue mentioned in this bug, but causing duplicate id issue. Yes, I am taking about radio buttons on mod/wiki/create.php. Have you tried it with your patch? Moving generateId away from constructor is duplicating radio button id's on mod/wiki/create.php. I am still looking at how this can be resolved and will post updates ASAP
          Hide
          Rajesh Taneja added a comment - - edited

          Hello Tim,

          I saw this again and came up with https://github.com/rajeshtaneja/moodle/commit/6090903d4a745a65eb1a8ab11c7533101faa7187 solution. As id for checkbox and radio gets generated in the constructor, we need them to be updated when renaming the elements. Hence this solution is very safe to use and does not touch critical part of forms library.

          As per cleaning of the forms library, I will prefer to go with https://github.com/rajeshtaneja/moodle/commit/b1131f3a6433bf95ef793b46be2a890fc28dddec, which can go in master and can be tested during QA for regression.

          Show
          Rajesh Taneja added a comment - - edited Hello Tim, I saw this again and came up with https://github.com/rajeshtaneja/moodle/commit/6090903d4a745a65eb1a8ab11c7533101faa7187 solution. As id for checkbox and radio gets generated in the constructor, we need them to be updated when renaming the elements. Hence this solution is very safe to use and does not touch critical part of forms library. As per cleaning of the forms library, I will prefer to go with https://github.com/rajeshtaneja/moodle/commit/b1131f3a6433bf95ef793b46be2a890fc28dddec , which can go in master and can be tested during QA for regression.
          Hide
          Tim Hunt added a comment -

          2. Re cleaning forms library, have you discussed this with one of the HQ lead developers, as I suggested above? Actually, since the generated ids are meaningless (other than the fact that they are unique) why do we even care if they start qf_ or id_? I wonder what the people who originally wrote this code were thinking of. Anway, why not just use the original _generateId implementation?

          1. Re when to call _generateId. Your solution is very un-safe. Assigning a probably-wrong ID in the constructor, and then overriding it later, is just stupid. What happens if some code calls getId in the mean time? The correct behaviour is to lasy-initialise the id when it is necessary.

          The problem with radio buttons stems from the fact that I was only testing with a form with checkboxes, and I was not thinking properly. The problem with radio buttons is (of course) that all the buttons in a radio-group have the same name. So, for radio buttons, generating id from name does not work. So, really, it is the code in formslib.php renderElement method that needs to change in some way to take account of radio buttons. (Perhaps it should call a method on the element, rather than doing the work inline?)

          Show
          Tim Hunt added a comment - 2. Re cleaning forms library, have you discussed this with one of the HQ lead developers, as I suggested above? Actually, since the generated ids are meaningless (other than the fact that they are unique) why do we even care if they start qf_ or id_? I wonder what the people who originally wrote this code were thinking of. Anway, why not just use the original _generateId implementation? 1. Re when to call _generateId. Your solution is very un-safe. Assigning a probably-wrong ID in the constructor, and then overriding it later, is just stupid. What happens if some code calls getId in the mean time? The correct behaviour is to lasy-initialise the id when it is necessary. The problem with radio buttons stems from the fact that I was only testing with a form with checkboxes, and I was not thinking properly. The problem with radio buttons is (of course) that all the buttons in a radio-group have the same name. So, for radio buttons, generating id from name does not work. So, really, it is the code in formslib.php renderElement method that needs to change in some way to take account of radio buttons. (Perhaps it should call a method on the element, rather than doing the work inline?)
          Hide
          Rajesh Taneja added a comment -

          Thanks Tim,
          Sorry I haven't talked to anyone yet about generateid, will try catch someone tomorrow.
          As per generateid in constructor, it is original html_quickform implementation. As per lazy initialization is concerned, we are looking at changing default behavior of quickform which might cause regression. Id in quickform is mostly used for JS validation and JS gets id while rendering the element, so I don't see any problem recreating id.

          FYI:
          If we are looking at solving id on matching question page then, it might be worth updating id while updating checkbox name.

          Show
          Rajesh Taneja added a comment - Thanks Tim, Sorry I haven't talked to anyone yet about generateid, will try catch someone tomorrow. As per generateid in constructor, it is original html_quickform implementation. As per lazy initialization is concerned, we are looking at changing default behavior of quickform which might cause regression. Id in quickform is mostly used for JS validation and JS gets id while rendering the element, so I don't see any problem recreating id. FYI: If we are looking at solving id on matching question page then, it might be worth updating id while updating checkbox name.
          Hide
          Rajesh Taneja added a comment - - edited

          Hello David,
          Can you please review my solution, for this issue.
          https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-30168

          Show
          Rajesh Taneja added a comment - - edited Hello David, Can you please review my solution, for this issue. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-30168
          Hide
          Tim Hunt added a comment -

          Rajesh, I think the problem with your approach is that it does not solve this problem:

          At the moment, all form controls get nice ids based on the name, except for checkbox, advcheckbox, and radiobutton.

          That 'except' is nasty. It means, for example, that you can use #id_name rules in CSS to style all other types of form field, but not checkboxes. My proposed fix tried to give us nice ids here.

          My fix works fine for checkboxes, because name is unique for those. The problem is radio buttons, because you get multiple onces with the same name.

          I think the nice solution would be: for radio buttons, use an id that is name_value, with appropriate cleaning. Of course, it would take a bit of work to implement that. Should we make the effort? I think we should.

          Show
          Tim Hunt added a comment - Rajesh, I think the problem with your approach is that it does not solve this problem: At the moment, all form controls get nice ids based on the name, except for checkbox, advcheckbox, and radiobutton. That 'except' is nasty. It means, for example, that you can use #id_name rules in CSS to style all other types of form field, but not checkboxes. My proposed fix tried to give us nice ids here. My fix works fine for checkboxes, because name is unique for those. The problem is radio buttons, because you get multiple onces with the same name. I think the nice solution would be: for radio buttons, use an id that is name_value, with appropriate cleaning. Of course, it would take a bit of work to implement that. Should we make the effort? I think we should.
          Hide
          Dan Poltawski added a comment -

          This may or my not be relevant (as i've not understood this issue properly), but just pointing out here that another 'unique id' is being used by the filepicker/manager form elements with:

          $client_id = uniqid();
          
          Show
          Dan Poltawski added a comment - This may or my not be relevant (as i've not understood this issue properly), but just pointing out here that another 'unique id' is being used by the filepicker/manager form elements with: $client_id = uniqid();
          Hide
          David Mudrak added a comment -

          Yup, I definitely second Tim here. Regenerating ids in setName() is conceptually very bad thing. And "id is not mostly used until JS comes to the stage" is just not an argument.

          Show
          David Mudrak added a comment - Yup, I definitely second Tim here. Regenerating ids in setName() is conceptually very bad thing. And "id is not mostly used until JS comes to the stage" is just not an argument.
          Hide
          David Mudrak added a comment -

          Dan, yes.

          substr(md5(microtime() . $idx++), 0, 6)
          

          is actually completely useless even when combined with the static counter. I would rather invest my money to Greece than rely on taking the first 6 characters of MD5 hash as anything unique. I would personally replace it with uniqid($prefix) where prefix includes static counter.

          Show
          David Mudrak added a comment - Dan, yes. substr(md5(microtime() . $idx++), 0, 6) is actually completely useless even when combined with the static counter. I would rather invest my money to Greece than rely on taking the first 6 characters of MD5 hash as anything unique. I would personally replace it with uniqid($prefix) where prefix includes static counter.
          Hide
          Tim Hunt added a comment -

          Actually David, 6 characters of an MD5 hash is 24 bits, is 16 million. Those are pretty good odds

          Show
          Tim Hunt added a comment - Actually David, 6 characters of an MD5 hash is 24 bits, is 16 million. Those are pretty good odds
          Hide
          Rajesh Taneja added a comment -

          Thanks for all the inputs, Tim and David.

          I agree that checkbox, advcheckbox and radio button doesn't get nice id. This is because they were getting id in constructor and at the time of rendering we were accepting those id's. Quickform doesn't generate id for any element except the above mentioned. Id's were being generated in formslib.php by renderElement.

          I have created another patch which will change id for duplicate elements and will keep nice id's for all. Can you please review this new patch for me.
          https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-30168-radio

          Show
          Rajesh Taneja added a comment - Thanks for all the inputs, Tim and David. I agree that checkbox, advcheckbox and radio button doesn't get nice id. This is because they were getting id in constructor and at the time of rendering we were accepting those id's. Quickform doesn't generate id for any element except the above mentioned. Id's were being generated in formslib.php by renderElement. I have created another patch which will change id for duplicate elements and will keep nice id's for all. Can you please review this new patch for me. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-30168-radio
          Hide
          Tim Hunt added a comment -

          If I am reading that correctly, then that is a terrible change. It will change all ids on all form elements (mostly by adding _1 at the end) which will break many themes.

          Show
          Tim Hunt added a comment - If I am reading that correctly, then that is a terrible change. It will change all ids on all form elements (mostly by adding _1 at the end) which will break many themes.
          Hide
          Rajesh Taneja added a comment -

          Hello Tim,
          This will only update id for duplicate elements, added to a form. We can restrict it for checkbox and radio elements to be on safer side.

          Show
          Rajesh Taneja added a comment - Hello Tim, This will only update id for duplicate elements, added to a form. We can restrict it for checkbox and radio elements to be on safer side.
          Hide
          Tim Hunt added a comment -

          Oh, I see. That was not clear from just the diff. You need to see more of the code.

          Show
          Tim Hunt added a comment - Oh, I see. That was not clear from just the diff. You need to see more of the code.
          Hide
          Tim Hunt added a comment -

          I have just come back to this, with a view to getting a final solution.

          To summarise the discussion so far, the best branch to date is https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-30168-radio

          This does several things:

          1. Change the unique id generation to just use the base class method, rather than overriding the method in a million different subclasses. This does require a change in 'upstread' formslib code, but we have hacked that code to death anyway, so this is probably the best approach. (This bit of the change saves many lines of code.)

          2. Stop generating any ids in constructors. They will be generated later, when needed.

          3. Add logic in HTML_QuickForm::addElement that tried to avoid duplicate names.

          This then lets the code at the top of MoodleQuickForm_Renderer::renderElement generate a nice id for all elements based on the element name.

          This code does work, according to our testing, at least. However, I don't quite understand why it works, since the element name changes in MoodleQuickForm_Renderer::renderElement happen after the changes in HTML_QuickForm::addElement which solve duplicate ids. Why don't they overwrite them.

          Ah, the logic in MoodleQuickForm_Renderer::renderElement is too complicated for its own good. It does different things depending on what the id is currently set to. Wow! that is really tricky to understand.

          Also, the MoodleQuickForm_Renderer::renderElement code is carefully writting to remove [] in the name when constructing the ids. The new HTML_QuickForm::addElement code does not do this, and so will generate bad ids when use with repeat-elements.

          I really think we should try again, to make a nicer fix.

          My proposal is:

          1. Re-implement HTML_QuickForm_element::_generateId to create the id according to our 'nice ids' algorithm in MoodleQuickForm_Renderer::renderElement.

          2. Do one override of this in HTML_QuickForm_radio so that the generated id for radio buttons is of the form name_value (cleaned up). I think that is nicer.

          3. Change MoodleQuickForm_Renderer::renderElement to just call the new _generateId to generate the ids we want, rather than putting id generation logic there.

          4. Remove the _generateId calls in constructors, and the unnecessary overrides.

          Once I see what that code looks like, I will submit it for peer review.

          Show
          Tim Hunt added a comment - I have just come back to this, with a view to getting a final solution. To summarise the discussion so far, the best branch to date is https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-30168-radio This does several things: 1. Change the unique id generation to just use the base class method, rather than overriding the method in a million different subclasses. This does require a change in 'upstread' formslib code, but we have hacked that code to death anyway, so this is probably the best approach. (This bit of the change saves many lines of code.) 2. Stop generating any ids in constructors. They will be generated later, when needed. 3. Add logic in HTML_QuickForm::addElement that tried to avoid duplicate names. This then lets the code at the top of MoodleQuickForm_Renderer::renderElement generate a nice id for all elements based on the element name. This code does work, according to our testing, at least. However, I don't quite understand why it works, since the element name changes in MoodleQuickForm_Renderer::renderElement happen after the changes in HTML_QuickForm::addElement which solve duplicate ids. Why don't they overwrite them. Ah, the logic in MoodleQuickForm_Renderer::renderElement is too complicated for its own good. It does different things depending on what the id is currently set to. Wow! that is really tricky to understand. Also, the MoodleQuickForm_Renderer::renderElement code is carefully writting to remove [] in the name when constructing the ids. The new HTML_QuickForm::addElement code does not do this, and so will generate bad ids when use with repeat-elements. I really think we should try again, to make a nicer fix. My proposal is: 1. Re-implement HTML_QuickForm_element::_generateId to create the id according to our 'nice ids' algorithm in MoodleQuickForm_Renderer::renderElement. 2. Do one override of this in HTML_QuickForm_radio so that the generated id for radio buttons is of the form name_value (cleaned up). I think that is nicer. 3. Change MoodleQuickForm_Renderer::renderElement to just call the new _generateId to generate the ids we want, rather than putting id generation logic there. 4. Remove the _generateId calls in constructors, and the unnecessary overrides. Once I see what that code looks like, I will submit it for peer review.
          Hide
          Tim Hunt added a comment -

          New proposed fix: https://github.com/timhunt/moodle/compare/master...MDL-30168_tim2

          Rajesh (or anyone else), please let me know what you think about this approach.

          Show
          Tim Hunt added a comment - New proposed fix: https://github.com/timhunt/moodle/compare/master...MDL-30168_tim2 Rajesh (or anyone else), please let me know what you think about this approach.
          Hide
          Rajesh Taneja added a comment -

          Patch looks spot-on to me Tim
          +1 from my side.

          Show
          Rajesh Taneja added a comment - Patch looks spot-on to me Tim +1 from my side.
          Hide
          Tim Hunt added a comment -

          Thanks Rajesh.

          Would anyone else like to offer an opinion before I submit this for integration?

          Show
          Tim Hunt added a comment - Thanks Rajesh. Would anyone else like to offer an opinion before I submit this for integration?
          Hide
          Sam Marshall added a comment -

          Patch looks good; are you allowed to edit the stuff in lib/pear? (I didn't read whole text of this bug, sorry if that was already discussed.)

          Show
          Sam Marshall added a comment - Patch looks good; are you allowed to edit the stuff in lib/pear? (I didn't read whole text of this bug, sorry if that was already discussed.)
          Hide
          Tim Hunt added a comment -

          Thanks sam. Yes, that is answered above. Generally we don't edit things in lib/pear, but quickforms has already been hacked to death, so it is an exception. Of course, this change does make it an even bigger exception, which is not great, but the new code is simpler.

          Two positive reviews is enough for me. Cherry-picking and submitting for integration.

          Show
          Tim Hunt added a comment - Thanks sam. Yes, that is answered above. Generally we don't edit things in lib/pear, but quickforms has already been hacked to death, so it is an exception. Of course, this change does make it an even bigger exception, which is not great, but the new code is simpler. Two positive reviews is enough for me. Cherry-picking and submitting for integration.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Sam Hemelryk added a comment -

          Hi Tim,

          Code looks good, I've been looking over this for the past while now and can't fault it
          I'll integrate this once the next integration cycle starts (later today or tomorrow hopefully).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Tim, Code looks good, I've been looking over this for the past while now and can't fault it I'll integrate this once the next integration cycle starts (later today or tomorrow hopefully). Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Integrated thanks Tim

          Show
          Sam Hemelryk added a comment - Integrated thanks Tim
          Hide
          Adrian Greeve added a comment -

          Tested in firefox on version 1.9, 2.0, 2.1 and 2.2
          I checked out the matching questions as well as multiple choice, random short answers, short answers. And then other forms: workshop editform.php (edit assessment form); lesson editpage.php (add a new page).

          I inspected the element using firefly and observed that the id was not identical with repeated elements.
          Nothing broke while interacting with different forms.

          Test passed.

          Show
          Adrian Greeve added a comment - Tested in firefox on version 1.9, 2.0, 2.1 and 2.2 I checked out the matching questions as well as multiple choice, random short answers, short answers. And then other forms: workshop editform.php (edit assessment form); lesson editpage.php (add a new page). I inspected the element using firefly and observed that the id was not identical with repeated elements. Nothing broke while interacting with different forms. Test passed.
          Hide
          Adrian Greeve added a comment -

          Oops, I forgot to mention that I ran unit tests on all branches besides 19 (testformslib.php doesn't exist) and they passed with no errors.

          Show
          Adrian Greeve added a comment - Oops, I forgot to mention that I ran unit tests on all branches besides 19 (testformslib.php doesn't exist) and they passed with no errors.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The git and cvs repositories are happy receiving your very first contribution to Moodle for 2012. Happy new year!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The git and cvs repositories are happy receiving your very first contribution to Moodle for 2012. Happy new year! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: