Moodle
  1. Moodle
  2. MDL-42270

When section name limit is exceeded no suitable error is provided.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.6, 2.5.2, 2.6
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide
      1. Go to www.lipsum.com and generate a 256 character string such as "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Maecenas sollicitudin fermentum justo sit amet tempor. Proin sed lorem mollis neque interdum consectetur eu quis ipsum. Duis dictum quis lectus egestas suscipit. Pellentesque viverra, libero vel sed."
      2. Create a course with the 'Topics' format.
      3. Edit the section summary for section 1.
      4. Click off 'Use default section name'.
      5. Copy / paste the supplied text in the 'Section name' box.
      6. Click on 'Save changes'.
      7. Observe the error 'Error writing to database More information about this error'.
      8. Click on 'Continue'.
      9. Observe the error 'A required parameter (id) was missing More information about this error'.
      10. Click on 'Continue'.
      11. Go back to the course.
      12. Apply the patch.
      13. Edit the section summary for section 1.
      14. Click off 'Use default section name'.
      15. Copy / paste the supplied text in the 'Section name' box.
      16. Confirm that the end period has been removed from the text so that it reads "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Maecenas sollicitudin fermentum justo sit amet tempor. Proin sed lorem mollis neque interdum consectetur eu quis ipsum. Duis dictum quis lectus egestas suscipit. Pellentesque viverra, libero vel sed" and therefore is not greater than 255 characters.

      Now, let's hack a bit and change this line (#30):

      https://github.com/gjb2048/moodle/compare/master...wip-MDL-42270_master_2#diff-84752bcc4372387af8c038da4d9322b3R30

      so both "255" occurrences are modified to "20". So the form will continue allowing 255, but the validation will be show if we use more than 20 chars.

      Once hacked:

      1) Go to any section and edit it.
      2) Click off 'Use default section name'.
      3) Add some text 20 chars long. Save. It will be saved ok.
      4) Edit the same section.
      5) Add 1 more char (so it will be 21 long). Save. The length error message is shown.

      That's all.

      Show
      Go to www.lipsum.com and generate a 256 character string such as "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Maecenas sollicitudin fermentum justo sit amet tempor. Proin sed lorem mollis neque interdum consectetur eu quis ipsum. Duis dictum quis lectus egestas suscipit. Pellentesque viverra, libero vel sed." Create a course with the 'Topics' format. Edit the section summary for section 1. Click off 'Use default section name'. Copy / paste the supplied text in the 'Section name' box. Click on 'Save changes'. Observe the error 'Error writing to database More information about this error'. Click on 'Continue'. Observe the error 'A required parameter (id) was missing More information about this error'. Click on 'Continue'. Go back to the course. Apply the patch. Edit the section summary for section 1. Click off 'Use default section name'. Copy / paste the supplied text in the 'Section name' box. Confirm that the end period has been removed from the text so that it reads "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Maecenas sollicitudin fermentum justo sit amet tempor. Proin sed lorem mollis neque interdum consectetur eu quis ipsum. Duis dictum quis lectus egestas suscipit. Pellentesque viverra, libero vel sed" and therefore is not greater than 255 characters. Now, let's hack a bit and change this line (#30): https://github.com/gjb2048/moodle/compare/master...wip-MDL-42270_master_2#diff-84752bcc4372387af8c038da4d9322b3R30 so both "255" occurrences are modified to "20". So the form will continue allowing 255, but the validation will be show if we use more than 20 chars. Once hacked: 1) Go to any section and edit it. 2) Click off 'Use default section name'. 3) Add some text 20 chars long. Save. It will be saved ok. 4) Edit the same section. 5) Add 1 more char (so it will be 21 long). Save. The length error message is shown. That's all.
    • Workaround:
      Hide

      None.

      Show
      None.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      wip-MDL-42270_M24_2
    • Pull 2.5 Branch:
      wip-MDL-42270_M25_2
    • Pull Master Branch:
      wip-MDL-42270_master_2
    • Rank:
      53516

      Description

      On https://moodle.org/mod/forum/discuss.php?d=241466, Paul K reported that no suitable error message was given to the users when they entered over the limit for the section name.

      Upon inspection of 'install.xml' the size of the 'name' attribute in the 'course_sections' table is 255 and this is not being checked by the method 'validation' in '/course/editsection_form.php'.

      1. sne.png
        17 kB
      2. sne2.png
        5 kB

        Issue Links

          Activity

          Hide
          Gareth J Barnard added a comment -

          Screen shot 'sne.png' shows the issue with developer level debugging turned on. Screen shot 'sne2.png' shows the solution.

          Show
          Gareth J Barnard added a comment - Screen shot 'sne.png' shows the issue with developer level debugging turned on. Screen shot 'sne2.png' shows the solution.
          Hide
          Rajesh Taneja added a comment -

          Thanks for reporting and fixing this Gareth,

          Patch looks good, although at first I thought why to pass {$a}, as we know it's 255 characters. Probably it's good to keep it that way in case we plan to increase the size in db.
          Won't be so fussy about it. Do you this error string can be improved a bit to "Section name exceeds {$a} characters". Added Helen as watcher for her inputs on string.

          [y] Syntax
          [y] Whitespace
          [y] Output
          [y] Language
          [-] Databases
          [y] Testing (instructions and automated tests)
          [-] Security
          [-] Documentation
          [y] Git
          [y] Sanity check

          Show
          Rajesh Taneja added a comment - Thanks for reporting and fixing this Gareth, Patch looks good, although at first I thought why to pass {$a}, as we know it's 255 characters. Probably it's good to keep it that way in case we plan to increase the size in db. Won't be so fussy about it. Do you this error string can be improved a bit to "Section name exceeds {$a} characters". Added Helen as watcher for her inputs on string. [y] Syntax [y] Whitespace [y] Output [y] Language [-] Databases [y] Testing (instructions and automated tests) [-] Security [-] Documentation [y] Git [y] Sanity check
          Hide
          Gareth J Barnard added a comment -

          Dear Rajesh Taneja & Helen Foster,

          Than you for peer reviewing the patch . I did base the words upon the existing:

          $string['uploadedfiletoobig'] = 'Sorry, but that file is too big (limit is {$a} bytes)';
          

          so that it would be consistent as you can see:

          $string['sectionnametoolong'] = 'Sorry, but the section name exceeds {$a} characters';
          

          and not so 'in your face' .

          Also on a separate note the form element 'name' is in a group on the form and I found that I had to use:

          $errors['name_group']
          

          which is the name of the group element instead of the element itself:

          $errors['name']
          

          so that the error message would actually appear. Is this a fault with the forms library?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Rajesh Taneja & Helen Foster , Than you for peer reviewing the patch . I did base the words upon the existing: $string['uploadedfiletoobig'] = 'Sorry, but that file is too big (limit is {$a} bytes)'; so that it would be consistent as you can see: $string['sectionnametoolong'] = 'Sorry, but the section name exceeds {$a} characters'; and not so 'in your face' . Also on a separate note the form element 'name' is in a group on the form and I found that I had to use: $errors['name_group'] which is the name of the group element instead of the element itself: $errors['name'] so that the error message would actually appear. Is this a fault with the forms library? Cheers, Gareth
          Hide
          Helen Foster added a comment -

          Raj, thanks for adding me as a watcher and Gareth, thanks for explaining how you came up with the string.

          I suggest the following wording:

          'The section name must be no longer than {$a} characters.'

          based on [errorminpasswordlength,core_auth] 'Passwords must be at least {$a} characters long.'

          as I think it makes it a little bit clearer what is required, however both of your suggestions are fine also.

          Show
          Helen Foster added a comment - Raj, thanks for adding me as a watcher and Gareth, thanks for explaining how you came up with the string. I suggest the following wording: 'The section name must be no longer than {$a} characters.' based on [errorminpasswordlength,core_auth] 'Passwords must be at least {$a} characters long.' as I think it makes it a little bit clearer what is required, however both of your suggestions are fine also.
          Hide
          Gareth J Barnard added a comment -

          Dear Helen Foster and Rajesh Taneja,

          All branches updated with the words suggested. Have a good weekend

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Helen Foster and Rajesh Taneja , All branches updated with the words suggested. Have a good weekend Cheers, Gareth
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Gareth,

          Patch looks grt, pushing for integration.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Gareth, Patch looks grt, pushing for integration.
          Hide
          Dan Poltawski added a comment -

          Reopening, sorry - strlen on its own does not respect wide utf-8 characters. Instead please use core_text::strlen() (and textlib::strlen() on earlier branches)

          Show
          Dan Poltawski added a comment - Reopening, sorry - strlen on its own does not respect wide utf-8 characters. Instead please use core_text::strlen() (and textlib::strlen() on earlier branches)
          Hide
          Gareth J Barnard added a comment -

          Dear Dan Poltawski,

          Thanks for integration reviewing the patch and saving me lots of time finding the correct method and class. All branches updated and rebased against this week's current state of play.

          Submitting for peer review Rajesh Taneja as that's all the power I have!

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Dan Poltawski , Thanks for integration reviewing the patch and saving me lots of time finding the correct method and class. All branches updated and rebased against this week's current state of play. Submitting for peer review Rajesh Taneja as that's all the power I have! Cheers, Gareth
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Gareth,

          Pushing it for integration...

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Gareth, Pushing it 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
          Gareth J Barnard added a comment -

          All branches rebased.

          Show
          Gareth J Barnard added a comment - All branches rebased.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Sorry, I'm reopening this. I don't understand why we are using custom validation here when rules should be enough:

          I've quick tried this:

          @@ -24,9 +24,10 @@ class editsection_form extends moodleform {
                   $mform->addElement('header', 'generalhdr', get_string('general'));
           
                   $elementgroup = array();
          -        $elementgroup[] = $mform->createElement('text', 'name', '', array('size' => '30'));
          +        $elementgroup[] = $mform->createElement('text', 'name', '', array('size' => '30', 'maxlength' => '255'));
                   $elementgroup[] = $mform->createElement('checkbox', 'usedefaultname', '', get_string('sectionusedefaultname'));
                   $mform->addGroup($elementgroup, 'name_group', get_string('sectionname'), ' ', false);
          +        $mform->addGroupRule('name_group', get_string('maximumchars', '', 255), 'maxlength', 255, 1);
          +        // Or, also, to point to the exact element we want.
          +        // $mform->addGroupRule('name_group', array('name' => array(array(get_string('maximumchars', '', 255), 'maxlength', 255))));
           
                   $mform->setDefault('usedefaultname', true);
                   $mform->setType('name', PARAM_TEXT);
          

          And it's working perfectly.

          Worth noting:

          1) The Moodle Docs page above recommends using the "err_maxlength" string. Nobody is using it in core, but "maximumchars" instead. Some day we should consider moving to the "official" (an unused one).

          2) While looking for information, I've seen some bugs in the tracker related with the "maxlength" rule. For reference: MDL-40267 and MDL-40813. Still I think we should be using the rule.

          So, I'm sorry Gareth, but I have to reopen this once more.

          Ciao

          Edited: Amended the original code and adding an alternative.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Sorry, I'm reopening this. I don't understand why we are using custom validation here when rules should be enough: http://docs.moodle.org/dev/lib/formslib.php_Validation http://pear.php.net/manual/en/package.html.html-quickform.html-quickform.addgrouprule.php I've quick tried this: @@ -24,9 +24,10 @@ class editsection_form extends moodleform { $mform->addElement('header', 'generalhdr', get_string('general')); $elementgroup = array(); - $elementgroup[] = $mform->createElement('text', 'name', '', array('size' => '30')); + $elementgroup[] = $mform->createElement('text', 'name', '', array('size' => '30', 'maxlength' => '255')); $elementgroup[] = $mform->createElement('checkbox', 'usedefaultname', '', get_string('sectionusedefaultname')); $mform->addGroup($elementgroup, 'name_group', get_string('sectionname'), ' ', false ); + $mform->addGroupRule('name_group', get_string('maximumchars', '', 255), 'maxlength', 255, 1); + // Or, also, to point to the exact element we want. + // $mform->addGroupRule('name_group', array('name' => array(array(get_string('maximumchars', '', 255), 'maxlength', 255)))); $mform->setDefault('usedefaultname', true ); $mform->setType('name', PARAM_TEXT); And it's working perfectly. Worth noting: 1) The Moodle Docs page above recommends using the "err_maxlength" string. Nobody is using it in core, but "maximumchars" instead. Some day we should consider moving to the "official" (an unused one). 2) While looking for information, I've seen some bugs in the tracker related with the "maxlength" rule. For reference: MDL-40267 and MDL-40813 . Still I think we should be using the rule. So, I'm sorry Gareth, but I have to reopen this once more. Ciao Edited: Amended the original code and adding an alternative.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Gareth J Barnard added a comment -

          Dear Eloy Lafuente (stronk7),

          All branches changed with new fix. Submitting for 'Peer review' as that is all I can do.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Eloy Lafuente (stronk7) , All branches changed with new fix. Submitting for 'Peer review' as that is all I can do. Cheers, Gareth
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sending this straight to integration, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Sending this straight to integration, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For reference, I've created MDL-42529 about to solve some of the problems detected while integrating this.

          Show
          Eloy Lafuente (stronk7) added a comment - For reference, I've created MDL-42529 about to solve some of the problems detected while integrating this.
          Hide
          Mary Evans added a comment -

          Eloy, can you arrange it so Gareth gets permissions so he can push fixes for Integration Review? It would be most helpful if you can.

          Show
          Mary Evans added a comment - Eloy, can you arrange it so Gareth gets permissions so he can push fixes for Integration Review? It would be most helpful if you can.
          Hide
          Dan Poltawski added a comment -

          Ah, it was because of MDL-40813 which I didn't comment it on gareths approach, but I prefer this (Petrs view seems to be that we avoid using those rules altogether)

          Show
          Dan Poltawski added a comment - Ah, it was because of MDL-40813 which I didn't comment it on gareths approach, but I prefer this (Petrs view seems to be that we avoid using those rules altogether)
          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
          Gareth J Barnard added a comment -

          All branches transplanted to another tree as requested.

          Show
          Gareth J Barnard added a comment - All branches transplanted to another tree as requested.
          Hide
          Gareth J Barnard added a comment -

          Hi Dan Poltawski,

          My apologies for not updating the pull branch references in the tracker.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Hi Dan Poltawski , My apologies for not updating the pull branch references in the tracker. Cheers, Gareth
          Hide
          Dan Poltawski added a comment -

          Hi Gareth,

          No problem.

          However, I think that Eloy & I have both been a tag team in making this issue harder for you than it needs to be.

          I think that we can't have the combination of maxlength in the form field and the validation rule. The problem is that then the user won't get any error if they use a section name which is too long. Correct me if i'm wrong, but testing this - at the moment its impossible to create a section name greater than 255 chars - so actually we don't fix the originally issue reported?

          Show
          Dan Poltawski added a comment - Hi Gareth, No problem. However, I think that Eloy & I have both been a tag team in making this issue harder for you than it needs to be. I think that we can't have the combination of maxlength in the form field and the validation rule. The problem is that then the user won't get any error if they use a section name which is too long. Correct me if i'm wrong, but testing this - at the moment its impossible to create a section name greater than 255 chars - so actually we don't fix the originally issue reported?
          Hide
          Gareth J Barnard added a comment -

          Hi Dan,

          Who's Stater and who is Waldorf?

          Indeed with the fix as it stands it is now a preventative measure and no error message is given. However, I believe looking back at the forum post that the real issue is the database exception, so in that event how we fix it depends on us and the need to prevent the exception happening.

          I'm going on dev chat if you want to chat more.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Hi Dan, Who's Stater and who is Waldorf? Indeed with the fix as it stands it is now a preventative measure and no error message is given. However, I believe looking back at the forum post that the real issue is the database exception, so in that event how we fix it depends on us and the need to prevent the exception happening. I'm going on dev chat if you want to chat more. Cheers, Gareth
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          About this, well. I'm only strong in one point. And it's that the validation rule must exist. Then, if we want to apply (or not) the maxlength to the element specifications, I've not a strong position about that. But the validation must happen.

          Edited: s/ht/th, grrr)

          Show
          Eloy Lafuente (stronk7) added a comment - - edited About this, well. I'm only strong in one point. And it's that the validation rule must exist. Then, if we want to apply (or not) the maxlength to the element specifications, I've not a strong position about that. But the validation must happen. Edited: s/ht/th, grrr)
          Hide
          Gareth J Barnard added a comment -

          Ok, so what's it to be then? - Thinking about it I would want an error message as in the first solution as in the second the keyboard entry just stops making you think that the AA Batteries have just expired on your wireless keyboard. But to be honest, I'm happy either way, as long as this does not drag into after the release of Moodle 2.6 and I have to add another branch to the issue (thus having an additional MOODLE_26_STABLE branch).

          Show
          Gareth J Barnard added a comment - Ok, so what's it to be then? - Thinking about it I would want an error message as in the first solution as in the second the keyboard entry just stops making you think that the AA Batteries have just expired on your wireless keyboard. But to be honest, I'm happy either way, as long as this does not drag into after the release of Moodle 2.6 and I have to add another branch to the issue (thus having an additional MOODLE_26_STABLE branch).
          Hide
          Dan Poltawski added a comment - - edited

          Thanks for explaining - i'm happy with enough with this solution and have integrated it into master, 25 and 24.

          Just for the record, in a brave new world, I would prefer it if it worked this way:

          1) No max field limit it the text field (that truncates content which has been copy and pasted, isn't displayed to the user and is unfriendly).
          2) A JS validation error when over length (the user friendly version of max field length - well, as long as we have to limit the length). Currently achieved by:

          $mform->addRule('name', get_string('maximumchars', '', 255), 'maxlength', 255, 'client');
          

          3) A server side validation error if over length and submitted. Currently achieved by:

          $mform->addRule('name', get_string('maximumchars', '', 255), 'maxlength', 255);
          

          4) (All other clean_param and things like that, after all of this).

          And it'll be done with one line, by a fancy new forms library

          Show
          Dan Poltawski added a comment - - edited Thanks for explaining - i'm happy with enough with this solution and have integrated it into master, 25 and 24. Just for the record, in a brave new world, I would prefer it if it worked this way: 1) No max field limit it the text field (that truncates content which has been copy and pasted, isn't displayed to the user and is unfriendly). 2) A JS validation error when over length (the user friendly version of max field length - well, as long as we have to limit the length). Currently achieved by: $mform->addRule('name', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); 3) A server side validation error if over length and submitted. Currently achieved by: $mform->addRule('name', get_string('maximumchars', '', 255), 'maxlength', 255); 4) (All other clean_param and things like that, after all of this). And it'll be done with one line, by a fancy new forms library
          Hide
          Dan Poltawski added a comment -

          (Sorry Gareth, commented before seeing your comments. I agree - we just need to get the issue fixed and not let it drag on, so have integrated).

          Show
          Dan Poltawski added a comment - (Sorry Gareth, commented before seeing your comments. I agree - we just need to get the issue fixed and not let it drag on, so have integrated).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 to postpone this to 2.7 (so we can make some more circles). LOL.

          Show
          Eloy Lafuente (stronk7) added a comment - +1 to postpone this to 2.7 (so we can make some more circles). LOL.
          Hide
          Gareth J Barnard added a comment - - edited

          -1 to postpone this to 2.7 so I can get on with the Grid format CONTRIB-4099 release and making Collapsed Topics, Grid format, Columns format, Noticeboard format, Shoelace theme and Mutant Banjo theme 2.6 compatible as there have been some API changes.

          And are they Alien crop circles Eloy?

          Thanks for integrating Dan

          Show
          Gareth J Barnard added a comment - - edited -1 to postpone this to 2.7 so I can get on with the Grid format CONTRIB-4099 release and making Collapsed Topics, Grid format, Columns format, Noticeboard format, Shoelace theme and Mutant Banjo theme 2.6 compatible as there have been some API changes. And are they Alien crop circles Eloy? Thanks for integrating Dan
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Really think 1) current solution is perfect enough and 2) we'll need to define the standard way to achieve this everywhere. In fact I suggested to set the maxlen in the element after doing some quick greps and verifying that we already were using it in a lot of fields here and there)

          (hey I was 100% joking about to postpone this. I agree 100% with Dan and with Gareth, well done both!).

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Really think 1) current solution is perfect enough and 2) we'll need to define the standard way to achieve this everywhere. In fact I suggested to set the maxlen in the element after doing some quick greps and verifying that we already were using it in a lot of fields here and there) (hey I was 100% joking about to postpone this. I agree 100% with Dan and with Gareth, well done both!).
          Hide
          Petr Škoda added a comment -

          Dan: please stop encouraging others to use the horribly broken addGroupRule()!!!

          I must fail this because it does not work for unicode characters, please use only standard maxlengh attribute - that is the only supported way to specify length limit in moodle forms for now.

          Show
          Petr Škoda added a comment - Dan: please stop encouraging others to use the horribly broken addGroupRule()!!! I must fail this because it does not work for unicode characters, please use only standard maxlengh attribute - that is the only supported way to specify length limit in moodle forms for now.
          Hide
          Gareth J Barnard added a comment - - edited

          Hi Petr Škoda,

          Ok, to be fair to all it was Eloy who proposed the 'addGroupRule' solution and I agreed as it was a simpler solution not requiring a new string in the Moodle language file. So, would in your opinion, the first solution -> https://github.com/gjb2048/moodle/compare/master...wip-MDL-42270_master be better and in your opinion get through testing?

          I now feel like a pawn in a game of Moodle HQ Chess. If I have no decision tonight I'll rebase and resubmit the first solution sometime tomorrow morning (GMT) as I have no desire to add yet another branch to this issue when M2.6 comes out.

          Cheers,

          Gareth

          P.S. Is 'maxlengh' a real thing or a typo of 'maxlength'? So I know what to search on when looking for the third solution to this.

          Show
          Gareth J Barnard added a comment - - edited Hi Petr Škoda , Ok, to be fair to all it was Eloy who proposed the 'addGroupRule' solution and I agreed as it was a simpler solution not requiring a new string in the Moodle language file. So, would in your opinion, the first solution -> https://github.com/gjb2048/moodle/compare/master...wip-MDL-42270_master be better and in your opinion get through testing? I now feel like a pawn in a game of Moodle HQ Chess. If I have no decision tonight I'll rebase and resubmit the first solution sometime tomorrow morning (GMT) as I have no desire to add yet another branch to this issue when M2.6 comes out. Cheers, Gareth P.S. Is 'maxlengh' a real thing or a typo of 'maxlength'? So I know what to search on when looking for the third solution to this.
          Hide
          Petr Škoda added a comment -

          ah, right, typo

          This is the correct solution, we use it in many other form fields:

          $elementgroup[] = $mform->createElement('text', 'name', '', array('size' => '30', 'maxlength' => '255'));
          

          The quickforms 'rule' for text length is broken, I keep repeating it and some integrators simply do not listen...

          Show
          Petr Škoda added a comment - ah, right, typo This is the correct solution, we use it in many other form fields: $elementgroup[] = $mform->createElement('text', 'name', '', array('size' => '30', 'maxlength' => '255')); The quickforms 'rule' for text length is broken, I keep repeating it and some integrators simply do not listen...
          Hide
          Petr Škoda added a comment -

          The code is in integration branches already, the next step is to create extra commit for each branch that removes the following line:

          $mform->addGroupRule('name_group', array('name' => array(array(get_string('maximumchars', '', 255), 'maxlength', 255))));
          

          and integrators should merge it asap

          Show
          Petr Škoda added a comment - The code is in integration branches already, the next step is to create extra commit for each branch that removes the following line: $mform->addGroupRule('name_group', array('name' => array(array(get_string('maximumchars', '', 255), 'maxlength', 255)))); and integrators should merge it asap
          Hide
          Gareth J Barnard added a comment - - edited

          Dear all,

          Ok, this is what I'll do tomorrow if somebody could chuck it back to me as I have no power to do anything at the moment. I'll make new branches containing Petr's third solution and submit for peer review as that's all I can do. If it could then be moved to integration.....

          God, I feel like I've asked for colons in single line comments all over again!

          Cheers,

          Gareth

          P.S. I'm more annoyed with myself that I don't yet know this sort of thing to be able to submit the right solution in the first place and save weeks of admin work.

          Show
          Gareth J Barnard added a comment - - edited Dear all, Ok, this is what I'll do tomorrow if somebody could chuck it back to me as I have no power to do anything at the moment. I'll make new branches containing Petr's third solution and submit for peer review as that's all I can do. If it could then be moved to integration..... God, I feel like I've asked for colons in single line comments all over again! Cheers, Gareth P.S. I'm more annoyed with myself that I don't yet know this sort of thing to be able to submit the right solution in the first place and save weeks of admin work.
          Hide
          Dan Poltawski added a comment -

          Right. Petr - you will see I mentioned it, above too. And Eloy proffered that we at least have the rule.

          And Gareth is right, he's the victim of many conflicting opinions on this issue, and utf8 bug and all, i'm sending this back to testing.

          Please ignore the utf8 problem for the purposes of this test. For the sake of gareths sanity.

          Show
          Dan Poltawski added a comment - Right. Petr - you will see I mentioned it, above too. And Eloy proffered that we at least have the rule. And Gareth is right, he's the victim of many conflicting opinions on this issue, and utf8 bug and all, i'm sending this back to testing. Please ignore the utf8 problem for the purposes of this test. For the sake of gareths sanity.
          Hide
          Petr Škoda added a comment -

          Are you serious Dan? This is a major regression and it is all your fault because you started using the rules for text fields. It must be fixed now!

          Show
          Petr Škoda added a comment - Are you serious Dan? This is a major regression and it is all your fault because you started using the rules for text fields. It must be fixed now!
          Show
          Petr Škoda added a comment - here are the fixes: https://github.com/skodak/moodle/compare/borkedtextrule26 https://github.com/skodak/moodle/compare/borkedtextrule25 https://github.com/skodak/moodle/compare/borkedtextrule24
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Wow, life is really simpler:

          And then:

          • This issue, validate length cannot exceed 255. And it's achieved. Knowing that the 2 issues above exist (we all are aware they exist, it just seems they need to be pushed higher in the development priorities. But that's APART from this issue. So please, test it and done (hopefully without using utf8 strings).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Wow, life is really simpler: Fix the UTF-8 rule thing: MDL-40267 Fix the clean_param() thing: MDL-40813 Document rules properly: MDL-42529 And then: This issue, validate length cannot exceed 255. And it's achieved. Knowing that the 2 issues above exist (we all are aware they exist, it just seems they need to be pushed higher in the development priorities. But that's APART from this issue. So please, test it and done (hopefully without using utf8 strings). Ciao
          Hide
          Dan Poltawski added a comment -

          Petr, these issues are existing and we can discuss them in the other issue which is already open for this. Not this one, please. Its confusing enough already and yes, I am saying I want to accept this solution here. Maybe I will work on a solution to the utf8 bugs later today.

          Your 'fixes' are to rely on the database server for validation.

          Show
          Dan Poltawski added a comment - Petr, these issues are existing and we can discuss them in the other issue which is already open for this. Not this one, please. Its confusing enough already and yes, I am saying I want to accept this solution here. Maybe I will work on a solution to the utf8 bugs later today. Your 'fixes' are to rely on the database server for validation.
          Hide
          Petr Škoda added a comment - - edited

          My fix is correct, stop this confusion and merge it please. The input field attribute prevents database errors, if anybody tries to hack around it the DB layer simply truncates it (mysql) or throws fatal error (pg). This is how it worked since Moodle 1.7.0

          The MDL-40267 must be fixed before we can even consider adding addRule for text field lengths in master branch (doing it in stable would be very wrong), the current rules are pretty simple: do not use any rules except 'required'.

          Show
          Petr Škoda added a comment - - edited My fix is correct, stop this confusion and merge it please. The input field attribute prevents database errors, if anybody tries to hack around it the DB layer simply truncates it (mysql) or throws fatal error (pg). This is how it worked since Moodle 1.7.0 The MDL-40267 must be fixed before we can even consider adding addRule for text field lengths in master branch (doing it in stable would be very wrong), the current rules are pretty simple: do not use any rules except 'required'.
          Hide
          Petr Škoda added a comment -

          btw it works fine with my patch for me - 255 limit is enforced without any error message, feel free to mark as tested if it gets merged, ciao

          Show
          Petr Škoda added a comment - btw it works fine with my patch for me - 255 limit is enforced without any error message, feel free to mark as tested if it gets merged, ciao
          Hide
          Petr Škoda added a comment -

          Note: even if MDL-40267 gets fixed this still does not work because maxlength attribute prevents the validation rules error message because the text cannot be longer.

          Show
          Petr Škoda added a comment - Note: even if MDL-40267 gets fixed this still does not work because maxlength attribute prevents the validation rules error message because the text cannot be longer.
          Hide
          Dan Poltawski added a comment -

          It does if you hack the html and as far as I know - since you were suggesting we only rely on that, that will respect utf8 limits (See we've also discussed this above).

          Show
          Dan Poltawski added a comment - It does if you hack the html and as far as I know - since you were suggesting we only rely on that, that will respect utf8 limits (See we've also discussed this above).
          Hide
          Petr Škoda added a comment -

          Dan, could you please stop inventing new coding style in this issue? Just apply my patch or delete the maxlength attribute and let's move on. There are bazillion of form fields that do not enforce the length on the server side. Let's address it consistently in 2.7dev after unfreeze. Ok?

          Show
          Petr Škoda added a comment - Dan, could you please stop inventing new coding style in this issue? Just apply my patch or delete the maxlength attribute and let's move on. There are bazillion of form fields that do not enforce the length on the server side. Let's address it consistently in 2.7dev after unfreeze. Ok?
          Hide
          Dan Poltawski added a comment -

          Petr. As I have now explained 3 times, we (me and Eloy) guided Gareth down this path, and it is unnecessary to change it at this point.

          Show
          Dan Poltawski added a comment - Petr. As I have now explained 3 times, we (me and Eloy) guided Gareth down this path, and it is unnecessary to change it at this point.
          Hide
          Petr Škoda added a comment - - edited

          You were wrong, and no it is not ok to introduce new coding style now.

          -I did what I was told in test instructions above and did not get any error message === FAIL!!!-

          Stop the excuses and apply my patch or delete the maxlength attribute.

          Show
          Petr Škoda added a comment - - edited You were wrong, and no it is not ok to introduce new coding style now. - I did what I was told in test instructions above and did not get any error message === FAIL!!! - Stop the excuses and apply my patch or delete the maxlength attribute.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Petr, can you detail at which exact point in testing instructions you got an unexpected behavior? That would help further investigating this issue.

          Gareth, no action is needed in your side at all.

          TIA and ciao :.-)

          Show
          Eloy Lafuente (stronk7) added a comment - Petr, can you detail at which exact point in testing instructions you got an unexpected behavior? That would help further investigating this issue. Gareth, no action is needed in your side at all. TIA and ciao :.-)
          Hide
          Petr Škoda added a comment -

          Dear integrators: I see that rules do not apply to you, do whatever you want with this issue. Ciao.

          Show
          Petr Škoda added a comment - Dear integrators: I see that rules do not apply to you, do whatever you want with this issue. Ciao.
          Hide
          Andrew Nicols added a comment -

          All works as expected fo rme.

          Show
          Andrew Nicols added a comment - All works as expected fo rme.
          Hide
          Mary Evans added a comment -

          Coffee anyone? (_)?

          Show
          Mary Evans added a comment - Coffee anyone? (_)?
          Hide
          Damyon Wiese added a comment -

          Here lies 52 bugs.
          All fixed or swept under a rug.
          If they come back one day,
          To our dismay,
          We all will feel quite un-smug.

          Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

          Show
          Damyon Wiese added a comment - Here lies 52 bugs. All fixed or swept under a rug. If they come back one day, To our dismay, We all will feel quite un-smug. Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: