Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1
    • Fix Version/s: 2.4
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide
      1. grep "addElement('file'"
      2. grep "createElement('file'"
      3. grep MoodleQuickForm_format
      4. grep "addElement('format'"
      5. grep "createElement('format'"
      6. grep setHelpButton
        Make sure none of the above are used.

      navigate pages with forms and make sure forms work fine, especially elements with help button.

      NOTE:
      sethelpButton is left in hidden.php and editor.php, make sure they have debug message.

      Show
      grep "addElement('file'" grep "createElement('file'" grep MoodleQuickForm_format grep "addElement('format'" grep "createElement('format'" grep setHelpButton Make sure none of the above are used. navigate pages with forms and make sure forms work fine, especially elements with help button. NOTE: sethelpButton is left in hidden.php and editor.php, make sure they have debug message.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-mdl-31294
    • Rank:
      37759

      Description

      Remove deprecated functions in lib/formslib.php

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment - - edited

          lib/form/format.php and lib/form/file.php is also deprecated and can be removed.

          Show
          Rajesh Taneja added a comment - - edited lib/form/format.php and lib/form/file.php is also deprecated and can be removed.
          Hide
          Petr Škoda added a comment -

          my -1, we should instead create new forms library

          Show
          Petr Škoda added a comment - my -1, we should instead create new forms library
          Hide
          Rajesh Taneja added a comment -

          Will open another issue to consider cleaning old file validation.

          Show
          Rajesh Taneja added a comment - Will open another issue to consider cleaning old file validation.
          Hide
          Frédéric Massart added a comment -

          Hi Raj,

          the code looks good to me and I don't see any reason why this shouldn't go for integration. This only thing that came in my mind was the two files that were removed. As some developers might not have thought that they'd be deleted, it could brake their codes if they used an include()/require() on them. But, I don't see any reason for them to manually include those files so... I guess we're all good !

          Show
          Frédéric Massart added a comment - Hi Raj, the code looks good to me and I don't see any reason why this shouldn't go for integration. This only thing that came in my mind was the two files that were removed. As some developers might not have thought that they'd be deleted, it could brake their codes if they used an include()/require() on them. But, I don't see any reason for them to manually include those files so... I guess we're all good !
          Hide
          Rajesh Taneja added a comment -

          Thanks Fred,

          Pushing it for integration review.
          Well, if we are deleting class then I don't see any reason to keep files, as they will also break there code.

          Show
          Rajesh Taneja added a comment - Thanks Fred, Pushing it for integration review. Well, if we are deleting class then I don't see any reason to keep files, as they will also break there code.
          Hide
          Aparup Banerjee added a comment -

          Hi Raj,
          I was a little confused about this initially but luckily from lib/form/group.php the @todo MDL-31047 has shed some background on this issue. i've linked one meta-ish issue and closed duplicates.

          • given that theres been some calls where we've been throwing debugging messages to the developer for awhile , those should be fine to remove. I hope and guess that they've been throwing that in MOODLE_23_STABLE. However there are some functions which currently don't seem to be throwing any (non-phpdoc) warnings to the developer.
            • lib/form/editor.php , lib/form/hidden.php have setHelpButton() being removed without any debugging() or exceptions.
            • lib/form/hidden.php removes getHelpButton() which didn't have any deprecation warning (debugging() call). i know it returns '' but it may not be noticeable like that .
            • MoodleQuickForm_format - its weird it was only throwing an exception and not really mentioning the word 'deprecated' but i guess that's effectively stopped its usage

          lets polish this to make this transition flawless for other innocent and unsuspecting developers out there

          Show
          Aparup Banerjee added a comment - Hi Raj, I was a little confused about this initially but luckily from lib/form/group.php the @todo MDL-31047 has shed some background on this issue. i've linked one meta-ish issue and closed duplicates. given that theres been some calls where we've been throwing debugging messages to the developer for awhile , those should be fine to remove. I hope and guess that they've been throwing that in MOODLE_23_STABLE. However there are some functions which currently don't seem to be throwing any (non-phpdoc) warnings to the developer. lib/form/editor.php , lib/form/hidden.php have setHelpButton() being removed without any debugging() or exceptions. lib/form/hidden.php removes getHelpButton() which didn't have any deprecation warning (debugging() call). i know it returns '' but it may not be noticeable like that . MoodleQuickForm_format - its weird it was only throwing an exception and not really mentioning the word 'deprecated' but i guess that's effectively stopped its usage lets polish this to make this transition flawless for other innocent and unsuspecting developers out there
          Hide
          Rajesh Taneja added a comment -

          Thanks Aparup,

          I have added debugging message in lib/form/editor.php and lib/form/hidden.php.
          Also, restored getHelpButton. I don't see any reason of them being there because in lib/formslib.php

          if (method_exists($element, 'getHelpButton')){
              $html = str_replace('{help}', element->getHelpButton(), $html);
          }else{
              $html = str_replace('{help}', '', $html);
          }
          

          Anyway, leaving them for now to avoid any regression.

          Show
          Rajesh Taneja added a comment - Thanks Aparup, I have added debugging message in lib/form/editor.php and lib/form/hidden.php. Also, restored getHelpButton. I don't see any reason of them being there because in lib/formslib.php if (method_exists($element, 'getHelpButton')){ $html = str_replace('{help}', element->getHelpButton(), $html); }else{ $html = str_replace('{help}', '', $html); } Anyway, leaving them for now to avoid any regression.
          Hide
          Aparup Banerjee added a comment -

          Thanks Raj,
          that's been integrated into master.
          Petr has mentioned a new forms lib? please link if there is any issue.

          Tester: please be trigger happy with testing forms.

          noting that the new deprecation message was updated to point to addHelpButton(). i suppose this isn't an issue as any dev would've seen that the old$mform->setHelpButton() was also properly deprecated. (assuming it was deprecated according to due process with debugging etc)

          Show
          Aparup Banerjee added a comment - Thanks Raj, that's been integrated into master. Petr has mentioned a new forms lib? please link if there is any issue. Tester: please be trigger happy with testing forms. noting that the new deprecation message was updated to point to addHelpButton(). i suppose this isn't an issue as any dev would've seen that the old$mform->setHelpButton() was also properly deprecated. (assuming it was deprecated according to due process with debugging etc)
          Hide
          Dan Poltawski added a comment -

          Does this need a mention in upgrade.txt? (/api_change tag)

          Show
          Dan Poltawski added a comment - Does this need a mention in upgrade.txt? (/api_change tag)
          Hide
          Aparup Banerjee added a comment -

          Not that I can think of. The API would have. Changed awhile ago. Deprecation are generally made aware of by adding deprecation debugging messages. (grr iPhone + Kira commenting is bad)

          Show
          Aparup Banerjee added a comment - Not that I can think of. The API would have. Changed awhile ago. Deprecation are generally made aware of by adding deprecation debugging messages. (grr iPhone + Kira commenting is bad)
          Hide
          Rajesh Taneja added a comment -

          Ankit just mentioned about phpdoc in hidden.php and editor.php points to setHelpButton.

          @Aparup: Can you please pick one extra commit which change this to addHelpButton.

          Thanks for pointing this Ankit.

          Show
          Rajesh Taneja added a comment - Ankit just mentioned about phpdoc in hidden.php and editor.php points to setHelpButton. @Aparup: Can you please pick one extra commit which change this to addHelpButton. Thanks for pointing this Ankit.
          Hide
          Aparup Banerjee added a comment -

          nice catch Ankit, thanks. that's been picked into master now.

          Show
          Aparup Banerjee added a comment - nice catch Ankit, thanks. that's been picked into master now.
          Hide
          Ankit Agarwal added a comment -

          Thanks for the fix.
          everything seems to be working as expected and I cannot find any instances of the removed code.
          Passing
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks for the fix. everything seems to be working as expected and I cannot find any instances of the removed code. Passing Thanks
          Hide
          Aparup Banerjee added a comment -

          yay, it works!

          This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

          Thank you all for taking the time to get us here.

          cheers!

          Show
          Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: