Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.2
    • Fix Version/s: 2.3
    • Component/s: General
    • Labels:
    • Testing Instructions:
      Hide

      1/ enable strict mode in php.ini
      2/ force strict mode in main config.php
      3/ browse around a bit

      there are too many areas affected, detailed code review should be enough I guess

      Show
      1/ enable strict mode in php.ini 2/ force strict mode in main config.php 3/ browse around a bit there are too many areas affected, detailed code review should be enough I guess
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w12_MDL-32094_m23_phpstrict
    • Rank:
      38798

      Description

      it is not enough to enable E_STRICT in config.php only

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          some E_STRICT (and warnings in PHP 5.4), it is most probably not complete yet

          Show
          Petr Škoda added a comment - some E_STRICT (and warnings in PHP 5.4), it is most probably not complete yet
          Hide
          Petr Škoda added a comment -

          if necessary please add info about other strict problems in MDL-32095

          Show
          Petr Škoda added a comment - if necessary please add info about other strict problems in MDL-32095
          Hide
          Sam Hemelryk added a comment -

          Thanks Petr - this has been integrated now. Definitely for the better and hopefully nothing broken.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Petr - this has been integrated now. Definitely for the better and hopefully nothing broken. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Hi Petr,

          Found the first bug with these changes.
          Unit tests are failing horribly for me after a bit of research I've tracked it back the duration forms element and have found that reverting one of the changes you made fixes the issue:

          
          diff --git a/lib/form/duration.php b/lib/form/duration.php
          index 06cd85a..e9fc7a4 100644
          --- a/lib/form/duration.php
          +++ b/lib/form/duration.php
          @@ -229,7 +229,7 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group {
                * @param  bool  $notused Not used.
                * @return array field name => value. The value is the time interval in seconds.
                */
          -    function exportValue(&$submitValues, $notused = false) {
          +    function exportValue($submitValues, $notused = false) {
                   // Get the values from all the child elements.
                   $valuearray = array();
                   foreach ($this->_elements as $element) {
          
          

          With that patch applied things work perfectly again, however before that I get first an exceeded memory limit, and then when I raise the memory limit I get the following error:

          Fatal error: Cannot pass parameter 1 by reference in /var/www/integration/lib/form/simpletest/testduration.php on line 110

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Petr, Found the first bug with these changes. Unit tests are failing horribly for me after a bit of research I've tracked it back the duration forms element and have found that reverting one of the changes you made fixes the issue: diff --git a/lib/form/duration.php b/lib/form/duration.php index 06cd85a..e9fc7a4 100644 --- a/lib/form/duration.php +++ b/lib/form/duration.php @@ -229,7 +229,7 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { * @param bool $notused Not used. * @ return array field name => value. The value is the time interval in seconds. */ - function exportValue(&$submitValues, $notused = false ) { + function exportValue($submitValues, $notused = false ) { // Get the values from all the child elements. $valuearray = array(); foreach ($ this ->_elements as $element) { With that patch applied things work perfectly again, however before that I get first an exceeded memory limit, and then when I raise the memory limit I get the following error: Fatal error: Cannot pass parameter 1 by reference in /var/www/integration/lib/form/simpletest/testduration.php on line 110 Cheers Sam
          Hide
          Petr Škoda added a comment -

          hi, I am going to review all exportValue()s asap and fix the test case.

          Show
          Petr Škoda added a comment - hi, I am going to review all exportValue()s asap and fix the test case.
          Hide
          Petr Škoda added a comment -

          here is the fix: https://github.com/skodak/moodle/commits/w12_MDL-32094_integration_fix
          please merge it to master integration branch

          Show
          Petr Škoda added a comment - here is the fix: https://github.com/skodak/moodle/commits/w12_MDL-32094_integration_fix please merge it to master integration branch
          Hide
          Petr Škoda added a comment -

          Aparup told me that his test runs were running out of 134MB available memory, but the admin/tool/unittest/index.php explicitly sets memory limit to MEMORY_EXTRA which is 256MB on 32bits (and Windows) and 384 on 64bit servers. There seems to be something wrong going on, if you run out of memory please verify moodle can actually change memory limits or manually set higher limits in PHP.ini

          Show
          Petr Škoda added a comment - Aparup told me that his test runs were running out of 134MB available memory, but the admin/tool/unittest/index.php explicitly sets memory limit to MEMORY_EXTRA which is 256MB on 32bits (and Windows) and 384 on 64bit servers. There seems to be something wrong going on, if you run out of memory please verify moodle can actually change memory limits or manually set higher limits in PHP.ini
          Hide
          Aparup Banerjee added a comment -

          Hi, just noting here (for Petr) that i haven't merged w12_MDL-32094_integration_fix

          It looks like a refactor - so not sure what is being fixed there. This is better left to issue integrator - Sam. (also lesser reverts)

          Show
          Aparup Banerjee added a comment - Hi, just noting here (for Petr) that i haven't merged w12_ MDL-32094 _integration_fix It looks like a refactor - so not sure what is being fixed there. This is better left to issue integrator - Sam. (also lesser reverts)
          Hide
          Sam Hemelryk added a comment -

          Awesome thanks guys, the patch has been integrated now

          Show
          Sam Hemelryk added a comment - Awesome thanks guys, the patch has been integrated now
          Hide
          Aparup Banerjee added a comment -

          note:

          //$CFG->debug = 38911;
          $CFG->debug = E_ALL | E_STRICT ; //32767
          is breaking css whereever a page has a problem.

          Show
          Aparup Banerjee added a comment - note: //$CFG->debug = 38911; $CFG->debug = E_ALL | E_STRICT ; //32767 is breaking css whereever a page has a problem.
          Hide
          Rajesh Taneja added a comment - - edited

          Not sure, what should be tested here. The whole site is full with notices.
          Although, two issues might be of interest:

          1. http://tracker.moodle.org/browse/MDL-32095?focusedCommentId=149562&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-149562
          2. notices are visible twice, one on top (before the site is rendered) and same gets repeated in moodle site (within page).

          Findings:

          1. With strict set in php.ini and config.php -> Whole site is full of notices
          2. With strict set in php.ini and not in config.php -> Site works as usual (with developer debug as well, no notices)
          3. With strict not set in php.ini and not in config.php -> Site works as usual (with developer debug as well, no notices)

          Passing the test ... Hoping they are supposed to be fixed in part 2/ MDL-32095

          Show
          Rajesh Taneja added a comment - - edited Not sure, what should be tested here. The whole site is full with notices. Although, two issues might be of interest: http://tracker.moodle.org/browse/MDL-32095?focusedCommentId=149562&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-149562 notices are visible twice, one on top (before the site is rendered) and same gets repeated in moodle site (within page). Findings: With strict set in php.ini and config.php -> Whole site is full of notices With strict set in php.ini and not in config.php -> Site works as usual (with developer debug as well, no notices) With strict not set in php.ini and not in config.php -> Site works as usual (with developer debug as well, no notices) Passing the test ... Hoping they are supposed to be fixed in part 2/ MDL-32095
          Hide
          Sam Hemelryk added a comment -

          Congratulations are in order, you've made it, or at least your code has!
          It's now part of Moodle and both the git and cvs repositories have been updated.

          This issue is being marked as fixed and closed.

          Thank you.

          Show
          Sam Hemelryk added a comment - Congratulations are in order, you've made it, or at least your code has! It's now part of Moodle and both the git and cvs repositories have been updated. This issue is being marked as fixed and closed. Thank you.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: