Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-35395

Provide some way to disable the core_formchangechecker for a given form

    Details

    • Testing Instructions:
      Hide

      This test script demonstrates the problem. If you run it, you will get a JavaScript error. The two comments indicate two ways to make the error go away. If you are doing clever Ajax things then you need the new disable_form_change_checker way of doing it.

      <?php
      require_once('config.php');
      require_once($CFG->libdir . '/formslib.php');
       
      $PAGE->set_context(context_system::instance());
      $PAGE->set_url('/test.php');
       
      class myform extends moodleform {
          protected function definition() {
              // Uncomment the following line to make the JavaScript error go away.
              // $this->_form->disable_form_change_checker();
              $this->add_action_buttons();
          }
       
          public function render() {
              ob_start();
              $this->display();
              return ob_get_clean();
          }
      }
       
      $mform = new myform();
       
      echo $OUTPUT->header();
      // For some reason, render a form without outputting it. The JavaScript error
      // will also go away if you add echo to the start of the next line.
      $mform->render();
      echo $OUTPUT->footer();

      Show
      This test script demonstrates the problem. If you run it, you will get a JavaScript error. The two comments indicate two ways to make the error go away. If you are doing clever Ajax things then you need the new disable_form_change_checker way of doing it. <?php require_once('config.php'); require_once($CFG->libdir . '/formslib.php');   $PAGE->set_context(context_system::instance()); $PAGE->set_url('/test.php');   class myform extends moodleform { protected function definition() { // Uncomment the following line to make the JavaScript error go away. // $this->_form->disable_form_change_checker(); $this->add_action_buttons(); }   public function render() { ob_start(); $this->display(); return ob_get_clean(); } }   $mform = new myform();   echo $OUTPUT->header(); // For some reason, render a form without outputting it. The JavaScript error // will also go away if you add echo to the start of the next line. $mform->render(); echo $OUTPUT->footer();
    • Workaround:
      Hide

      (some of the dirty ones commented in the description of this issue)

      Show
      (some of the dirty ones commented in the description of this issue)
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      At some point core_formchangechecker module was added to all forms in Moodle.

      And it's great, but for some old/legacy/contrib forms still performing multiple submits and not fully migrated to have pure js alternatives (nested elements... actions at distance...).

      Right now it's really hacky to disable such core_formchangechecker from a given form:

      • Use the "ignoredirty" class at field level (does not work at form level and it's imperfect coz does not mark the form as submitted apparently.
      • Hack all the submit calls, prepending "M.core_formchangechecker.set_form_submitted()" hacky.
      • Overwrite some core stuff or hack it to avoid the inclusion of the module.

      All them imperfect, dirty, hacky.

      So it would be great to have some documented way to simply skip the use of the module in a form programatically. Say, supporting the "ignoredirty" at form level (module conditionally included based on that...) or whatever.

      Ciao

        Gliffy Diagrams

          Activity

          Hide
          poltawski Dan Poltawski added a comment -

          Adding Andrew

          Show
          poltawski Dan Poltawski added a comment - Adding Andrew
          Hide
          timhunt Tim Hunt added a comment -

          Just to comment, that from the developer's point of view, the API should probably look like

          $mform->set_dont_block_navigation_when_data_has_changed();

          (or $this->set_dont_block_navigation_when_data_has_changed(); within the form definition class itself.)

          You can probably think of a better method name than that, but you get the idea. I don't have any ideas about how to implement that.

          Show
          timhunt Tim Hunt added a comment - Just to comment, that from the developer's point of view, the API should probably look like $mform->set_dont_block_navigation_when_data_has_changed(); (or $this->set_dont_block_navigation_when_data_has_changed(); within the form definition class itself.) You can probably think of a better method name than that, but you get the idea. I don't have any ideas about how to implement that.
          Hide
          timhunt Tim Hunt added a comment -

          This is causing us pain at the OU, so I am fixing this.

          Show
          timhunt Tim Hunt added a comment - This is causing us pain at the OU, so I am fixing this.
          Hide
          timhunt Tim Hunt added a comment -

          Ray, please can you peer-review this.

          Show
          timhunt Tim Hunt added a comment - Ray, please can you peer-review this.
          Hide
          timhunt Tim Hunt added a comment -

          Ray gave me a verbal peer review. Submitting for integration.

          Show
          timhunt Tim Hunt added a comment - Ray gave me a verbal peer review. Submitting for integration.
          Hide
          timhunt Tim Hunt added a comment -

          (We need this as part of converting a plugin from 2.2 to 2.3, which is why I have submitted it to the stable branch. I do not see any way that it can cause regressions.)

          Show
          timhunt Tim Hunt added a comment - (We need this as part of converting a plugin from 2.2 to 2.3, which is why I have submitted it to the stable branch. I do not see any way that it can cause regressions.)
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated (23 and master), thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (23 and master), thanks!
          Hide
          salvetore Michael de Raadt added a comment -

          The test instructions for this issue are rather irregular.

          If this can be tested with a code script, why have the unit tests not been added to?

          Show
          salvetore Michael de Raadt added a comment - The test instructions for this issue are rather irregular. If this can be tested with a code script, why have the unit tests not been added to?
          Hide
          fred Frédéric Massart added a comment -

          Hi guys,

          I tried to follow the test instructions the best I could and it seems to work fine so I think we can pass the test. However, I found it a bit strange that we are not checking that the formchangechecker is correctly disabled (assumption based on the presence of a JS error or not), and we are not checking if it still works either. Of course I did not write the patch and I might be wrong, if so please forgive me.

          I also noticed the comment "For some reason, render a form without outputting it. The JavaScript error will also go away if you add echo to the start of the next line.", and wondered why not adding a very quick test in the JS to ensure that the formid exists in the DOM?

          As I said above, the current test instructions made the test pass, so it is just a click away if my comments are not relevant.

          Thanks!

          Show
          fred Frédéric Massart added a comment - Hi guys, I tried to follow the test instructions the best I could and it seems to work fine so I think we can pass the test. However, I found it a bit strange that we are not checking that the formchangechecker is correctly disabled (assumption based on the presence of a JS error or not), and we are not checking if it still works either. Of course I did not write the patch and I might be wrong, if so please forgive me. I also noticed the comment "For some reason, render a form without outputting it. The JavaScript error will also go away if you add echo to the start of the next line." , and wondered why not adding a very quick test in the JS to ensure that the formid exists in the DOM? As I said above, the current test instructions made the test pass, so it is just a click away if my comments are not relevant. Thanks!
          Hide
          timhunt Tim Hunt added a comment -

          Micheal d: the problem here is whether some JavaScript gets run in the web browser under certain conditions. Why do you think I did not write a unit test in PHP code to try to test this? (I suppose you could try doing a test Mocking the $PAGE->requires object, but really life is too short. Note that there are no existing unit tests for this code I was just making a small enhancement to.

          Fred, the reasons we get intelligent people like you to test things is so they can apply their critical skills to find anything that might be broken. Please do not just blindly follow the testing instructions, but also do any additional tests you think are worth it. Of course the testing instructions should be complete, but the reason we have testing at all is because developers sometimes make mistakes, or do not think of everything their change might break.

          I thought about adding a test to the JavaScript so that things did not break, but I rejected it. There are other situations where the JavaScript error will highlight a real bug in the code, and so it is helpful, so it seemed better to leave it there. The situations where you want to render a form but then throw away the HTML are rare - but this came up in an OU plugin, so we needed a fix, and this improvement request already existed so I implemented it.

          Thanks for your thoughtful testing.

          Show
          timhunt Tim Hunt added a comment - Micheal d: the problem here is whether some JavaScript gets run in the web browser under certain conditions. Why do you think I did not write a unit test in PHP code to try to test this? (I suppose you could try doing a test Mocking the $PAGE->requires object, but really life is too short. Note that there are no existing unit tests for this code I was just making a small enhancement to. Fred, the reasons we get intelligent people like you to test things is so they can apply their critical skills to find anything that might be broken. Please do not just blindly follow the testing instructions, but also do any additional tests you think are worth it. Of course the testing instructions should be complete, but the reason we have testing at all is because developers sometimes make mistakes, or do not think of everything their change might break. I thought about adding a test to the JavaScript so that things did not break, but I rejected it. There are other situations where the JavaScript error will highlight a real bug in the code, and so it is helpful, so it seemed better to leave it there. The situations where you want to render a form but then throw away the HTML are rare - but this came up in an OU plugin, so we needed a fix, and this improvement request already existed so I implemented it. Thanks for your thoughtful testing.
          Hide
          fred Frédéric Massart added a comment -

          No worries Tim, passing the test. Thanks!

          Show
          fred Frédéric Massart added a comment - No worries Tim, passing the test. Thanks!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          From somewhere within the clouds...

          Congrats, this has been sent upstream and is now part of Moodle (your favorite LMS platform). Many thanks for your awesome collaboration!

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - From somewhere within the clouds... Congrats, this has been sent upstream and is now part of Moodle (your favorite LMS platform). Many thanks for your awesome collaboration! Ciao
          Hide
          salvetore Michael de Raadt added a comment -

          Was there documentation created in relation to this API change? I couldn't find any.

          Show
          salvetore Michael de Raadt added a comment - Was there documentation created in relation to this API change? I couldn't find any.
          Hide
          timhunt Tim Hunt added a comment -

          I don't think so. I guess it should be added somewhere in the formslib docs. In the definition() method, you need to do something like

          $mform->disable_form_change_checker()

          enable_form_change_checker() is also available if you need to undo that for any reason.

          Show
          timhunt Tim Hunt added a comment - I don't think so. I guess it should be added somewhere in the formslib docs. In the definition() method, you need to do something like $mform->disable_form_change_checker() enable_form_change_checker() is also available if you need to undo that for any reason.
          Show
          timhunt Tim Hunt added a comment - Dev docs added at http://docs.moodle.org/dev/Form_API#disable_form_change_checker.28.29 and http://docs.moodle.org/dev/lib/formslib.php_Form_Definition#disable_form_change_checker

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                12/Nov/12