Moodle
  1. Moodle
  2. MDL-18405

formslib makes my head catch fire with rage.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.4, 2.0
    • Fix Version/s: None
    • Component/s: Forms Library
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      12845

      Description

      the combination of:

      • no submit button
      • validation
      • data_after_definition

      =RAGE.

      Here's the problem. I'm registering a non-submit submit button to add stuff to a page. Then, in data_after_definition, I detect whether that button was pressed, and do some extra stuff.
      However, the extra stuff relies on the form being valid.... and the validation routine a) gets called AFTER data_after_definion and b) not at all when a no submit button was pressed.

      I think this is BROKEN, however I guess current code relies in it.

      So attached is a patch that

      a) abstracts the moodle validation logic OUT of the is_validated function (which currently messes with data_after_definition stuff) and b, gives the new function a parameter to override the not calling validate when a no submit button was pressed.

      It's a safe patch and doesn't break BC at all.

        Activity

        Hide
        Petr Škoda added a comment -

        1/ my -2 for this change in 1.9.x
        2/ inr 2.x my +1 for rewrite of internals and general cleanup of code logic of formslib (in short stop using quickforms and improve)

        Show
        Petr Škoda added a comment - 1/ my -2 for this change in 1.9.x 2/ inr 2.x my +1 for rewrite of internals and general cleanup of code logic of formslib (in short stop using quickforms and improve)
        Hide
        Jamie Pratt added a comment -

        I agree with Petr that a rewrite of the core formslib code replacing quickforms is a good idea for Moodle 2.0. I think the code could be greatly simplified improving readability, maintainability and extendability. Am really surprised that PEAR code could be so horrible.

        Show
        Jamie Pratt added a comment - I agree with Petr that a rewrite of the core formslib code replacing quickforms is a good idea for Moodle 2.0. I think the code could be greatly simplified improving readability, maintainability and extendability. Am really surprised that PEAR code could be so horrible.
        Hide
        Jamie Pratt added a comment -

        Penny, can you not do what you need with an extra if condition as described on this page : http://docs.moodle.org/en/Development:lib/formslib.php_no_submit_button_pressed

        Show
        Jamie Pratt added a comment - Penny, can you not do what you need with an extra if condition as described on this page : http://docs.moodle.org/en/Development:lib/formslib.php_no_submit_button_pressed
        Hide
        Penny Leach added a comment -

        No - I don't have control over the caller - it's a questiontype - the caller is question/question.php... I guess that could be patched to call a plugin method if it's available.

        Show
        Penny Leach added a comment - No - I don't have control over the caller - it's a questiontype - the caller is question/question.php... I guess that could be patched to call a plugin method if it's available.
        Hide
        Penny Leach added a comment -

        adding tim.

        Show
        Penny Leach added a comment - adding tim.
        Hide
        Tim Hunt added a comment -

        If it is really necessary, I am happy to add the necessary hooks to question/question.php

        My understanding is that definition_after_data is a horrible hack that was put in to serve one case only, and should never be used.

        Without really knowing the details or looking at the code, I am wondering whether there is some other moodle_form method that is called at the right stage of the processing, and which would let you do this. Basically, if you follow the repeat_elements related code flow, can you copy what that is doing from a sub-class, as opposed to something hard-coded into forms?

        Show
        Tim Hunt added a comment - If it is really necessary, I am happy to add the necessary hooks to question/question.php My understanding is that definition_after_data is a horrible hack that was put in to serve one case only, and should never be used. Without really knowing the details or looking at the code, I am wondering whether there is some other moodle_form method that is called at the right stage of the processing, and which would let you do this. Basically, if you follow the repeat_elements related code flow, can you copy what that is doing from a sub-class, as opposed to something hard-coded into forms?
        Hide
        Petr Škoda added a comment -

        oh, no tim - definition_after_data is a hack but is intended to be used in most complex cases. In fact the repeated elements was a special purpose hack

        Show
        Petr Škoda added a comment - oh, no tim - definition_after_data is a hack but is intended to be used in most complex cases. In fact the repeated elements was a special purpose hack
        Hide
        Tim Hunt added a comment -

        No, repeat elements was in the original list of things required from a forms library - during the early discussions it was one of the complex cases I raised because I knew something like that was required by several of the question engines.

        Show
        Tim Hunt added a comment - No, repeat elements was in the original list of things required from a forms library - during the early discussions it was one of the complex cases I raised because I knew something like that was required by several of the question engines.
        Hide
        Penny Leach added a comment -

        Sorry, but IMO all of formslib is a big hack

        And Tim - pretty sure that repeat_elements stuff happens before validation as well. (I'm talking about SERVER validation here, not the client side stuff, which masks the problem 99% of the time I think).

        Show
        Penny Leach added a comment - Sorry, but IMO all of formslib is a big hack And Tim - pretty sure that repeat_elements stuff happens before validation as well. (I'm talking about SERVER validation here, not the client side stuff, which masks the problem 99% of the time I think).
        Hide
        Penny Leach added a comment -

        heh, the patch was bad - it should have:

        return $this->moodle_validate();

        in it. rather than just

        $this->moodle_validate();

        Show
        Penny Leach added a comment - heh, the patch was bad - it should have: return $this->moodle_validate(); in it. rather than just $this->moodle_validate();
        Hide
        Penny Leach added a comment -

        Tim, after talking to Jamie we thought the best thing would be to add the hook into question code, rather than messing with the forms lib. I agree although I'm not convinced it will solve the problem and I don't want to have to mess with forms again!

        But if you commit the hook I will happily port my stuff over to it (this is the matrix qtype and also the flash drag&drop one I'm about to start working on with Jamie.)

        Show
        Penny Leach added a comment - Tim, after talking to Jamie we thought the best thing would be to add the hook into question code, rather than messing with the forms lib. I agree although I'm not convinced it will solve the problem and I don't want to have to mess with forms again! But if you commit the hook I will happily port my stuff over to it (this is the matrix qtype and also the flash drag&drop one I'm about to start working on with Jamie.)
        Hide
        Tim Hunt added a comment -

        Penny, is there a patch to question.php that does what you want?

        Show
        Tim Hunt added a comment - Penny, is there a patch to question.php that does what you want?
        Hide
        Penny Leach added a comment -

        course not. If you're happy with the approach I'll make one.

        Show
        Penny Leach added a comment - course not. If you're happy with the approach I'll make one.
        Hide
        Tim Hunt added a comment -

        Ah, I see, that is what you were asking. Yes, I am happy with the general approach given the above arguments. Please go ahead and make a patch.

        Show
        Tim Hunt added a comment - Ah, I see, that is what you were asking. Yes, I am happy with the general approach given the above arguments. Please go ahead and make a patch.
        Hide
        Penny Leach added a comment -

        Jamie - the docs don't elaborate on how to detect which nosubmit button was pressed - is there a better way to do this than look in optional_param (which is what repeat_elements does) ?

        Show
        Penny Leach added a comment - Jamie - the docs don't elaborate on how to detect which nosubmit button was pressed - is there a better way to do this than look in optional_param (which is what repeat_elements does) ?
        Hide
        Penny Leach added a comment -

        I don't think adding a hook in question/question.php solves this problem. The reason is that the validate function returns if it was a no submit button.

        That's the exact problem I was trying to get around - I don't want to make the action happen that the non submit button was pressed, if the form doesn't validate.

        For example, think of the matrix qtype. The user adds rows and colums, and then hits, 'add grades' or whatever the button is called, to assign the correct answers to each cell.

        The add grades button is a nosubmit button, and the action is to make some new form elements so they can select which cells are correct. However, in order for this to make sense, the form must validate - in this case, there must be at least enough rows and columns to make the matrix.

        If you look at the is_validated method in formslib, it

        a) calls definition_after_data FIRST
        b) returns if a no submit button was pressed.

        I want almost the exact opposite:

        a) validate the form whether there was a no submit button or not
        b) call definition_after_data ONLY IF the form was validated.

        Using no_submit_button_pressed in question/question.php doesn't solve either of those problems. It's only a more elegant way to do if (optional_param('addgrades')) in the form definition, as far as I can tell.

        Show
        Penny Leach added a comment - I don't think adding a hook in question/question.php solves this problem. The reason is that the validate function returns if it was a no submit button. That's the exact problem I was trying to get around - I don't want to make the action happen that the non submit button was pressed, if the form doesn't validate. For example, think of the matrix qtype. The user adds rows and colums, and then hits, 'add grades' or whatever the button is called, to assign the correct answers to each cell. The add grades button is a nosubmit button, and the action is to make some new form elements so they can select which cells are correct. However, in order for this to make sense, the form must validate - in this case, there must be at least enough rows and columns to make the matrix. If you look at the is_validated method in formslib, it a) calls definition_after_data FIRST b) returns if a no submit button was pressed. I want almost the exact opposite: a) validate the form whether there was a no submit button or not b) call definition_after_data ONLY IF the form was validated. Using no_submit_button_pressed in question/question.php doesn't solve either of those problems. It's only a more elegant way to do if (optional_param('addgrades')) in the form definition, as far as I can tell.
        Hide
        Petr Škoda added a comment -

        oh, definition_after_data() is supposed to be executed BEFORE the validation, it does not make much sense to call it before because definition_after_data() may set up the validation rules

        Show
        Petr Škoda added a comment - oh, definition_after_data() is supposed to be executed BEFORE the validation, it does not make much sense to call it before because definition_after_data() may set up the validation rules
        Hide
        Penny Leach added a comment -

        Right. So is there a way to do what I want?

        Show
        Penny Leach added a comment - Right. So is there a way to do what I want?
        Hide
        Penny Leach added a comment -

        (or can we please just apply the patch?) (a fixed version)

        Show
        Penny Leach added a comment - (or can we please just apply the patch?) (a fixed version)
        Hide
        Petr Škoda added a comment -

        my -1 for the patch, I do not think it is a good practice to call validate before the definition_after_data() even in case of no submit button
        my +1 for complete rewrite of formslib in 2.0

        Show
        Petr Škoda added a comment - my -1 for the patch, I do not think it is a good practice to call validate before the definition_after_data() even in case of no submit button my +1 for complete rewrite of formslib in 2.0
        Hide
        Penny Leach added a comment -

        Petr, did you actually read the patch? It doesn't change existing behaviour AT ALL. It just separates the logic into two functions, so that custom question types can call a separate function if they want to.

        Just saying 'it will be better in 2.0' is not actually good enough for those of us who need working solutions in 1.9 for the next year a minimum.

        Show
        Penny Leach added a comment - Petr, did you actually read the patch? It doesn't change existing behaviour AT ALL. It just separates the logic into two functions, so that custom question types can call a separate function if they want to. Just saying 'it will be better in 2.0' is not actually good enough for those of us who need working solutions in 1.9 for the next year a minimum.
        Hide
        Tim Hunt added a comment -

        With one change, +1 from me for this patch.

        The change I want is that moodle_validate is a crap function name. When I am looking at the code and trying to work out the difference between is_validated and moodle_validate, I have no idea what the difference is.

        validate_defined_fields is the best suggestion for a method name that I can think of. Also, please write a phpdoc comment explaining what this method does, and when you should use it, and when you should use is_validated instead.

        Then +1 from me for committing it, as a reasonable workaround until Petr rewrites fromslib.

        Show
        Tim Hunt added a comment - With one change, +1 from me for this patch. The change I want is that moodle_validate is a crap function name. When I am looking at the code and trying to work out the difference between is_validated and moodle_validate, I have no idea what the difference is. validate_defined_fields is the best suggestion for a method name that I can think of. Also, please write a phpdoc comment explaining what this method does, and when you should use it, and when you should use is_validated instead. Then +1 from me for committing it, as a reasonable workaround until Petr rewrites fromslib.
        Hide
        Penny Leach added a comment -

        Fair call Tim. I'll work something up shortly. Cheers for your review.

        Show
        Penny Leach added a comment - Fair call Tim. I'll work something up shortly. Cheers for your review.
        Hide
        Penny Leach added a comment -

        It's so nice to be able to close a bug that's called "formslib makes my head catch fire with rage"!

        Tim, I used your function name and wrote some rather verbose phpdocs about when this function might be used.

        Cheers everyone for your input.

        Show
        Penny Leach added a comment - It's so nice to be able to close a bug that's called "formslib makes my head catch fire with rage"! Tim, I used your function name and wrote some rather verbose phpdocs about when this function might be used. Cheers everyone for your input.
        Hide
        Petr Škoda added a comment -

        today is Tuesday - The Review Day, we are not supposed to commit anything into STABLE unless it is a regression from last week

        Show
        Petr Škoda added a comment - today is Tuesday - The Review Day, we are not supposed to commit anything into STABLE unless it is a regression from last week
        Hide
        Penny Leach added a comment -

        Sorry, yesterday was a holiday here which made me think it was Monday. Feel free to back it out and I'll commit it tomorrow.

        Show
        Penny Leach added a comment - Sorry, yesterday was a holiday here which made me think it was Monday. Feel free to back it out and I'll commit it tomorrow.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: