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:
    • Rank:
      44071

      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

        Activity

        Hide
        Dan Poltawski added a comment -

        Adding Andrew

        Show
        Dan Poltawski added a comment - Adding Andrew
        Hide
        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
        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
        Tim Hunt added a comment -

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

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

        Ray, please can you peer-review this.

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

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

        Show
        Tim Hunt added a comment - Ray gave me a verbal peer review. Submitting for integration.
        Hide
        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
        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
        Eloy Lafuente (stronk7) added a comment -

        Integrated (23 and master), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (23 and master), thanks!
        Hide
        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
        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
        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
        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
        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
        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
        Frédéric Massart added a comment -

        No worries Tim, passing the test. Thanks!

        Show
        Frédéric Massart added a comment - No worries Tim, passing the test. Thanks!
        Hide
        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
        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
        Michael de Raadt added a comment -

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

        Show
        Michael de Raadt added a comment - Was there documentation created in relation to this API change? I couldn't find any.
        Hide
        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
        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
        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: