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

          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