Moodle
  1. Moodle
  2. MDL-31055

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

    Details

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

      Description

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

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

        Issue Links

          Activity

          Hide
          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
          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
          Tim Gus added a comment -

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

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

          Thanks. Submitting for integration.

          Show
          Tim Hunt added a comment - Thanks. Submitting for integration.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Tim Hunt added a comment -

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

          Show
          Tim Hunt added a comment - Ah, thanks for explaining. Is the integration bible public?
          Hide
          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
          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
          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
          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 Agarwal added a comment -

          attaching the test script from the linked issue

          Show
          Ankit Agarwal added a comment - attaching the test script from the linked issue
          Hide
          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 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
          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
          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 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 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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - +1 to revert this (if it does nothing, better keep it out).
          Hide
          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
          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
          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
          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
          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
          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: