Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31055

Partially backport MDL-27045 to prevent some PHP warnings/notices

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.16
    • Fix Version/s: 1.9.17
    • Component/s: Forms Library
    • Labels:

      Description

      Tim Gus did the hard work, so I am going to submit this for integration.

        Gliffy Diagrams

        1. checkboxtest_19.php
          1 kB
          Tim Hunt
        2. checkboxtest.php
          1 kB
          Ankit Agarwal

          Issue Links

            Activity

            Hide
            timhunt Tim Hunt added a comment -

            Tim, can you have a quick look and verify that I have correctly transferred your patch into git, before I submit it for integration.

            In particular, please can you confirm that I got the author information on the commit correct. Thanks.

            Show
            timhunt Tim Hunt added a comment - Tim, can you have a quick look and verify that I have correctly transferred your patch into git, before I submit it for integration. In particular, please can you confirm that I got the author information on the commit correct. Thanks.
            Hide
            tgus Tim Gus added a comment -

            The patch from github looks correctly transferred as well as the author information.

            Show
            tgus Tim Gus added a comment - The patch from github looks correctly transferred as well as the author information.
            Hide
            timhunt Tim Hunt added a comment -

            Thanks. Submitting for integration.

            Show
            timhunt Tim Hunt added a comment - Thanks. Submitting for integration.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            nebgor Aparup Banerjee added a comment -

            thanks, i've integrated this into 1.9.x

            (amended the git message to have this MDL issue in the beginning of the message.)

            Show
            nebgor Aparup Banerjee added a comment - thanks, i've integrated this into 1.9.x (amended the git message to have this MDL issue in the beginning of the message.)
            Hide
            timhunt Tim Hunt added a comment -

            Grrr! I intentionally did not amend the commit message, so that all the fixes for MDL-27045 had the same commit comment. This issues was just about the task of back-porting that previous issue to one more branch. This is not really the issue that is being fixed. I am sure the commit number has not been amended in other situations where a fix was first applied to one branch, and later back-ported to more. Perhaps you integrators would like to discuss this amongst yourselves, and agree what the policy is.

            Show
            timhunt Tim Hunt added a comment - Grrr! I intentionally did not amend the commit message, so that all the fixes for MDL-27045 had the same commit comment. This issues was just about the task of back-porting that previous issue to one more branch. This is not really the issue that is being fixed. I am sure the commit number has not been amended in other situations where a fix was first applied to one branch, and later back-ported to more. Perhaps you integrators would like to discuss this amongst yourselves, and agree what the policy is.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I think we are more or less used to (basic):

            MDL-31055 Backport MDL-27045 to 19_STABLE

            (or similars, with complete description, but always with 1st issue being the current one, not the original to be backported. That goes to the description)

            So I think Apu did ok. Ciao

            PS: Adding this to the integration bible

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I think we are more or less used to (basic): MDL-31055 Backport MDL-27045 to 19_STABLE (or similars, with complete description, but always with 1st issue being the current one, not the original to be backported. That goes to the description) So I think Apu did ok. Ciao PS: Adding this to the integration bible
            Hide
            timhunt Tim Hunt added a comment -

            Ah, thanks for explaining. Is the integration bible public?

            Show
            timhunt Tim Hunt added a comment - Ah, thanks for explaining. Is the integration bible public?
            Hide
            nebgor Aparup Banerjee added a comment -

            Tim, spoke briefly to Eloy and agreed that ' MDL-31055 backport disabledIf fix from 2.0. (MDL-27045)' was towards what we want. The direct MDL issue bringing this commit from patch to into moodle.git is first, the rest can be left to description.

            i'm not sure of the other situations but if there is supposed to be another convention that would be preferred, it should be agreed and documented.

            I'm hoping things would be clarified further when we've got integration docs.

            Show
            nebgor Aparup Banerjee added a comment - Tim, spoke briefly to Eloy and agreed that ' MDL-31055 backport disabledIf fix from 2.0. ( MDL-27045 )' was towards what we want. The direct MDL issue bringing this commit from patch to into moodle.git is first, the rest can be left to description. i'm not sure of the other situations but if there is supposed to be another convention that would be preferred, it should be agreed and documented. I'm hoping things would be clarified further when we've got integration docs.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            I'm aiming to have the very 1st version public available along January. Till now, all I've is a loooong list of comments/habits/notes... stay tuned.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited I'm aiming to have the very 1st version public available along January. Till now, all I've is a loooong list of comments/habits/notes... stay tuned. Ciao
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            attaching the test script from the linked issue

            Show
            ankit_frenz Ankit Agarwal added a comment - attaching the test script from the linked issue
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            Hi Tim,
            Am getting the following error when I try to run the test script (with error reporting set to E_ALL)
            <code>
            Notice: Undefined variable: PAGE in H:\wamp\www\repo2\MOODLE_19_STABLE\moodle\checkboxtest.php on line 35

            ( ! ) Fatal error: Call to a member function set_url() on a non-object in H:\wamp\www\repo2\MOODLE_19_STABLE\moodle\checkboxtest.php on line 35
            </code>

            My guess is the test script was not designed for 1.9? waiting for further test instructions on this.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited Hi Tim, Am getting the following error when I try to run the test script (with error reporting set to E_ALL) <code> Notice: Undefined variable: PAGE in H:\wamp\www\repo2\MOODLE_19_STABLE\moodle\checkboxtest.php on line 35 ( ! ) Fatal error: Call to a member function set_url() on a non-object in H:\wamp\www\repo2\MOODLE_19_STABLE\moodle\checkboxtest.php on line 35 </code> My guess is the test script was not designed for 1.9? waiting for further test instructions on this. Thanks
            Hide
            timhunt Tim Hunt added a comment -

            Indeed, the test script was designed for 2.x.

            Attached is a version on the script that works in 1.9.

            Show
            timhunt Tim Hunt added a comment - Indeed, the test script was designed for 2.x. Attached is a version on the script that works in 1.9.
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            Hi Tim,
            This is what I am getting
            With checkbox ticked : - + - - +

            with checkbox unticked: - + - + -

            which is different from the expected result as stated in the linked issue

            Verify that the following pattern of enabled/disabled text fields is shown (with + meaning "enabled" and - meaning "greyed out"):
             
            When checkbox is ticked: + - + - +
            When checkbox is not ticked: - + - + -

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited Hi Tim, This is what I am getting With checkbox ticked : - + - - + with checkbox unticked: - + - + - which is different from the expected result as stated in the linked issue Verify that the following pattern of enabled/disabled text fields is shown (with + meaning "enabled" and - meaning "greyed out"):   When checkbox is ticked: + - + - + When checkbox is not ticked: - + - + -
            Hide
            timhunt Tim Hunt added a comment -

            Oh dear. And in fact, what you are seeing is exactly the same as what I saw when I was fixing the test script, on an older version of 1.9, so the patch here seems to have no effect.

            Still, it also does no harm

            Well, it has not effect on the JavaScript, it does fix PHP notices.

            I suggest we just leave this. We only integrated this change because someone in the community submitted it, and it can only make things better. The fact it does not make things perfect, well ... this is 1.9, we are not really supporting it.

            Show
            timhunt Tim Hunt added a comment - Oh dear. And in fact, what you are seeing is exactly the same as what I saw when I was fixing the test script, on an older version of 1.9, so the patch here seems to have no effect. Still, it also does no harm Well, it has not effect on the JavaScript, it does fix PHP notices. I suggest we just leave this. We only integrated this change because someone in the community submitted it, and it can only make things better. The fact it does not make things perfect, well ... this is 1.9, we are not really supporting it.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            +1 to revert this (if it does nothing, better keep it out).

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - +1 to revert this (if it does nothing, better keep it out).
            Hide
            timhunt Tim Hunt added a comment -

            The change does something. It fixes a PHP warnings:

            Warning: Missing argument 1 for HTML_QuickForm_advcheckbox::getPrivateName(), called in /fs1/www_root/tjh238/moodle_head/lib/formslib.php on line XXXX and defined in /fs1/www_root/tjh238/moodle_head/lib/pear/HTML/QuickForm/advcheckbox.php on line XX

            Notice: Undefined variable: elementName in /fs1/www_root/tjh238/moodle_head/lib/pear/HTML/QuickForm/advcheckbox.php on line XXX

            Show
            timhunt Tim Hunt added a comment - The change does something. It fixes a PHP warnings: Warning: Missing argument 1 for HTML_QuickForm_advcheckbox::getPrivateName(), called in /fs1/www_root/tjh238/moodle_head/lib/formslib.php on line XXXX and defined in /fs1/www_root/tjh238/moodle_head/lib/pear/HTML/QuickForm/advcheckbox.php on line XX Notice: Undefined variable: elementName in /fs1/www_root/tjh238/moodle_head/lib/pear/HTML/QuickForm/advcheckbox.php on line XXX
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Oki, I'm going to pass this but:

            • rewording a bit the title ("partially backport MDL-27045 to prevent some PHP warnings/notices").
            • noting that this should not have been backported ever. 19_STABLE normal support is over since > 7 months ago. Only security support is enabled till June 2012 (we don't need the bible for that).

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Oki, I'm going to pass this but: rewording a bit the title ("partially backport MDL-27045 to prevent some PHP warnings/notices"). noting that this should not have been backported ever. 19_STABLE normal support is over since > 7 months ago. Only security support is enabled till June 2012 (we don't need the bible for that). Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            This is now available in the git and cvs repositories.

            Consider the responsibility of your fingerprints engraved there for future generations!

            Thanks for the work, closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - This is now available in the git and cvs repositories. Consider the responsibility of your fingerprints engraved there for future generations! Thanks for the work, closing, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Mar/12