Moodle
  1. Moodle
  2. MDL-32234

invalid index error while browsing users with filter

    Details

    • Testing Instructions:
      Hide
      1. Log in as admin
      2. Edit profile and change timezone to UTC (Can be any other then server time)
      3. Browse users (Settings -> site administration -> Users -> accounts -> Browse list of users)
      4. Add some filter text and press "Add filter"
      5. no notice should appear.

      Test 2:

      1. Log in as admin
      2. Edit profile and set timezone to UTC
      3. Add calendar event for today at 00:00
      4. Edit profile and set timezone to server time
      5. Check today event, time should be properly shifted (for perth it should be 8:00 A.m.)
      6. Now add another event and change timezone
      7. make sure time is changed accordingly.

      Test 3:
      Run unit test: lib/simpletest/testmoodlelib.php on mac/linux/windows
      If possible test different cases to make sure change in timezone works fine.

      Show
      Log in as admin Edit profile and change timezone to UTC (Can be any other then server time) Browse users (Settings -> site administration -> Users -> accounts -> Browse list of users) Add some filter text and press "Add filter" no notice should appear. Test 2: Log in as admin Edit profile and set timezone to UTC Add calendar event for today at 00:00 Edit profile and set timezone to server time Check today event, time should be properly shifted (for perth it should be 8:00 A.m.) Now add another event and change timezone make sure time is changed accordingly. Test 3: Run unit test: lib/simpletest/testmoodlelib.php on mac/linux/windows If possible test different cases to make sure change in timezone works fine.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-32234

      Description

      Note: turn debugging on.
      Steps to reproduce:

      1. Log in as admin
      2. Edit profile and change timezone to UTC (Can be any other then server time)
      3. Browse users (Settings -> site administration -> Users -> accounts -> Browse list of users)
      4. Add some filter text and press "Add filter"
      5. notice will appear.

      FYI:
      This is happening because usergetdate returns mon in two digits and select.php use strict matching.

      Solution:
      In dateselector.php typecase $currentdate['mon'] on line 153.
      (int) $currentdate['mon'];

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Rajesh Taneja added a comment -

            Increasing priority as it is broken functionality and can affect multiple modules.

            Show
            Rajesh Taneja added a comment - Increasing priority as it is broken functionality and can affect multiple modules.
            Hide
            Rajesh Taneja added a comment -

            https://github.com/rajeshtaneja/moodle/blob/6efbcd9257b34c1cc51c5856fd7ba43f806f590f/lib/moodlelib.php#L2074
            getdate array should match with one produced by $datestring = gmstrftime('%B_%A_%j_%Y_%m_%w_%d_%H_%M_%S', $time);
            %m returns two digit representation of the month, whereas getdate returns numeric representation of a month.

            In theory it should not break anything, but still think it should go in master only and then back-ported later to stable branches.

            Show
            Rajesh Taneja added a comment - https://github.com/rajeshtaneja/moodle/blob/6efbcd9257b34c1cc51c5856fd7ba43f806f590f/lib/moodlelib.php#L2074 getdate array should match with one produced by $datestring = gmstrftime('%B_%A_%j_%Y_%m_%w_%d_%H_%M_%S', $time); %m returns two digit representation of the month, whereas getdate returns numeric representation of a month. In theory it should not break anything, but still think it should go in master only and then back-ported later to stable branches.
            Hide
            Dan Poltawski added a comment -

            Hi Raj,

            I agree about the change because usergetdate should not return different formatted results if called without a timezone. (Which it does by calling getdate which returns month without the leading 0).

            A really trivial comment about thing about the commit message ( http://docs.moodle.org/dev/Commit_cheat_sheet ) - could you shortern the first line and include mention of the function name. It helps in viewing the logs to have that shortened and including the function name just helps when viewing history to know exactly where it affected

            Show
            Dan Poltawski added a comment - Hi Raj, I agree about the change because usergetdate should not return different formatted results if called without a timezone. (Which it does by calling getdate which returns month without the leading 0). A really trivial comment about thing about the commit message ( http://docs.moodle.org/dev/Commit_cheat_sheet ) - could you shortern the first line and include mention of the function name. It helps in viewing the logs to have that shortened and including the function name just helps when viewing history to know exactly where it affected
            Hide
            Rajesh Taneja added a comment -

            Thanks Dan,
            Commit message is shortened now. Pushing it for integration review.

            Show
            Rajesh Taneja added a comment - Thanks Dan, Commit message is shortened now. Pushing it for integration review.
            Hide
            Dan Poltawski added a comment -

            Hi Raj - you didn't need to shortern the commit message, just first line

            I really encourage you to write detailed messages in the next lines!

            Show
            Dan Poltawski added a comment - Hi Raj - you didn't need to shortern the commit message, just first line I really encourage you to write detailed messages in the next lines!
            Hide
            Aparup Banerjee added a comment - - edited

            Hi Raj,
            this looks good, it seems other calls won't be affected or will cast into integer.
            i was wondering why not strip for %d too ( Two-digit day of the month (with leading zeros) ) as the same index error could be avoided elsewhere not to mention the format is kept same as getdate().

            $datestring = gmstrftime('%B_%A_%j_%Y_%-m_%w_%-d_%H_%M_%S', $time);
            

            ?

            Show
            Aparup Banerjee added a comment - - edited Hi Raj, this looks good, it seems other calls won't be affected or will cast into integer. i was wondering why not strip for %d too ( Two-digit day of the month (with leading zeros) ) as the same index error could be avoided elsewhere not to mention the format is kept same as getdate(). $datestring = gmstrftime('%B_%A_%j_%Y_%-m_%w_%-d_%H_%M_%S', $time); ?
            Hide
            Rajesh Taneja added a comment -

            Thanks Aparup,

            You are right this should have been checked for all the values. Following is fixed now:

            1. All int values are stripped with leading zero.
            2. Values are typecasted properly, before they return
            3. %j returns 001 through 366, whereas getdate() returns 0 through 365. This has been taken care off.

            I have added 21 and 22 branches as well, and it should go on stable as well.

            Show
            Rajesh Taneja added a comment - Thanks Aparup, You are right this should have been checked for all the values. Following is fixed now: All int values are stripped with leading zero. Values are typecasted properly, before they return %j returns 001 through 366, whereas getdate() returns 0 through 365. This has been taken care off. I have added 21 and 22 branches as well, and it should go on stable as well.
            Hide
            Dan Poltawski added a comment -

            All looks good here, but this is slightly more scary change now.

            Perhaps a deep look of uses is required.

            Show
            Dan Poltawski added a comment - All looks good here, but this is slightly more scary change now. Perhaps a deep look of uses is required.
            Hide
            Aparup Banerjee added a comment -

            reopening for more work (going on at the moment too)

            Show
            Aparup Banerjee added a comment - reopening for more work (going on at the moment too)
            Hide
            Rajesh Taneja added a comment -

            I think this is good to go.
            Dan can you please review this again for me.

            Thanks

            Show
            Rajesh Taneja added a comment - I think this is good to go. Dan can you please review this again for me. Thanks
            Hide
            Dan Poltawski added a comment -

            Looks good to me

            Show
            Dan Poltawski added a comment - Looks good to me
            Hide
            Rajesh Taneja added a comment -

            Thanks Dan,
            Pushing it for integration.

            Show
            Rajesh Taneja added a comment - Thanks Dan, Pushing it for integration.
            Hide
            Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Rajesh Taneja added a comment -

            Branches re-based.

            Show
            Rajesh Taneja added a comment - Branches re-based.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Hi Rajesh,

            I was moving your changes in (not existing anymore) lib/simpletest/testmoodlelib.php to the new lib/tests/moodlelib_test.php and executing it I'm getting one failure:

            phpunit lib/tests/moodlelib_test.php
             
            Time: 3 seconds, Memory: 71.00Mb
             
            There was 1 failure:
             
            1) moodlelib_testcase::test_usergetdate
            Failed asserting that 7 matches expected 0.
             
            integration/lib/tests/moodlelib_test.php:1016
            integration/lib/phpunit/lib.php:1162
            

            So it seems that the code is introducing some problem with the seconds... so I'm reopening this.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Hi Rajesh, I was moving your changes in (not existing anymore) lib/simpletest/testmoodlelib.php to the new lib/tests/moodlelib_test.php and executing it I'm getting one failure: phpunit lib/tests/moodlelib_test.php   Time: 3 seconds, Memory: 71.00Mb   There was 1 failure:   1) moodlelib_testcase::test_usergetdate Failed asserting that 7 matches expected 0.   integration/lib/tests/moodlelib_test.php:1016 integration/lib/phpunit/lib.php:1162 So it seems that the code is introducing some problem with the seconds... so I'm reopening this. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Hi Rajesh, it seems that we cannot rely in the "-" modifier at all. I asked @ HQ for help running this:

            php -r 'error_reporting(0); date_default_timezone_set("UTC"); foreach (str_sp
            lit("BAjYmwdHMS" ) as $m) { echo "$m: " . gmstrftime("%$m", 7) . "#" . gmstrfti
            me("%-$m", 7) . "#" .  strftime("%$m", 7) . "#" .  strftime("%-$m", 7) . "#" .
            PHP_EOL; }'
            

            And got all sort of results, in summary:

            • Linux seemed to accept it ok.
            • MacOS X was working ok for Eric but not for me (I was getting "-S" on output)
            • Windows directly "eat" any modifier with "-" (and requires setting error reporting to 0.
            • Solaris returns "-S" too.

            So I think we must do that without those "-".

            Here it's a copy of the discussion @ HQ. Thanks everybody:

            [15:48:36] <Cindereloy> Offtopic. Can anybody execute this under linux and windows:
             
            php -r 'date_default_timezone_set("UTC"); foreach (str_split("BAjYmwdHMS" ) as $m) { echo "$m: " . gmstrftime("%$m", 7) . "#" . gmstrftime("%-$m", 7) . "#" .  strftime("%$m", 7) . "#" .  strftime("%-$m", 7) . "#" . PHP_EOL; }'
             
            and paste results?
            [15:48:49] <Cindereloy> (it's not a virus)
            [15:49:28] <Eric Merrill> B: January#January#January#January#
            A: Thursday#Thursday#Thursday#Thursday#
            j: 001#1#001#1#
            Y: 1970#1970#1970#1970#
            m: 01#1#01#1#
            w: 4#4#4#4#
            d: 01#1#01#1#
            H: 00#0#00#0#
            M: 00#0#00#0#
            S: 07#7#07#7#
            [15:49:37] <Cindereloy> that is which OS?
            [15:49:44] <Eric Merrill> RHL 5 I think
            [15:49:52] <timhunt> repeated many thousands of times.
            [15:50:15] <Eric Merrill> Identical on mac os x too
            [15:50:18] <timhunt> How can I kill the warnings and notices?
            [15:50:22] <Cindereloy> identical on mac?
            [15:50:24] <david> 
            B: January#January#January#January#
            A: Thursday#Thursday#Thursday#Thursday#
            j: 001#1#001#1#
            Y: 1970#1970#1970#1970#
            m: 01#1#01#1#
            w: 4#4#4#4#
            d: 01#1#01#1#
            H: 00#0#00#0#
            M: 00#0#00#0#
            S: 07#7#07#7#
             
            [15:50:34] <Cindereloy> grrr, here i get:
             
            B: January#-B#January#-B#
            A: Thursday#-A#Thursday#-A#
            j: 001#-j#001#-j#
            Y: 1970#-Y#1970#-Y#
            m: 01#-m#01#-m#
            w: 4#-w#4#-w#
            d: 01#-d#01#-d#
            H: 00#-H#00#-H#
            M: 00#-M#00#-M#
            S: 07#-S#07#-S#
             
            [15:50:37] <Eric Merrill> Red Hat and Mac seem to give identical results
            [15:51:08] <timhunt> $ php -r 'error_reporting(0); date_default_timezone_set("UTC"); foreach (str_sp
            lit("BAjYmwdHMS" ) as $m) { echo "$m: " . gmstrftime("%$m", 7) . "#" . gmstrfti
            me("%-$m", 7) . "#" .  strftime("%$m", 7) . "#" .  strftime("%-$m", 7) . "#" .
            PHP_EOL; }'
            B: January##January##
            A: Thursday##Thursday##
            j: 001##001##
            Y: 1970##1970##
            m: 01##01##
            w: 4##4##
            d: 01##01##
            H: 00##00##
            M: 00##00##
            S: 07##07##
            [15:51:25] <Cindereloy> buf, that's worse. It "eats" them.
            [15:51:26] <timhunt> I added error_reporting 0
            [15:51:48] <Cindereloy> oki, thanks. I think the conclusion is that we cannot rely on the "-" modifier.
            [15:51:52] <Andrew Nicols> Do you want it on Solaris too?
            [15:51:58] <Cindereloy> yes thanks
            [15:52:03] <Andrew Nicols> 504 nicols@current10x:~> /opt/csw/php5/bin/php -r 'date_default_timezone_set("UTC"); foreach (str_split("BAjYmwdHMS" ) as $m) { echo "$m: " . gmstrftime("%$m", 7) . "#" . gmstrftime("%-$m", 7) . "#" .  strftime("%$m", 7) . "#" .  strftime("%-$m", 7) . "#" . PHP_EOL
            ; }'
            B: January#%-B#January#%-B#
            A: Thursday#%-A#Thursday#%-A#
            j: 001#%-j#001#%-j#
            Y: 1970#%-Y#1970#%-Y#
            m: 01#%-m#01#%-m#
            w: 4#%-w#4#%-w#
            d: 01#%-d#01#%-d#
            H: 00#%-H#00#%-H#
            M: 00#%-M#00#%-M#
            S: 07#%-S#07#%-S#
             
            [15:52:09] <Cindereloy> aha
            [15:52:28] <Cindereloy> oki, many thanks. 
            

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Hi Rajesh, it seems that we cannot rely in the "-" modifier at all. I asked @ HQ for help running this: php -r 'error_reporting(0); date_default_timezone_set("UTC"); foreach (str_sp lit("BAjYmwdHMS" ) as $m) { echo "$m: " . gmstrftime("%$m", 7) . "#" . gmstrfti me("%-$m", 7) . "#" . strftime("%$m", 7) . "#" . strftime("%-$m", 7) . "#" . PHP_EOL; }' And got all sort of results, in summary: Linux seemed to accept it ok. MacOS X was working ok for Eric but not for me (I was getting "-S" on output) Windows directly "eat" any modifier with "-" (and requires setting error reporting to 0. Solaris returns "-S" too. So I think we must do that without those "-". Here it's a copy of the discussion @ HQ. Thanks everybody: [15:48:36] <Cindereloy> Offtopic. Can anybody execute this under linux and windows:   php -r 'date_default_timezone_set("UTC"); foreach (str_split("BAjYmwdHMS" ) as $m) { echo "$m: " . gmstrftime("%$m", 7) . "#" . gmstrftime("%-$m", 7) . "#" . strftime("%$m", 7) . "#" . strftime("%-$m", 7) . "#" . PHP_EOL; }'   and paste results? [15:48:49] <Cindereloy> (it's not a virus) [15:49:28] <Eric Merrill> B: January#January#January#January# A: Thursday#Thursday#Thursday#Thursday# j: 001#1#001#1# Y: 1970#1970#1970#1970# m: 01#1#01#1# w: 4#4#4#4# d: 01#1#01#1# H: 00#0#00#0# M: 00#0#00#0# S: 07#7#07#7# [15:49:37] <Cindereloy> that is which OS? [15:49:44] <Eric Merrill> RHL 5 I think [15:49:52] <timhunt> repeated many thousands of times. [15:50:15] <Eric Merrill> Identical on mac os x too [15:50:18] <timhunt> How can I kill the warnings and notices? [15:50:22] <Cindereloy> identical on mac? [15:50:24] <david> B: January#January#January#January# A: Thursday#Thursday#Thursday#Thursday# j: 001#1#001#1# Y: 1970#1970#1970#1970# m: 01#1#01#1# w: 4#4#4#4# d: 01#1#01#1# H: 00#0#00#0# M: 00#0#00#0# S: 07#7#07#7#   [15:50:34] <Cindereloy> grrr, here i get:   B: January#-B#January#-B# A: Thursday#-A#Thursday#-A# j: 001#-j#001#-j# Y: 1970#-Y#1970#-Y# m: 01#-m#01#-m# w: 4#-w#4#-w# d: 01#-d#01#-d# H: 00#-H#00#-H# M: 00#-M#00#-M# S: 07#-S#07#-S#   [15:50:37] <Eric Merrill> Red Hat and Mac seem to give identical results [15:51:08] <timhunt> $ php -r 'error_reporting(0); date_default_timezone_set("UTC"); foreach (str_sp lit("BAjYmwdHMS" ) as $m) { echo "$m: " . gmstrftime("%$m", 7) . "#" . gmstrfti me("%-$m", 7) . "#" . strftime("%$m", 7) . "#" . strftime("%-$m", 7) . "#" . PHP_EOL; }' B: January##January## A: Thursday##Thursday## j: 001##001## Y: 1970##1970## m: 01##01## w: 4##4## d: 01##01## H: 00##00## M: 00##00## S: 07##07## [15:51:25] <Cindereloy> buf, that's worse. It "eats" them. [15:51:26] <timhunt> I added error_reporting 0 [15:51:48] <Cindereloy> oki, thanks. I think the conclusion is that we cannot rely on the "-" modifier. [15:51:52] <Andrew Nicols> Do you want it on Solaris too? [15:51:58] <Cindereloy> yes thanks [15:52:03] <Andrew Nicols> 504 nicols@current10x:~> /opt/csw/php5/bin/php -r 'date_default_timezone_set("UTC"); foreach (str_split("BAjYmwdHMS" ) as $m) { echo "$m: " . gmstrftime("%$m", 7) . "#" . gmstrftime("%-$m", 7) . "#" . strftime("%$m", 7) . "#" . strftime("%-$m", 7) . "#" . PHP_EOL ; }' B: January#%-B#January#%-B# A: Thursday#%-A#Thursday#%-A# j: 001#%-j#001#%-j# Y: 1970#%-Y#1970#%-Y# m: 01#%-m#01#%-m# w: 4#%-w#4#%-w# d: 01#%-d#01#%-d# H: 00#%-H#00#%-H# M: 00#%-M#00#%-M# S: 07#%-S#07#%-S#   [15:52:09] <Cindereloy> aha [15:52:28] <Cindereloy> oki, many thanks.
            Hide
            Rajesh Taneja added a comment -

            Thanks for pointing this Eloy
            I have updated all branches.

            Show
            Rajesh Taneja added a comment - Thanks for pointing this Eloy I have updated all branches.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Rajesh Taneja added a comment -

            Branches re-based

            Show
            Rajesh Taneja added a comment - Branches re-based
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks! (21, 22 & master).

            Note: I've added one extra commit on each branch to change current assertEqual() tests to assert[Identical|Same]() (simpletest|phpunit), so both value and type will be tested.

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (21, 22 & master). Note: I've added one extra commit on each branch to change current assertEqual() tests to assert [Identical|Same] () (simpletest|phpunit), so both value and type will be tested.
            Hide
            Jason Fowler added a comment -

            all good raj, thanks for the clear test instructions

            Show
            Jason Fowler added a comment - all good raj, thanks for the clear test instructions
            Hide
            Eloy Lafuente (stronk7) added a comment -

            UPDATE tracker_issues
               SET status = 'Closed',
                  comment = 'Thanks!'
            WHEN participants = 'Did a gorgeous work'
            

            This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

            Show
            Eloy Lafuente (stronk7) added a comment - UPDATE tracker_issues SET status = 'Closed', comment = 'Thanks!' WHEN participants = 'Did a gorgeous work' This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: