Issue Details (XML | Word | Printable)

Key: MDL-15183
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Critical Critical
Assignee: Jenny Gray
Reporter: Jamie Pratt
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

changes in lib/graphlib are causing notices in overview graph

Created: 08/Jun/08 06:01 PM   Updated: 30/Apr/09 10:26 PM
Return to search
Component/s: Lib
Affects Version/s: 1.9.2
Fix Version/s: 1.9.2

File Attachments: 1. File graphlib.diff (0.6 kB)


Participants: Jamie Pratt, Jenny Gray and Petr Skoda
Security Level: None
QA Assignee: Nicolas Connault
Resolved date: 11/Jun/08
Affected Branches: MOODLE_19_STABLE
Fixed Branches: MOODLE_19_STABLE


 Description  « Hide
Before changes in the graphlib I had the following lines in the overview graph :

//following two lines seem to silence notice warnings from graphlib.php
$line->y_tick_labels = null;
$line->offset_relation = null;

Since changes in graphlib these no longer work but now these lines silence the warnings :

//following two lines seem to silence notice warnings from graphlib.php
$line->parameter['y_tick_labels'] = null;
$line->offset_relation = null;

I think other graphs need to be checked to see if they are also broken. The library really needs fixing so that these properties are set to a sensible default or something and don't need to be set if they are not used.

Assigning this to Jenny since according to CVS Jenny made the changes to graphlib.

 All   Comments   Change History   Version Control      Sort Order: Ascending order - Click to sort in descending order
Petr Skoda added a comment - 08/Jun/08 06:55 PM
please fix before review Tuesday, thanks

Jenny Gray added a comment - 09/Jun/08 06:13 PM
Well this is what you get for trying to do something quickly that looked simple I guess. A big SORRY for me, this 1 line change has been a pain all the way.

I've just reverted the code in graphlib, so all core graphs should go back to working now.

What I was trying to do was resolve an issue for some-one else at the OU who had used $graph->parameter['y_tick_labels'] rather than the way Jamie has defaulted the values above. She's on maternity leave so sadly I can't ask why! I've altered her code to match Jamie's instead so the contrib module should work now with the core code - which is better.

What I wonder though - and why I'm not closing this yet, is whether we should add to the class init to default offset_relation and y_tick_labels to null, rather than expect every new graph() to do it?


Jamie Pratt added a comment - 09/Jun/08 06:26 PM
Hi Jenny,

Thanks for fixing this.

My +1 to set default properties of class graph :

var $y_tick_labels = null;
var $offset_relation = null;

Jamie


Jamie Pratt added a comment - 10/Jun/08 06:19 PM
Jenny's patch looks good to me. Tested in overview report and it looks OK. No need for :

//following two lines seem to silence notice warnings from graphlib.php
$line->parameter['y_tick_labels'] = null;
$line->offset_relation = null;

Anymore.


Jenny Gray added a comment - 11/Jun/08 04:26 PM
Defaults added as Jamie suggested.