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

          Attachments

            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