Moodle
  1. Moodle
  2. MDL-15974

Unnecessary PHP Notices in lib/weblib.php and type/multichoice/questiontype.php

    Details

    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE
    • Rank:
      26373

      Description

      Noticing lots of noise in the webserver logs, such as:

      [error] PHP Notice: Undefined property: picture in /var/www/stjohn-moodle-18/moodle/lib/weblib.php on line 3512
      [error] PHP Notice: Undefined property: shuffleanswers in /var/www/stjohn-moodle-18/moodle/question/type/multichoice/questiontype.php on line 167
      [error] PHP Notice: Undefined property: delay1 in /var/www/stjohn-moodle-18/moodle/mod/quiz/attempt.php on line 173

      These are due to if ($variable) statements needing to be if (!empty($variable))

      Patch for MOODLE_18_STABLE (as of 2008-08-08) attached.

      1. empty-checks.diff
        3 kB
        Jonathan Harker
      2. empty-checks-19.diff
        3 kB
        Jonathan Harker
      3. empty-checks-cvshead.diff
        1 kB
        Jonathan Harker

        Activity

        Hide
        Jonathan Harker added a comment -

        Noticed these in MOODLE_19_STABLE (2008-08-08) as well - attached 1.9 patch

        Show
        Jonathan Harker added a comment - Noticed these in MOODLE_19_STABLE (2008-08-08) as well - attached 1.9 patch
        Hide
        Jonathan Harker added a comment -

        ...and for CVSHEAD - smaller patch, mod/quiz/attempts.php has been reworked.

        Show
        Jonathan Harker added a comment - ...and for CVSHEAD - smaller patch, mod/quiz/attempts.php has been reworked.
        Hide
        Jonathan Harker added a comment -

        I should note that I don't have CVS write access, and don't seem to be able to assign it to anyone

        Show
        Jonathan Harker added a comment - I should note that I don't have CVS write access, and don't seem to be able to assign it to anyone
        Hide
        Tim Hunt added a comment -

        Today is testing day (http://docs.moodle.org/en/Development:Weekly_Code_Review) so I'll check these in tomorrow.

        Note, you should really only select the latest release from each branch under Affects version/s, although that is not a big deal.

        Show
        Tim Hunt added a comment - Today is testing day ( http://docs.moodle.org/en/Development:Weekly_Code_Review ) so I'll check these in tomorrow. Note, you should really only select the latest release from each branch under Affects version/s, although that is not a big deal.
        Hide
        Tim Hunt added a comment -

        Actually, I am not convinced by your patch. For example, if you call print_user_picture, then $user->picture must be set, otherwise it is an error in the caller. Just putting in the !empty hides a real bug, rather than fixing it.

        Similarly, $cmoptions->shuffleanswers and $question->options->shuffleanswers should both be set before calling whatever function that is in the multichoice question type.

        So you need to find what people are doing when those errors occur, then we can fix the callers.

        Show
        Tim Hunt added a comment - Actually, I am not convinced by your patch. For example, if you call print_user_picture, then $user->picture must be set, otherwise it is an error in the caller. Just putting in the !empty hides a real bug, rather than fixing it. Similarly, $cmoptions->shuffleanswers and $question->options->shuffleanswers should both be set before calling whatever function that is in the multichoice question type. So you need to find what people are doing when those errors occur, then we can fix the callers.
        Hide
        Jonathan Harker added a comment -

        Hi Tim,

        Not necessarily.

        The problem arose from the difference in database schema (on postgresql) between a fresh install of a codebase and the same codebase using a database upgraded from an earlier (1.5) moodle.

        In a fresh install, the picture field in mdl_user (and other problem fields) are defined as integer not null default 0, whereas in the upgraded database (which has gone through the various upgrade functions) they do not have the not null or default set.

        So you're right in a way, the code is expecting these fields to be not null and therefore to be always set (with a default zero integer value). But in real life people are running upgraded databases where this is not the case, so we need to either make sure that the Moodle code is more robust by not assuming things are necessarily set (which the !empty() check handles) which is much easier, or by going through a complete database regression analysis on all supported database platforms and adding tidy up functions to the upgrade.php of lib as well as all modules and blocks, as well as checking that the install.xml structure is actually reflecting the right choices of not null and default values, which would be nice, but also time consuming, not particularly maintainable in the long term (imo) and somewhat error prone.

        Show
        Jonathan Harker added a comment - Hi Tim, Not necessarily. The problem arose from the difference in database schema (on postgresql) between a fresh install of a codebase and the same codebase using a database upgraded from an earlier (1.5) moodle. In a fresh install, the picture field in mdl_user (and other problem fields) are defined as integer not null default 0, whereas in the upgraded database (which has gone through the various upgrade functions) they do not have the not null or default set. So you're right in a way, the code is expecting these fields to be not null and therefore to be always set (with a default zero integer value). But in real life people are running upgraded databases where this is not the case, so we need to either make sure that the Moodle code is more robust by not assuming things are necessarily set (which the !empty() check handles) which is much easier, or by going through a complete database regression analysis on all supported database platforms and adding tidy up functions to the upgrade.php of lib as well as all modules and blocks, as well as checking that the install.xml structure is actually reflecting the right choices of not null and default values, which would be nice, but also time consuming, not particularly maintainable in the long term (imo) and somewhat error prone.
        Hide
        Tim Hunt added a comment -

        Yes, but Moodle is not really un-robust in this situation, it only outputs notices if you have debugging turned up to the hightest level. Moodle still functions correctly. In my opinion those notices are right, they are telling you something minor is not right with your Moodle install. That is exactly what the situation is.

        If you go to Admin -> Experimental -> XMLDB editor, you will see some options [Check Indexes] [Check Bigints] at the top of the screen. I think that the correct way forwards here is to make a new option [Check defaults] that checks for the kinds of inconsistencies you are seeing with old sites that have been upgraded.

        Show
        Tim Hunt added a comment - Yes, but Moodle is not really un-robust in this situation, it only outputs notices if you have debugging turned up to the hightest level. Moodle still functions correctly. In my opinion those notices are right, they are telling you something minor is not right with your Moodle install. That is exactly what the situation is. If you go to Admin -> Experimental -> XMLDB editor, you will see some options [Check Indexes] [Check Bigints] at the top of the screen. I think that the correct way forwards here is to make a new option [Check defaults] that checks for the kinds of inconsistencies you are seeing with old sites that have been upgraded.
        Hide
        Tim Hunt added a comment -

        OK, well I checked this in, just to get it off my open bug list.

        By the way, why don't you have CVS commit access?

        Show
        Tim Hunt added a comment - OK, well I checked this in, just to get it off my open bug list. By the way, why don't you have CVS commit access?
        Hide
        Andrew Davis added a comment -

        This fix feels a little hacky but its evidently been running fine for a year now. A more bulletproof solution for ensuring an upgraded schema exactly matches a newly installed schema would be good but is probably too big a task to be done in 1.9. Closing.

        Show
        Andrew Davis added a comment - This fix feels a little hacky but its evidently been running fine for a year now. A more bulletproof solution for ensuring an upgraded schema exactly matches a newly installed schema would be good but is probably too big a task to be done in 1.9. Closing.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: