Details

    • Type: Sub-task
    • Status: Closed
    • Priority: 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

      Description

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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment -

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

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

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

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

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

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Petr - this has been integrated now. Definitely for the better and hopefully nothing broken. Cheers Sam
            Hide
            samhemelryk 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
            samhemelryk 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
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - hi, I am going to review all exportValue()s asap and fix the test case.
            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            nebgor 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
            nebgor 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
            samhemelryk Sam Hemelryk added a comment -

            Awesome thanks guys, the patch has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Awesome thanks guys, the patch has been integrated now
            Hide
            nebgor 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
            nebgor 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
            rajeshtaneja 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
            rajeshtaneja 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
            samhemelryk 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
            samhemelryk 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:
                  Fix Release Date:
                  25/Jun/12