Details

    • Type: Sub-task
    • Status: Closed
    • Priority: 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

      Description

      Remove deprecated functions in lib/formslib.php

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              rajeshtaneja Rajesh Taneja added a comment - - edited

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

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

              my -1, we should instead create new forms library

              Show
              skodak Petr Skoda added a comment - my -1, we should instead create new forms library
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Will open another issue to consider cleaning old file validation.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Will open another issue to consider cleaning old file validation.
              Hide
              fred 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
              fred 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
              rajeshtaneja 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
              rajeshtaneja 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
              nebgor 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
              nebgor 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
              rajeshtaneja 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
              rajeshtaneja 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
              nebgor 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
              nebgor 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
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski Dan Poltawski added a comment - Does this need a mention in upgrade.txt? (/api_change tag)
              Hide
              nebgor 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
              nebgor 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
              rajeshtaneja 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
              rajeshtaneja 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
              nebgor Aparup Banerjee added a comment -

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

              Show
              nebgor Aparup Banerjee added a comment - nice catch Ankit, thanks. that's been picked into master now.
              Hide
              ankit_frenz 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_frenz 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
              nebgor 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
              nebgor 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!
              Hide
              dobedobedoh Andrew Nicols added a comment -

              This missed some of the functions it was meant to deprecate:

              1. save_files
              Show
              dobedobedoh Andrew Nicols added a comment - This missed some of the functions it was meant to deprecate: save_files

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    3/Dec/12