Moodle
  1. Moodle
  2. MDL-20918

the usergetdate() function returns different information depending on the PHP version

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.4
    • Fix Version/s: 1.9.8
    • Component/s: Libraries
    • Labels:
      None
    • Environment:
      all
    • Rank:
      31852

      Description

      A number of people are reporting that they cannot get a monthly attendance report to display when using version 2.1.1 on moodle 1.9.5.

      When they click on "monthly report" the page returns with "nothing to display". They are able to see weekly reports.

      See this discussion for more information:
      http://moodle.org/mod/forum/discuss.php?d=133026

      1. moodlelib.patch
        1 kB
        Andrew Davis
      2. testmoodlelib.php.patch
        2 kB
        Andrew Davis

        Activity

        Hide
        Richard Webb added a comment -

        As I documented in the thread where this bug report was spawned, I believe this is a result of the core Moodle function usergetdate (in lib/moodlelib.php) returning data in a different order if the time zone is not set in Moodle. If I am correct, then this should be moved from a CONTRIB bug to a MDL bug.

        The short-term solution is to change usergetdate to reorder the return when the time zone has not been set. However, the more reasonable (yet far more intensive change because it requires changing the code in every piece of code that calls usergetdate) is to change the order of data returned when a user has set the time zone to match the default order that PHP returns with its getdate function.

        Show
        Richard Webb added a comment - As I documented in the thread where this bug report was spawned, I believe this is a result of the core Moodle function usergetdate (in lib/moodlelib.php) returning data in a different order if the time zone is not set in Moodle. If I am correct, then this should be moved from a CONTRIB bug to a MDL bug. The short-term solution is to change usergetdate to reorder the return when the time zone has not been set. However, the more reasonable (yet far more intensive change because it requires changing the code in every piece of code that calls usergetdate) is to change the order of data returned when a user has set the time zone to match the default order that PHP returns with its getdate function.
        Hide
        Martin Dougiamas added a comment -

        One for you Andrew. Can you look into this whole issue? See some analysis on the linked forum discussion.

        Show
        Martin Dougiamas added a comment - One for you Andrew. Can you look into this whole issue? See some analysis on the linked forum discussion.
        Hide
        Andrew Davis added a comment -

        At the moment I'm intending to change moodle to match the default order produced by getdate(). It shouldn't be too big a change (he said optimistically) as moodle seems to mostly refer to the elements of the array by key and not by index. As long as they're referred to by key their numerical index doesn't matter. I don't about contrib code however.

        It does seem like a good idea to bring us inline with the default rather than reshuffling the default to match our own order.

        Show
        Andrew Davis added a comment - At the moment I'm intending to change moodle to match the default order produced by getdate(). It shouldn't be too big a change (he said optimistically) as moodle seems to mostly refer to the elements of the array by key and not by index. As long as they're referred to by key their numerical index doesn't matter. I don't about contrib code however. It does seem like a good idea to bring us inline with the default rather than reshuffling the default to match our own order.
        Hide
        Andrew Davis added a comment - - edited

        ok, Ive attached patches to moodlelib.php and testmoodlelib.php for everyone to have a look at. Let me know if I've done anything silly

        I added some unit tests. The changes I made to usergetdate() itself were a little more invasive than I expected. I wrote the unit tests testing the return array with and without a time zone then modified usergetdate() as necessary to get the returned array to be basically identical.

        The core of moodle shouldn't be affected at all by these changes. I've looked through every call and they're all accessing the array using keys like this...
        $thisdate = usergetdate(time());
        echo($thisdate['mon']);

        The alternative is something like this.
        list($seconds,$minutes,$hours,$mday,$wday,$mon,$year,$yday,$weekday,$month) = usergetdate(time());
        I would recommend NOT doing it this way. Any changes in the array order will cause the wrong values to wind up in the wrong variables which could have all sorts of hard to track down side effects. The unit tests should catch that but... Note that the unit tests do it this way so that they document the order of things if you are going to do it this way.

        Show
        Andrew Davis added a comment - - edited ok, Ive attached patches to moodlelib.php and testmoodlelib.php for everyone to have a look at. Let me know if I've done anything silly I added some unit tests. The changes I made to usergetdate() itself were a little more invasive than I expected. I wrote the unit tests testing the return array with and without a time zone then modified usergetdate() as necessary to get the returned array to be basically identical. The core of moodle shouldn't be affected at all by these changes. I've looked through every call and they're all accessing the array using keys like this... $thisdate = usergetdate(time()); echo($thisdate ['mon'] ); The alternative is something like this. list($seconds,$minutes,$hours,$mday,$wday,$mon,$year,$yday,$weekday,$month) = usergetdate(time()); I would recommend NOT doing it this way. Any changes in the array order will cause the wrong values to wind up in the wrong variables which could have all sorts of hard to track down side effects. The unit tests should catch that but... Note that the unit tests do it this way so that they document the order of things if you are going to do it this way.
        Hide
        Andrew Davis added a comment -

        Went through this with Martin. Committed.

        Show
        Andrew Davis added a comment - Went through this with Martin. Committed.

          People

          • Votes:
            3 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: