Issue Details (XML | Word | Printable)

Key: MDL-15974
Type: Bug Bug
Status: Resolved Resolved
Resolution: Fixed
Priority: Minor Minor
Assignee: Tim Hunt
Reporter: Jonathan Harker
Votes: 0
Watchers: 1
Operations

Add/Edit UI Mockup to this issue
If you were logged in you would be able to see more operations.
Moodle

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

Created: 08/Aug/08 09:13 AM   Updated: 27/Nov/08 02:15 PM
Return to search
Component/s: General, Lib, Questions, Quiz
Affects Version/s: 1.8.6, 1.9.2
Fix Version/s: 1.7.7, 1.8.8, 1.9.4

File Attachments: 1. File empty-checks-19.diff (3 kB)
2. File empty-checks-cvshead.diff (1 kB)
3. File empty-checks.diff (3 kB)


Participants: Jonathan Harker and Tim Hunt
Security Level: None
Resolved date: 27/Nov/08
Affected Branches: MOODLE_18_STABLE, MOODLE_19_STABLE
Fixed Branches: MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE


 Description  « Hide
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.


 All   Comments   Change History   Version Control      Sort Order: Ascending order - Click to sort in descending order
Jonathan Harker added a comment - 08/Aug/08 09:39 AM
Noticed these in MOODLE_19_STABLE (2008-08-08) as well - attached 1.9 patch

Jonathan Harker added a comment - 08/Aug/08 12:04 PM
...and for CVSHEAD - smaller patch, mod/quiz/attempts.php has been reworked.

Jonathan Harker added a comment - 08/Aug/08 12:05 PM
I should note that I don't have CVS write access, and don't seem to be able to assign it to anyone

Tim Hunt added a comment - 12/Aug/08 11:43 AM
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.


Tim Hunt added a comment - 13/Aug/08 10:54 AM
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.


Jonathan Harker added a comment - 13/Aug/08 12:26 PM
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.


Tim Hunt added a comment - 13/Aug/08 03:18 PM
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.


Tim Hunt added a comment - 27/Nov/08 02:15 PM
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?