Moodle
  1. Moodle
  2. MDL-15183

changes in lib/graphlib are causing notices in overview graph

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.9.2
    • Fix Version/s: 1.9.2
    • Component/s: Libraries
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      24411

      Description

      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.

        Activity

        Hide
        Petr Škoda added a comment -

        please fix before review Tuesday, thanks

        Show
        Petr Škoda added a comment - please fix before review Tuesday, thanks
        Hide
        Jenny Gray added a comment -

        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?

        Show
        Jenny Gray added a comment - 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?
        Hide
        Jamie Pratt added a comment -

        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

        Show
        Jamie Pratt added a comment - 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
        Hide
        Jamie Pratt added a comment -

        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.

        Show
        Jamie Pratt added a comment - 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.
        Hide
        Jenny Gray added a comment -

        Defaults added as Jamie suggested.

        Show
        Jenny Gray added a comment - Defaults added as Jamie suggested.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: