Details

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

      no idea how to test this enough for stable branch, I suppose detailed review is necessary concentrating on two changes in loginpage and formslib

      the notices are visible if you install PHP5.4 and disable E_STRICT, for now it is not possible to test it in PHP 5.3 because there are way too many other strict problems in our code

      Show
      no idea how to test this enough for stable branch, I suppose detailed review is necessary concentrating on two changes in loginpage and formslib the notices are visible if you install PHP5.4 and disable E_STRICT, for now it is not possible to test it in PHP 5.3 because there are way too many other strict problems in our code
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w03_MDL-31006_m23_php54_1
    • Rank:
      37410

      Description

      • creation of object from null
      • invalid offset
      • array to string conversion

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          This issue is not supposed to fix all PHP5.4 warnings and notices, I will create a new issue to address the rest later.

          Show
          Petr Škoda added a comment - This issue is not supposed to fix all PHP5.4 warnings and notices, I will create a new issue to address the rest later.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          "and disable E_STRICT" ?

          Show
          Eloy Lafuente (stronk7) added a comment - "and disable E_STRICT" ?
          Hide
          Petr Škoda added a comment -

          yes, problems in this patch are standard notices in PHP 5.4RC, previously E_STRICT in PHP 5.3

          Show
          Petr Škoda added a comment - yes, problems in this patch are standard notices in PHP 5.4RC, previously E_STRICT in PHP 5.3
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (21, 22 and master), thanks!

          Note1: Because of 1 typo the the commit message I've integrated this by cherry-picking (instead of merge).

          Note2: I've backported it to 21_STABLE (supported), there were 2 conflicts, easy to solve.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (21, 22 and master), thanks! Note1: Because of 1 typo the the commit message I've integrated this by cherry-picking (instead of merge). Note2: I've backported it to 21_STABLE (supported), there were 2 conflicts, easy to solve. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Got 1 assertion failed in all branches:

          filter_delete_config_test.test_filter_delete_all_for_filter 
          
          Expected false, got [Object: of stdClass] at [/lib/simpletest/testfilterconfig.php line 750]
          

          Before the changes it was returning null and curiously, this passes:

          $this->assertFalse(null);
          

          Going to review uses (to check that there are no uses relying on false/null) and then fix it to check for empty stdClass() instead.

          Show
          Eloy Lafuente (stronk7) added a comment - Got 1 assertion failed in all branches: filter_delete_config_test.test_filter_delete_all_for_filter Expected false , got [ Object : of stdClass] at [/lib/simpletest/testfilterconfig.php line 750] Before the changes it was returning null and curiously, this passes: $ this ->assertFalse( null ); Going to review uses (to check that there are no uses relying on false/null) and then fix it to check for empty stdClass() instead.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Using

          grep -r '[^>:]get_config([^,]*)' *
          

          revealed 94 uses, grrr. At least:

          (array)null === (array)new stdClass() ==> empty array
          

          Looking...

          Show
          Eloy Lafuente (stronk7) added a comment - Using grep -r '[^>:]get_config([^,]*)' * revealed 94 uses, grrr. At least: (array) null === (array) new stdClass() ==> empty array Looking...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Crap, supercrap, but this is not true:

          (bool)null === (bool)new stdClass() ==> false and true
          

          So we cannot change previous behavior without modifying code here and there (the results of the calls are used within conditions.

          So, I'm reverting the change in get_config() back to return null. Feel free to create another issue about moving again to stdClass() + review uses in conditions.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Crap, supercrap, but this is not true: (bool) null === (bool) new stdClass() ==> false and true So we cannot change previous behavior without modifying code here and there (the results of the calls are used within conditions. So, I'm reverting the change in get_config() back to return null. Feel free to create another issue about moving again to stdClass() + review uses in conditions. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Done, I've added this to master, 22, and 21:

          http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=7fb7e9835adac5bb756d9eb4bfcf307d802bc791

          If we really want it to start returning stdClass(), we'll need to review all the uses where it's being evaluated as boolean, changing it to empty($xxxx) or whatever.

          But not here and today. For your consideration.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Done, I've added this to master, 22, and 21: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=7fb7e9835adac5bb756d9eb4bfcf307d802bc791 If we really want it to start returning stdClass(), we'll need to review all the uses where it's being evaluated as boolean, changing it to empty($xxxx) or whatever. But not here and today. For your consideration. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, I've verified:

          • all unit tests passing.
          • install and upgrade working
          • login working
          • blocks content and configuration working
          • filters doing their work
          • server-side validation of required and empty texts

          So I think this can be considered passed. All the rest of changes were only to get variables initialized before setting their members.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, I've verified: all unit tests passing. install and upgrade working login working blocks content and configuration working filters doing their work server-side validation of required and empty texts So I think this can be considered passed. All the rest of changes were only to get variables initialized before setting their members. Ciao
          Hide
          Petr Škoda added a comment -

          Hi, I have looked at the config already, first trying to "fix" all incorrect uses and then decided to return the empty stdClass instead - I did not find any place that would be broken by this change - most of them did already the change of false to something else.

          In any case I agree this needs more work and separate review, next week, I am filing new issue.

          Show
          Petr Škoda added a comment - Hi, I have looked at the config already, first trying to "fix" all incorrect uses and then decided to return the empty stdClass instead - I did not find any place that would be broken by this change - most of them did already the change of false to something else. In any case I agree this needs more work and separate review, next week, I am filing new issue.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well, I started looking for uses sequentially, using the grep above. And in the 2nd/3rd file I found something like:

          $config = get_config('xxxx');
          if (!$config) {
              ....
              ....
          

          And that evaluates differently if we switch to stdClass(), and that what the reason for the (partial) revert. There are no problems in places casting to array, but any place using if as boolean or checking with empty() will need change for sure.

          Just in case it helps looking for the patterns to modify.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, I started looking for uses sequentially, using the grep above. And in the 2nd/3rd file I found something like: $config = get_config('xxxx'); if (!$config) { .... .... And that evaluates differently if we switch to stdClass(), and that what the reason for the (partial) revert. There are no problems in places casting to array, but any place using if as boolean or checking with empty() will need change for sure. Just in case it helps looking for the patterns to modify. Ciao
          Hide
          Petr Škoda added a comment -

          Yes, I looked at the places that explicitely look for false, but the end result was the same when I used the stdClass in get_config(). Maybe the best way will be to do it in master with full code cleanup and do the backporting only when necessary. In any case thanks a lot!

          Show
          Petr Škoda added a comment - Yes, I looked at the places that explicitely look for false, but the end result was the same when I used the stdClass in get_config(). Maybe the best way will be to do it in master with full code cleanup and do the backporting only when necessary. In any case thanks a lot!
          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
          Hide
          Tim Hunt added a comment -

          Cherry-picking fixes that use context_system into 2.1 was a pretty bad idea: http://moodle.org/mod/forum/discuss.php?d=194417. I just filed MDL-31330.

          Show
          Tim Hunt added a comment - Cherry-picking fixes that use context_system into 2.1 was a pretty bad idea: http://moodle.org/mod/forum/discuss.php?d=194417 . I just filed MDL-31330 .

            People

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

              Dates

              • Created:
                Updated:
                Resolved: