Moodle
  1. Moodle
  2. MDL-31167 PHP strict META
  3. MDL-12730

include E_STRICT in DEBUG_DEVELOPER starting in 2.3

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.2
    • Fix Version/s: 2.3
    • Component/s: Administration
    • Labels:
    • Rank:
      1252

      Description

      starting with 2.0 only PHP5 supported - yay!
      we sure want to fix all strict errors too, right?

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          hmmmm, not yet...

          Show
          Petr Škoda added a comment - hmmmm, not yet...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For reference, some bits happening right now under 2.0.2+ and E_STRICT:

          Strict standards: Declaration of ReferenceExpectation::test() should be compatible with that of SimpleExpectation::test() in /Users/stronk7/git_moodle/moodle/lib/simpletestlib/test_case.php on line 17
          
          Strict standards: Declaration of SimpleImageSubmitTag::write() should be compatible with that of SimpleWidget::write() in /Users/stronk7/git_moodle/moodle/lib/simpletestlib/page.php on line 14
          
          Strict standards: Non-static method PEAR::setErrorHandling() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 63
          
          Strict standards: Non-static method HTML_QuickForm::registerElementType() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 2337
          
          Strict standards: Non-static method HTML_QuickForm::registerElementType() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 2338
          
          
          Strict standards: Non-static method HTML_QuickForm::registerElementType() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 2339
          
          Strict standards: Non-static method HTML_QuickForm::registerElementType() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 2340
          
          Strict standards: Non-static method HTML_QuickForm::registerElementType() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 2341
          
          Strict standards: Non-static method HTML_QuickForm::registerElementType() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 2342
          ...
          ...
          
          Strict standards: Declaration of repository_instance_form::validation() should be compatible with that of moodleform::validation() in /Users/stronk7/git_moodle/moodle/repository/lib.php on line 1578
          
          Strict standards: Declaration of repository_local::get_listing() should be compatible with that of repository::get_listing() in /Users/stronk7/git_moodle/moodle/repository/lib.php on line 825
          
          Strict standards: Declaration of repository_recent::type_config_form() should be compatible with that of repository::type_config_form() in /Users/stronk7/git_moodle/moodle/repository/lib.php on line 825
          
          Strict standards: Declaration of auth_plugin_manual::user_confirm() should be compatible with that of auth_plugin_base::user_confirm() in /Users/stronk7/git_moodle/moodle/auth/manual/auth.php on line 3
          
          Strict standards: Creating default object from empty value in /Users/stronk7/git_moodle/moodle/blocks/navigation/block_navigation.php on line 197
          
          Strict standards: Creating default object from empty value in /Users/stronk7/git_moodle/moodle/blocks/settings/block_settings.php on line 132
          

          So:

          1. One more reason to switch from simpletest to phpunit
          2. formslib, lalala
          3. other minor bits here and there

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For reference, some bits happening right now under 2.0.2+ and E_STRICT: Strict standards: Declaration of ReferenceExpectation::test() should be compatible with that of SimpleExpectation::test() in /Users/stronk7/git_moodle/moodle/lib/simpletestlib/test_case.php on line 17 Strict standards: Declaration of SimpleImageSubmitTag::write() should be compatible with that of SimpleWidget::write() in /Users/stronk7/git_moodle/moodle/lib/simpletestlib/page.php on line 14 Strict standards: Non- static method PEAR::setErrorHandling() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 63 Strict standards: Non- static method HTML_QuickForm::registerElementType() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 2337 Strict standards: Non- static method HTML_QuickForm::registerElementType() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 2338 Strict standards: Non- static method HTML_QuickForm::registerElementType() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 2339 Strict standards: Non- static method HTML_QuickForm::registerElementType() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 2340 Strict standards: Non- static method HTML_QuickForm::registerElementType() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 2341 Strict standards: Non- static method HTML_QuickForm::registerElementType() should not be called statically in /Users/stronk7/git_moodle/moodle/lib/formslib.php on line 2342 ... ... Strict standards: Declaration of repository_instance_form::validation() should be compatible with that of moodleform::validation() in /Users/stronk7/git_moodle/moodle/repository/lib.php on line 1578 Strict standards: Declaration of repository_local::get_listing() should be compatible with that of repository::get_listing() in /Users/stronk7/git_moodle/moodle/repository/lib.php on line 825 Strict standards: Declaration of repository_recent::type_config_form() should be compatible with that of repository::type_config_form() in /Users/stronk7/git_moodle/moodle/repository/lib.php on line 825 Strict standards: Declaration of auth_plugin_manual::user_confirm() should be compatible with that of auth_plugin_base::user_confirm() in /Users/stronk7/git_moodle/moodle/auth/manual/auth.php on line 3 Strict standards: Creating default object from empty value in /Users/stronk7/git_moodle/moodle/blocks/navigation/block_navigation.php on line 197 Strict standards: Creating default object from empty value in /Users/stronk7/git_moodle/moodle/blocks/settings/block_settings.php on line 132 So: One more reason to switch from simpletest to phpunit formslib, lalala other minor bits here and there Ciao
          Hide
          Gerard Caulfield added a comment -

          How about creating a higher debug level or an option which displays E_STRICT? If not shouldn't we at least be passing these along to error_log() so that people know of the issue and so that coders are not creating new E_STRICT issues?

          Show
          Gerard Caulfield added a comment - How about creating a higher debug level or an option which displays E_STRICT? If not shouldn't we at least be passing these along to error_log() so that people know of the issue and so that coders are not creating new E_STRICT issues?
          Hide
          Petr Škoda added a comment -

          My current plan is to fix all strict issues in 2.3dev at least because most of them are standard warnings/notices in PHP 5.4. Moodle 2.3dev should imo display all strict problems. thanks for reminding me about this issue, I already forgot it exists...

          Show
          Petr Škoda added a comment - My current plan is to fix all strict issues in 2.3dev at least because most of them are standard warnings/notices in PHP 5.4. Moodle 2.3dev should imo display all strict problems. thanks for reminding me about this issue, I already forgot it exists...
          Hide
          Gerard Caulfield added a comment -

          awesome

          Show
          Gerard Caulfield added a comment - awesome
          Hide
          Petr Škoda added a comment -

          Originally we used numbers because PHP4 did not understand new constants. We use the constants now because future versions of PHP may change their values. The 32768 was originally intended as a "show debugging" flag, but it was later changed to > comparison in debugging(). This comparison is not technically correct, luckily our debug levels are defined in ascending order so that usually works.

          Show
          Petr Škoda added a comment - Originally we used numbers because PHP4 did not understand new constants. We use the constants now because future versions of PHP may change their values. The 32768 was originally intended as a "show debugging" flag, but it was later changed to > comparison in debugging(). This comparison is not technically correct, luckily our debug levels are defined in ascending order so that usually works.
          Hide
          Sam Hemelryk added a comment -

          Hi Petr,

          I've been looking at this just now.
          I've come around to the idea of this, and certainly the sooner we get it in the better.
          However there was one immediate problem that jumped out at me when I started to play with this.
          Because the debug value has changed STRICT notices are not going to be shown for anyone who had turned debugging on prior to this change.
          In order to see those changes they are going to need to edit and re-save their debug setup, or like myself change the value in config.php.

          As said I certainly think this should go in still and will likely integrate shortly anyway so that it is there during today's testing.
          However I think we should consider introducing some upgrade code or at least a notice if the old debug value is being used.
          For reference:

          Debug level Old value New value How it should be set in config.php
          None 0 0 $CFG->debug = 0;
          Minimal 5 5 $CFG->debug = E_ERROR | E_PARSE;
          Normal 15 15 $CFG->debug = E_ERROR | E_PARSE | E_WARNING | E_NOTICE;
          All 6143 30719 $CFG->debug = E_ALL & ~E_STRICT
          Developer 38911 32767 $CFG->debug = E_ALL | E_STRICT

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Petr, I've been looking at this just now. I've come around to the idea of this, and certainly the sooner we get it in the better. However there was one immediate problem that jumped out at me when I started to play with this. Because the debug value has changed STRICT notices are not going to be shown for anyone who had turned debugging on prior to this change. In order to see those changes they are going to need to edit and re-save their debug setup, or like myself change the value in config.php. As said I certainly think this should go in still and will likely integrate shortly anyway so that it is there during today's testing. However I think we should consider introducing some upgrade code or at least a notice if the old debug value is being used. For reference: Debug level Old value New value How it should be set in config.php None 0 0 $CFG->debug = 0; Minimal 5 5 $CFG->debug = E_ERROR | E_PARSE; Normal 15 15 $CFG->debug = E_ERROR | E_PARSE | E_WARNING | E_NOTICE; All 6143 30719 $CFG->debug = E_ALL & ~E_STRICT Developer 38911 32767 $CFG->debug = E_ALL | E_STRICT Cheers Sam
          Hide
          Petr Škoda added a comment -

          adding upgrade step, thanks

          Show
          Petr Škoda added a comment - adding upgrade step, thanks
          Hide
          Petr Škoda added a comment -

          reopening, I got some more E_STRICT problem reports, I will create one more E_STRICT patch and then fix this enabler. Ciao

          Show
          Petr Škoda added a comment - reopening, I got some more E_STRICT problem reports, I will create one more E_STRICT patch and then fix this enabler. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Taking on this....

          Show
          Eloy Lafuente (stronk7) added a comment - Taking on this....
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks! (note I've picked, amended and sent together the 2 original commits, observing your authorship, of course).

          I also tested the admin UI and upgrade and seemed to work ok. Also executed all the unit tests and got two failures there @ lib/simpletest/testoutputlib.php that didn't exist before the patch, so I guess we need to look there. Will do after integrating MDL-32095, just in case it includes the fix.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (note I've picked, amended and sent together the 2 original commits, observing your authorship, of course). I also tested the admin UI and upgrade and seemed to work ok. Also executed all the unit tests and got two failures there @ lib/simpletest/testoutputlib.php that didn't exist before the patch, so I guess we need to look there. Will do after integrating MDL-32095 , just in case it includes the fix. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          As commented above, test passed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - As commented above, test passed. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has landed upstream, finally! Yay!

          תודה רבה && شكرا جزيلا



          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has landed upstream, finally! Yay! תודה רבה && شكرا جزيلا Closing, ciao
          Hide
          Daniele Cordella added a comment -

          Coooooool! Finally. Thanks.

          Show
          Daniele Cordella added a comment - Coooooool! Finally. Thanks.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: