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
    • Rank:
      39014

      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'];

        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: