Details

    • Testing Instructions:
      Hide
      1. There should not be any "label" associated with any of the "submit" buttons generated by mform
      2. This change effects all instances of mform "submit" buttons in Moodle. Please make sure you check as many instances as possible, a few are listed below:-
        -> Forgot password page
        -> Profile editing page
        -> Course editing page
        -> Blog add page (blog/edit.php?action=add)
        -> Note add pages
        -> Pretty much every page in moodle
      1. Just do a few activities on some of these pages to make sure there are no regressions.
      Show
      There should not be any "label" associated with any of the "submit" buttons generated by mform This change effects all instances of mform "submit" buttons in Moodle. Please make sure you check as many instances as possible, a few are listed below:- -> Forgot password page -> Profile editing page -> Course editing page -> Blog add page (blog/edit.php?action=add) -> Note add pages -> Pretty much every page in moodle Just do a few activities on some of these pages to make sure there are no regressions.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      MDL-30892-master2
    • Rank:
      33901

      Description

      The buttons on the forgot password page do not need labels. The button text is enough of a label.

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment - - edited

          Hi Greg,

          From what I can see this we have empty labels associated with the buttons. And I don't see any way of removing them without overwriting the Forms API. even the $element->SetLabel(''); creates an empty label.

          Am not sure we should really be changing the API for this. will try to get other people's opinion on this.

          Here is the dump of current html

          		
          		<div id="fitem_id_username" class="fitem fitem_ftext"><div class="fitemtitle"><label for="id_username">Username </label></div><div class="felement ftext"><input name="username" type="text" id="id_username" /></div></div>
          		<div id="fitem_id_submitbuttonusername" class="fitem fitem_fsubmit"><div class="fitemtitle"><label for="id_submitbuttonusername"> </label></div><div class="felement fsubmit"><input name="submitbuttonusername" value="Search" type="submit" id="id_submitbuttonusername" /></div></div>
          		</div>
          

          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Greg, From what I can see this we have empty labels associated with the buttons. And I don't see any way of removing them without overwriting the Forms API. even the $element->SetLabel(''); creates an empty label. Am not sure we should really be changing the API for this. will try to get other people's opinion on this. Here is the dump of current html <div id= "fitem_id_username" class= "fitem fitem_ftext" > <div class= "fitemtitle" > <label for= "id_username" > Username </label> </div> <div class= "felement ftext" > <input name= "username" type= "text" id= "id_username" /> </div> </div> <div id= "fitem_id_submitbuttonusername" class= "fitem fitem_fsubmit" > <div class= "fitemtitle" > <label for= "id_submitbuttonusername" > </label> </div> <div class= "felement fsubmit" > <input name= "submitbuttonusername" value= "Search" type= "submit" id= "id_submitbuttonusername" /> </div> </div> </div> Thanks
          Hide
          Greg Kraus added a comment -

          In the grand scheme of things, this is quite minor. It might be something you keep in mind at a future date if you look at redesigning some of the Forms API.

          Show
          Greg Kraus added a comment - In the grand scheme of things, this is quite minor. It might be something you keep in mind at a future date if you look at redesigning some of the Forms API.
          Hide
          Ankit Agarwal added a comment -

          Hi,
          I have made changes in the API and submitting this for review.
          If its decided to go forward with this change, it probabily should go only into master.
          Also if we decide to go with this change it might be a good idea to make the same changes to "button" as made to "Submit" in this patch.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi, I have made changes in the API and submitting this for review. If its decided to go forward with this change, it probabily should go only into master. Also if we decide to go with this change it might be a good idea to make the same changes to "button" as made to "Submit" in this patch. Thanks
          Hide
          Andrew Davis added a comment -

          I'm not an mforms expert so not entirely sure about the code change. You need testing instructions. Make sure to include using the forgot password page and other forms that also use this code. Just to check that this has no unanticipated side effects.

          Show
          Andrew Davis added a comment - I'm not an mforms expert so not entirely sure about the code change. You need testing instructions. Make sure to include using the forgot password page and other forms that also use this code. Just to check that this has no unanticipated side effects.
          Hide
          Rajesh Taneja added a comment - - edited

          Current patch, can lead to following problems:

          1. help for button will not be rendered
          2. Group (Advance group) may break
          3. returning value from fronzen is not correct.
            FYI:
            If you ever want to introduce new template, then your element should implement getElementTemplateType

          IMO: Solution should be implemented in HTML_QuickForm_Renderer_Tableless::renderElement, where actual label gets in html. You can also remove label in MoodleQuickForm_Renderer::renderElement. Check for label, req, advanceimg and help, if not exists and not group then remove label tag. This can be safely done on all elements (except group), as empty label doesn't have any significance.

          Show
          Rajesh Taneja added a comment - - edited Current patch, can lead to following problems: help for button will not be rendered Group (Advance group) may break returning value from fronzen is not correct. FYI: If you ever want to introduce new template, then your element should implement getElementTemplateType IMO: Solution should be implemented in HTML_QuickForm_Renderer_Tableless::renderElement, where actual label gets in html. You can also remove label in MoodleQuickForm_Renderer::renderElement. Check for label, req, advanceimg and help, if not exists and not group then remove label tag. This can be safely done on all elements (except group), as empty label doesn't have any significance.
          Hide
          Ankit Agarwal added a comment -

          updated the github url with an alternative solution as suggested by Raj.
          Just for reference previous solution is https://github.com/ankitagarwal/moodle/compare/MDL-30892-master

          Thanks

          Show
          Ankit Agarwal added a comment - updated the github url with an alternative solution as suggested by Raj. Just for reference previous solution is https://github.com/ankitagarwal/moodle/compare/MDL-30892-master Thanks
          Hide
          Rajesh Taneja added a comment -

          Patch looks good to me Ankit, although added Tim for his opinion.

          FYI: You might want to squash your branches, and component is formslib IMO (In commit message)

          Show
          Rajesh Taneja added a comment - Patch looks good to me Ankit, although added Tim for his opinion. FYI: You might want to squash your branches, and component is formslib IMO (In commit message)
          Hide
          Tim Hunt added a comment -

          The logic in the if statement looks OK, but that is a monster line of code. I think I would make a label_is_required method to hide all that. Either $this->label_is_required or $element->label_is_required.

          Using the preg_replace to get rid of the label is just ugly. Can't you deal with this at the place where the label tag is generated? Also, are you sure the preg_replace will never delete too much stuff? (Try a group of elements with labels between the buttons) This is really the sort of code that needs unit tests.

          Show
          Tim Hunt added a comment - The logic in the if statement looks OK, but that is a monster line of code. I think I would make a label_is_required method to hide all that. Either $this->label_is_required or $element->label_is_required. Using the preg_replace to get rid of the label is just ugly. Can't you deal with this at the place where the label tag is generated? Also, are you sure the preg_replace will never delete too much stuff? (Try a group of elements with labels between the buttons) This is really the sort of code that needs unit tests.
          Hide
          Ankit Agarwal added a comment -

          Thanks Tim for the feedback.
          If you look at https://github.com/ankitagarwal/moodle/compare/MDL-30892-master , that was an attempt to remove it when its generated, but unfortunately we cannot do that since we have to check more multiple things before we remove <label> tag (label, req,advanced img), hence we used this alternate approach.

          Also can you please explain the test case a little better? From my understanding we are doing it only on per element basis and it should not delete too much of stuff. if you can suggest some test cases, I would be more than happy to try it out.

          Also if we are removing the <label> tags that will leave us with empty <div class="fitemtitle"></div> tags, should we be messing with that as well or it is just better to leave that untouched?

          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Tim for the feedback. If you look at https://github.com/ankitagarwal/moodle/compare/MDL-30892-master , that was an attempt to remove it when its generated, but unfortunately we cannot do that since we have to check more multiple things before we remove <label> tag (label, req,advanced img), hence we used this alternate approach. Also can you please explain the test case a little better? From my understanding we are doing it only on per element basis and it should not delete too much of stuff. if you can suggest some test cases, I would be more than happy to try it out. Also if we are removing the <label> tags that will leave us with empty <div class="fitemtitle"></div> tags, should we be messing with that as well or it is just better to leave that untouched? Thanks
          Hide
          Tim Hunt added a comment -

          Well, my worry is that your regex does greedy matching, and I know some formslib constructs, like groups of elements, can have several <label> tags together. So, it is hard to convince oneself that your code will never misfire and delete too much, even though it works in every case you have tested.

          I can see why you were forced to put the code where it is now, but I think we are agreed it is not idea.

          Show
          Tim Hunt added a comment - Well, my worry is that your regex does greedy matching, and I know some formslib constructs, like groups of elements, can have several <label> tags together. So, it is hard to convince oneself that your code will never misfire and delete too much, even though it works in every case you have tested. I can see why you were forced to put the code where it is now, but I think we are agreed it is not idea.
          Hide
          Rajesh Taneja added a comment -

          I agree with Tim that, it's not ideal solution.
          Not sure, if we can achieve any ideal solution with Quickform design.

          @Ankit: You can try following to replace first label tag.

          $data = preg_replace("#<label>(.+?)</label>#is", '', $data, 1);
          
          Show
          Rajesh Taneja added a comment - I agree with Tim that, it's not ideal solution. Not sure, if we can achieve any ideal solution with Quickform design. @Ankit: You can try following to replace first label tag. $data = preg_replace( "#<label>(.+?)</label>#is" , '', $data, 1);
          Hide
          Ankit Agarwal added a comment - - edited

          Thanks Tim for the feedback.

          @Rajesh
          I already tried that. Here is the output of few things that I tried:-

          echo "<xmp>";
          $html = "asdasdas<dasd<label>xxxxxxxx</label>1111111111</label>22222222<label>3333333</label>444444444";
          echo $html;
          $html = preg_replace('$<label>.+?</label>$ism', '', $html,1);
          echo PHP_EOL;
          echo $html;
          $html = "asdasdas<dasd<label>xxxxxxxx</label>1111111111</label>22222222<label>3333333</label>444444444";
          $html = preg_replace('$<label>(.+?)</label>$ism', '', $html, 1);
          echo PHP_EOL;
          echo $html;
          $html = "asdasdas<dasd<label>xxxxxxxx</label>1111111111</label>22222222<label>3333333</label>444444444";
          $html = preg_replace("#<label>(.+?)</label>#is", '', $html, 1);
          echo PHP_EOL;
          echo $html;
          $html = "asdasdas<dasd<label>xxxxxxxx</label>1111111111</label>22222222<label>3333333</label>444444444";
          $html = preg_replace("/<label>.*?<\/label>/is", '', $html, 1);
          echo PHP_EOL;
          echo $html;
          $html = "asdasdas<dasd<label>xxxxxxxx</label>1111111111</label>22222222<label>3333333</label>444444444";
          $html = preg_replace("/<label>.*<\/label>/isU", '', $html, 1);
          echo PHP_EOL;
          echo $html;
          echo "</xmp>";
          

          Output

          asdasdas<dasd<label>xxxxxxxx</label>1111111111</label>22222222<label>3333333</label>444444444 
          asdasdas<dasd1111111111</label>22222222<label>3333333</label>444444444
          asdasdas<dasd1111111111</label>22222222<label>3333333</label>444444444
          asdasdas<dasd1111111111</label>22222222<label>3333333</label>444444444
          asdasdas<dasd1111111111</label>22222222<label>3333333</label>444444444
          asdasdas<dasd1111111111</label>22222222<label>3333333</label>444444444
          

          Looking at that the best one to go for is

          
          $html = preg_replace('$<label>(.+?)</label>$ism', '', $html,1);
          

          But the big question is, if we can have multiple "label" pairs in $html, how do we know which one to delete? because all above regex assumes that the first label pair that we encounter needs to be removed.

          Thanks

          Show
          Ankit Agarwal added a comment - - edited Thanks Tim for the feedback. @Rajesh I already tried that. Here is the output of few things that I tried:- echo "<xmp>" ; $html = "asdasdas<dasd<label>xxxxxxxx</label>1111111111</label>22222222<label>3333333</label>444444444" ; echo $html; $html = preg_replace('$<label>.+?</label>$ism', '', $html,1); echo PHP_EOL; echo $html; $html = "asdasdas<dasd<label>xxxxxxxx</label>1111111111</label>22222222<label>3333333</label>444444444" ; $html = preg_replace('$<label>(.+?)</label>$ism', '', $html, 1); echo PHP_EOL; echo $html; $html = "asdasdas<dasd<label>xxxxxxxx</label>1111111111</label>22222222<label>3333333</label>444444444" ; $html = preg_replace( "#<label>(.+?)</label>#is" , '', $html, 1); echo PHP_EOL; echo $html; $html = "asdasdas<dasd<label>xxxxxxxx</label>1111111111</label>22222222<label>3333333</label>444444444" ; $html = preg_replace( "/<label>.*?<\/label>/is" , '', $html, 1); echo PHP_EOL; echo $html; $html = "asdasdas<dasd<label>xxxxxxxx</label>1111111111</label>22222222<label>3333333</label>444444444" ; $html = preg_replace( "/<label>.*<\/label>/isU" , '', $html, 1); echo PHP_EOL; echo $html; echo "</xmp>" ; Output asdasdas<dasd<label>xxxxxxxx</label>1111111111</label>22222222<label>3333333</label>444444444 asdasdas<dasd1111111111</label>22222222<label>3333333</label>444444444 asdasdas<dasd1111111111</label>22222222<label>3333333</label>444444444 asdasdas<dasd1111111111</label>22222222<label>3333333</label>444444444 asdasdas<dasd1111111111</label>22222222<label>3333333</label>444444444 asdasdas<dasd1111111111</label>22222222<label>3333333</label>444444444 Looking at that the best one to go for is $html = preg_replace('$<label>(.+?)</label>$ism', '', $html,1); But the big question is, if we can have multiple "label" pairs in $html, how do we know which one to delete? because all above regex assumes that the first label pair that we encounter needs to be removed. Thanks
          Hide
          Tim Hunt added a comment -

          "... how do we know which one to delete?"

          That is exactly the kind of thing that you need to worry about if you are trying to fix this bug by using regular expressions. I don't know.

          Show
          Tim Hunt added a comment - "... how do we know which one to delete?" That is exactly the kind of thing that you need to worry about if you are trying to fix this bug by using regular expressions. I don't know.
          Hide
          Rajesh Taneja added a comment -

          AFAIK we are deleting string from defined template, so first label tag is what we are looking here.
          Also, looking at

          <label>{label}<!-- BEGIN required -->{req}<!-- END required -->{advancedimg} {help}</label>
          

          I don't see how we can have label tag within label tag, as this is done on one element at a time, it seems reasonable to delete first label.

          Show
          Rajesh Taneja added a comment - AFAIK we are deleting string from defined template, so first label tag is what we are looking here. Also, looking at <label>{label}<!-- BEGIN required -->{req}<!-- END required -->{advancedimg} {help}</label> I don't see how we can have label tag within label tag, as this is done on one element at a time, it seems reasonable to delete first label.
          Hide
          Ankit Agarwal added a comment -

          I had a talk with Tim and he is okay with the above approach. Rajesh/Jason suggested to use yet another approach of replacing "label" with "Span" tags. Here are the three possible solutions:-

          1. https://github.com/ankitagarwal/moodle/compare/MDL-30892-master
          2. https://github.com/ankitagarwal/moodle/compare/MDL-30892-master2
          3. https://github.com/ankitagarwal/moodle/compare/MDL-30892-master3

          feedback will be appreciated. For the moment I am tilted to go with the second solution
          thanks

          Show
          Ankit Agarwal added a comment - I had a talk with Tim and he is okay with the above approach. Rajesh/Jason suggested to use yet another approach of replacing "label" with "Span" tags. Here are the three possible solutions:- https://github.com/ankitagarwal/moodle/compare/MDL-30892-master https://github.com/ankitagarwal/moodle/compare/MDL-30892-master2 https://github.com/ankitagarwal/moodle/compare/MDL-30892-master3 feedback will be appreciated. For the moment I am tilted to go with the second solution thanks
          Hide
          Ankit Agarwal added a comment -

          Submitting this for integration.

          @integrators
          I have submitted the second solution for integration, if you think that any other better alternative is present, would love to have your feedback on this.
          Thanks

          Show
          Ankit Agarwal added a comment - Submitting this for integration. @integrators I have submitted the second solution for integration, if you think that any other better alternative is present, would love to have your feedback on this. Thanks
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated (yesterday) 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
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated (yesterday) 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 Ankit + everyone I am reopening this sorry.

          I dislike the hardcoding, serious checks, and regex. I also noted that while it removes the empty labels from individual elements it doesn't remove the empty label created by moodleform::add_action_buttons.

          I've had a quick look and I seriously think that the hardcoding has to go (I've already reopened another issue for the same reason Ankit and perhaps you will find the following idea some help in that).
          I also think that elements need to be responsible for the checks that go into deciding whether they have a label.
          I think the best solution is going to involve making use of the element template system published by the renderers.
          What about defining a new elementTemplate 'defaultnolabel', or 'minimum' or something (because there is useless HTML if you just remove the label) and then for each element you want to have use it perform the checks within the elements getElementTemplateType method.
          I gave this a quick test by just coping the default template and removing the label, then setting the submit element type to use defaultnolabel and everything appeared to work fine for individual elements.
          It didn't work for add_action_buttons as that is a group and groups use a standard group system, not entirely sure what the solution is there but certainly it now ties in with the other reopened issue which is about properly using labels for groups.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Ankit + everyone I am reopening this sorry. I dislike the hardcoding, serious checks, and regex. I also noted that while it removes the empty labels from individual elements it doesn't remove the empty label created by moodleform::add_action_buttons. I've had a quick look and I seriously think that the hardcoding has to go (I've already reopened another issue for the same reason Ankit and perhaps you will find the following idea some help in that). I also think that elements need to be responsible for the checks that go into deciding whether they have a label. I think the best solution is going to involve making use of the element template system published by the renderers. What about defining a new elementTemplate 'defaultnolabel', or 'minimum' or something (because there is useless HTML if you just remove the label) and then for each element you want to have use it perform the checks within the elements getElementTemplateType method. I gave this a quick test by just coping the default template and removing the label, then setting the submit element type to use defaultnolabel and everything appeared to work fine for individual elements. It didn't work for add_action_buttons as that is a group and groups use a standard group system, not entirely sure what the solution is there but certainly it now ties in with the other reopened issue which is about properly using labels for groups. Cheers Sam
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Sam,
          Thanks for the feedback.
          I agree that it is not an clean solution. But I already tried the other approach of new template as you can see here:-
          http://tracker.moodle.org/browse/MDL-30892?focusedCommentId=150996&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-150996

          Issue with that was we always had to remove the "label" and its counterparts (help button, advanced img etc)
          Let me know if that solutions appears a better candidate to you.

          moodleform::add_action_buttons calls moodleform::creatElement so the label still gets removed for "submit" buttons(not for normal button types)
          or are you referring to the empty group label created?

          So once a solution is accepted that we all agree on, I will create an issue to port the changes to the type "button".
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Sam, Thanks for the feedback. I agree that it is not an clean solution. But I already tried the other approach of new template as you can see here:- http://tracker.moodle.org/browse/MDL-30892?focusedCommentId=150996&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-150996 Issue with that was we always had to remove the "label" and its counterparts (help button, advanced img etc) Let me know if that solutions appears a better candidate to you. moodleform::add_action_buttons calls moodleform::creatElement so the label still gets removed for "submit" buttons(not for normal button types) or are you referring to the empty group label created? So once a solution is accepted that we all agree on, I will create an issue to port the changes to the type "button". Thanks
          Hide
          Sam Hemelryk added a comment -

          Hi Ankit,

          I'm going to work through this in the issue.
          Those checks relate to the element and its state. You have the following checks being performed at the moment:

          1. It's a submit button - well that's easy it has the getElementTemplateType callback (which is where you'd perform the checks likely)
          2. The label property is not empty - the property belongs to the element so easy as
          3. It's not a required element - I can't think of a use case where you'd ever make a submit button a required element. Even if it did I can't imagine where a submit button would use a label and being required.
          4. Whether it has a help button - Its looking for a property (which we know exists) and a method (which we know doesn't) both belonging to the element so no problems there.
          5. Whether is is a required element - This one is genuinely harder as there's no way for the element to know whether it is advanced or not. The element isn't informed and it doesn't have reference to any forms it may have been added to. I can also see how making a submit button advanced could be useful. Although I'm not entirely sure

          So really looking that list there are only two tests that we can't easily perform within the elements callback with the code as it is. Tests 3 and 5 whether an element is required, and whether it is advanced.
          Unfortunately unlike most callbacks already within QuickForms this callback doesn't provide the caller, or any potentially relevant information.

          In short I see the following three solutions:
          a) Take the current approach.
          b) Provide information to getElementTemplateType when calling it and use templates.
          c) Look for another callback and if it exists let it decide whether or not the label should be removed.

          In regards to the current approach [a] what I really don't like about it is that it is hardcoded, it has to meet all of the checks noted above in order to get through. Its good in that it solves the exact problem at hand and requires only small change. Also having a bit of a play I see that elements that extend submit button don't tend ot override the elements type (setType during construct) so other elements that immediately jump to mind as leading these checks to become unmaintainable are in fact going to benefit as well (think of the cancel button for example that also suffers from the same problem).
          Unfortunately other areas like radio buttons won't benefit from this example but could really be fixed in the same way. For example look at this form:

          <?php
          
          require_once('config.php');
          require_once($CFG->dirroot.'/lib/formslib.php');
          
          class test_form extends moodleform {
              function definition() {
                  $this->_form->addElement('header','heading', 'My form');
                  $this->_form->addElement('date_selector', 'startdate', 'Test select');
                  $this->_form->addElement('radio', 'radiobtn', 'Choose a format', 'Simple text', '1');
                  $this->_form->addElement('radio', 'radiobtn', '', 'Advanced text', '1');
                  $this->_form->addElement('submit', 'submit', 'Submit button');
                  $this->_form->addElement('cancel', 'cancel', 'Cancel button');
          
                  $this->add_action_buttons();
          
              }
          }
          $PAGE->set_url('/test.php');
          $PAGE->set_context(get_system_context());
          $PAGE->set_title('Test');
          $PAGE->set_heading('Test');
          $PAGE->set_pagelayout('embedded');
          
          $mform = new test_form();
          
          echo $OUTPUT->header();
          $mform->display();
          echo $OUTPUT->footer();
          

          In that example I used for testing I see that the second radio button for which I set an empty label still ends up with two labels. The first is like the button labels,. its empty and ideally it should be removed.
          As mentioned the only think I don't like is the hard coding. Pretty much just on principle because I know it can lead to complex, unmaintainable code every time someone finds another "exception" such as the radio button example noted.

          Next [b] passing more information to getElementTemplateType feels more correct to me in that other elements will have the same opportunities as the submit button. What I really don't like about this is that it is changing an existing API. But it is an option.
          Also worth noting it relies on the template existing, or the element having set the template it wants to use.

          Finally [c] introduce a new callback that gives the element the opportunity to decide. In this way you could inspect each element and if it has a method say "safeToRemoveEmptyLabel" and when called with all the relevant information (likely a reference to the renderer as well) if it returns true the existing bit of regex removes any empty labels. This would basically be replacing all of those checks with the callback and working on the answer it gives if it exists.
          To be truthful I don't like this solution, it is still munging HTML using regex and I do feel that the templates are a better place to solve this.

          Anyway in summary all three solutions have their flaws, really it is just a case of deciding.
          If everyone feels the first solution is better I am happy to integrate it, really I just want to see it discussed.
          The mforms version in Moodle is a tired old forms library, but I think replacing it (or upgrading it) is on the board any time soon so what ever we choose we have to live with.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Ankit, I'm going to work through this in the issue. Those checks relate to the element and its state. You have the following checks being performed at the moment: It's a submit button - well that's easy it has the getElementTemplateType callback (which is where you'd perform the checks likely) The label property is not empty - the property belongs to the element so easy as It's not a required element - I can't think of a use case where you'd ever make a submit button a required element. Even if it did I can't imagine where a submit button would use a label and being required. Whether it has a help button - Its looking for a property (which we know exists) and a method (which we know doesn't) both belonging to the element so no problems there. Whether is is a required element - This one is genuinely harder as there's no way for the element to know whether it is advanced or not. The element isn't informed and it doesn't have reference to any forms it may have been added to. I can also see how making a submit button advanced could be useful. Although I'm not entirely sure So really looking that list there are only two tests that we can't easily perform within the elements callback with the code as it is. Tests 3 and 5 whether an element is required, and whether it is advanced. Unfortunately unlike most callbacks already within QuickForms this callback doesn't provide the caller, or any potentially relevant information. In short I see the following three solutions: a) Take the current approach. b) Provide information to getElementTemplateType when calling it and use templates. c) Look for another callback and if it exists let it decide whether or not the label should be removed. In regards to the current approach [a] what I really don't like about it is that it is hardcoded, it has to meet all of the checks noted above in order to get through. Its good in that it solves the exact problem at hand and requires only small change. Also having a bit of a play I see that elements that extend submit button don't tend ot override the elements type (setType during construct) so other elements that immediately jump to mind as leading these checks to become unmaintainable are in fact going to benefit as well (think of the cancel button for example that also suffers from the same problem). Unfortunately other areas like radio buttons won't benefit from this example but could really be fixed in the same way. For example look at this form: <?php require_once('config.php'); require_once($CFG->dirroot.'/lib/formslib.php'); class test_form extends moodleform { function definition() { $ this ->_form->addElement('header','heading', 'My form'); $ this ->_form->addElement('date_selector', 'startdate', 'Test select'); $ this ->_form->addElement('radio', 'radiobtn', 'Choose a format', 'Simple text', '1'); $ this ->_form->addElement('radio', 'radiobtn', '', 'Advanced text', '1'); $ this ->_form->addElement('submit', 'submit', 'Submit button'); $ this ->_form->addElement('cancel', 'cancel', 'Cancel button'); $ this ->add_action_buttons(); } } $PAGE->set_url('/test.php'); $PAGE->set_context(get_system_context()); $PAGE->set_title('Test'); $PAGE->set_heading('Test'); $PAGE->set_pagelayout('embedded'); $mform = new test_form(); echo $OUTPUT->header(); $mform->display(); echo $OUTPUT->footer(); In that example I used for testing I see that the second radio button for which I set an empty label still ends up with two labels. The first is like the button labels,. its empty and ideally it should be removed. As mentioned the only think I don't like is the hard coding. Pretty much just on principle because I know it can lead to complex, unmaintainable code every time someone finds another "exception" such as the radio button example noted. Next [b] passing more information to getElementTemplateType feels more correct to me in that other elements will have the same opportunities as the submit button. What I really don't like about this is that it is changing an existing API. But it is an option. Also worth noting it relies on the template existing, or the element having set the template it wants to use. Finally [c] introduce a new callback that gives the element the opportunity to decide. In this way you could inspect each element and if it has a method say "safeToRemoveEmptyLabel" and when called with all the relevant information (likely a reference to the renderer as well) if it returns true the existing bit of regex removes any empty labels. This would basically be replacing all of those checks with the callback and working on the answer it gives if it exists. To be truthful I don't like this solution, it is still munging HTML using regex and I do feel that the templates are a better place to solve this. Anyway in summary all three solutions have their flaws, really it is just a case of deciding. If everyone feels the first solution is better I am happy to integrate it, really I just want to see it discussed. The mforms version in Moodle is a tired old forms library, but I think replacing it (or upgrading it) is on the board any time soon so what ever we choose we have to live with. Cheers Sam
          Hide
          Ankit Agarwal added a comment - - edited

          Thanks for feedback Sam,

          I understand that the hard coding is not a good idea, we can go ahead and create a new api for this.
          say a property "nolabel" and if its set remove label field irrespective of any other setting. But that will need us to study all elements behavior and see how we can proceed with this. I can create a dev issue for that if required.

          As far the current options are considered, I guess we can safely implement the new template without any regressions in core. But it might break something in someone's contrib. so if we want to account for all those situations, we will have to go with the regex approach.

          So just needs a decision being made.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Thanks for feedback Sam, I understand that the hard coding is not a good idea, we can go ahead and create a new api for this. say a property "nolabel" and if its set remove label field irrespective of any other setting. But that will need us to study all elements behavior and see how we can proceed with this. I can create a dev issue for that if required. As far the current options are considered, I guess we can safely implement the new template without any regressions in core. But it might break something in someone's contrib. so if we want to account for all those situations, we will have to go with the regex approach. So just needs a decision being made. Thanks
          Hide
          Michael de Raadt added a comment -

          It looks like we've reached an impasse on this issue.

          A lot of effort is being invested in what is really a minor issue. At this point I suggest two possible courses of action.

          1. Leave this issue, with its potential solutions, and wait for a Quickform upgrade/replacement; or
          2. Implement the "hardcoded" check that resolves this issue and add a todo to review the solution after a Quickform upgrade/replacement.
          Show
          Michael de Raadt added a comment - It looks like we've reached an impasse on this issue. A lot of effort is being invested in what is really a minor issue. At this point I suggest two possible courses of action. Leave this issue, with its potential solutions, and wait for a Quickform upgrade/replacement; or Implement the "hardcoded" check that resolves this issue and add a todo to review the solution after a Quickform upgrade/replacement.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Just my opinion,

          in general I don't think we should be applying "central" (formslib) solutions like this, as far as they (the conditions) can become out of control if new element types arrive. Proper solution should be always for each element type, and not post-processing output.

          In the other side... if you think this works and supports for all current elements in core without any breakage (and that would need tests to demo it certainly) if could be accepted.

          So, surely, I could summarize my feelings as:

          • 70% -> leave it for 2.4 and revamped forms support.
          • 30% -> apply it now if covered by complete tests.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Just my opinion, in general I don't think we should be applying "central" (formslib) solutions like this, as far as they (the conditions) can become out of control if new element types arrive. Proper solution should be always for each element type, and not post-processing output. In the other side... if you think this works and supports for all current elements in core without any breakage (and that would need tests to demo it certainly) if could be accepted. So, surely, I could summarize my feelings as: 70% -> leave it for 2.4 and revamped forms support. 30% -> apply it now if covered by complete tests. Ciao
          Hide
          Ankit Agarwal added a comment -

          This will have to wait for 2.4 or till the forms are redone.
          sending this back to stable backlog. If someone wants the solution is present in this issue and can be pulled and used.
          Thanks

          Show
          Ankit Agarwal added a comment - This will have to wait for 2.4 or till the forms are redone. sending this back to stable backlog. If someone wants the solution is present in this issue and can be pulled and used. Thanks
          Hide
          Ankit Agarwal added a comment -

          Attaching the solution as diff.
          closing the issue as deferred.
          Thanks

          Show
          Ankit Agarwal added a comment - Attaching the solution as diff. closing the issue as deferred. Thanks

            People

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

              Dates

              • Created:
                Updated:
                Resolved: