Moodle
  1. Moodle
  2. MDL-18375

Displaying and selecting dates throughout Moodle should allow the use of multiple calendars

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.4, 2.3.7, 2.4, 2.5
    • Fix Version/s: 2.6
    • Component/s: Calendar
    • Labels:
    • Environment:
      cross platform
    • Testing Instructions:
      Hide

      To tester - this looks like a lot, but it really isn't. The steps have been broken down.

      Test 1 - Check all PHPUnit tests pass.
      1. Run phpunit tests and ensure there are no errors.
      Test 2 - Check that installing and updating create expected database columns.
      1. Perform a fresh install and an upgrade to the latest integration branch.
      2. Check that there is a new column in the user and course table called 'calendartype' for both the fresh install and upgrade.
      3. Make sure that the default value for the course is blank and the default for the user is 'gregorian' for both the fresh install and upgrade.
      Test 3 - Test that the calendar does not display when there is no other Calendar type besides 'Gregorian'.
      1. Log in as an administrator.
      2. Visit the edit profile page, ensure there is no field with the label 'Preferred calendar'.
      3. Visit a course settings page, ensure there is no field with the label 'Force calendar' under 'Appearance'.
      Test 4 - Test the 'Date/Time' user profile field works as expected.
      1. Log in as an administrator.
      2. Visit <yoursite>/user/profile/index.php
      3. Click to add a 'Date/Time' field.
      4. Set the 'Short name' and 'name' field to whatever you like.
      5. Set the 'Start year' to 1950.
      6. Set the 'End year' to 2020.
      7. Click to save changes.
      8. Run the command "git clone https://github.com/markn86/moodle-calendartype_hijri.git hijri" in the folder calendar/type.
      9. Visit the admin/index.php page to install the new plugin.
      10. Visit the edit profile page and ensure there is a field with the label 'Preferred calendar'.
      11. Ensure this field contains the options 'Gregorian' and 'Hijri'.
      12. Scroll down to 'Other fields' and check that the 'Date/Time' field you created earlier is listed there.
      13. Enable this field and check that it is displaying today's date.
      14. Change the date to 16th August 2013.
      15. Scroll back up and change the preferred calendar to 'Hijri'.
      16. Save the user profile page.
      17. On the next page check that the 'Date/Time' user profile field is listed and says '10 Shawwāl 1434' (the day may be a 9 depending on your location).
      18. Click to edit your profile again and check that this date is also displayed in the select boxes then save.
      19. Click to edit your profile again and change 'Preferred calendar' back to 'Gregorian' then save.
      20. Click to edit your profile again and check it displays '16th August 2013'.
      21. Click to edit your profile again and change 'Preferred calendar' back to 'Hijri' then save.
      22. Visit <yoursite>/user/profile/index.php.
      23. Click to edit the profile field you created.
      24. Check that the 'Short name' is set to '1369'.
      25. Check that the 'End year' is set to '1441'.
      26. Click 'Save changes'.
      27. Click to edit the profile field you created.
      28. Check that the 'Short name' is set to '1369'.
      29. Check that the 'End year' is set to '1441'.
      30. Visit the edit profile page and change 'Preferred calendar' back to 'Gregorian'.
      31. Visit the settings for the profile field you created.
      32. Check that the 'Short name' is set to '1950'.
      33. Check that the 'End year' is set to '2020'.
      Test 5 - Test dates are saved correctly.
      1. Log in as an administrator.
      2. Visit the edit profile page and set 'Preferred calendar' to 'Hijri'.
      3. Visit a course and create an assignment.
      4. Set the name and description to whatever you want and save.
      5. Go back to edit your profile and change 'Preferred calendar' to 'Gregorian'.
      6. Click to view the assignment and check that dates displayed are today's date.
      Test 6 - Test that the course calendar setting takes preference.
      1. Log in as a student.
      2. Go to edit your profile and set the 'Preferred calendar' to 'Hijri'.
      3. As an administrator do the same to your profile.
      4. As an administrator visit the course settings for a course that student belongs to and set 'Force calendar' to 'Gregorian'.
      5. Visit that course and create an assignment and ensure the dates you can select are in Gregorian.
      6. Click on the assignment as the student and ensure the dates are displayed in Gregorian.
      7. Change the course setting 'Force calendar' to 'Do not force'.
      8. Repeat steps 5-6 but instead check the dates are in Hijri.
      Test 7 - Test that uninstalling the calendar does not break everything.
      1. Log in as an administrator.
      2. Visit the edit profile page and set your 'Preferred calendar' to 'Hijri'.
      3. Do the same as a student.
      4. Visit a course and set 'Force calendar' to 'Hijri'.
      5. As the admin visit <yoursite>/admin/plugins.php and scroll down to 'Calendar types'.
      6. Check that you cannot uninstall the Gregorian calendar type.
      7. Click to uninstall the Hijri calendar.
      8. Once you see the screen saying that it was successful remove the directory calendar/type/hijri.
      9. Click on continue.
      10. Visit your profile page and ensure dates are shown in Gregorian with no warnings.
      11. Visit the course page and ensure dates are shown in Gregorian with no warnings.
      Show
      To tester - this looks like a lot, but it really isn't. The steps have been broken down. Test 1 - Check all PHPUnit tests pass. Run phpunit tests and ensure there are no errors. Test 2 - Check that installing and updating create expected database columns. Perform a fresh install and an upgrade to the latest integration branch. Check that there is a new column in the user and course table called 'calendartype' for both the fresh install and upgrade. Make sure that the default value for the course is blank and the default for the user is 'gregorian' for both the fresh install and upgrade. Test 3 - Test that the calendar does not display when there is no other Calendar type besides 'Gregorian'. Log in as an administrator. Visit the edit profile page, ensure there is no field with the label 'Preferred calendar'. Visit a course settings page, ensure there is no field with the label 'Force calendar' under 'Appearance'. Test 4 - Test the 'Date/Time' user profile field works as expected. Log in as an administrator. Visit <yoursite>/user/profile/index.php Click to add a 'Date/Time' field. Set the 'Short name' and 'name' field to whatever you like. Set the 'Start year' to 1950. Set the 'End year' to 2020. Click to save changes. Run the command "git clone https://github.com/markn86/moodle-calendartype_hijri.git hijri" in the folder calendar/type. Visit the admin/index.php page to install the new plugin. Visit the edit profile page and ensure there is a field with the label 'Preferred calendar'. Ensure this field contains the options 'Gregorian' and 'Hijri'. Scroll down to 'Other fields' and check that the 'Date/Time' field you created earlier is listed there. Enable this field and check that it is displaying today's date. Change the date to 16th August 2013. Scroll back up and change the preferred calendar to 'Hijri'. Save the user profile page. On the next page check that the 'Date/Time' user profile field is listed and says '10 Shawwāl 1434' (the day may be a 9 depending on your location). Click to edit your profile again and check that this date is also displayed in the select boxes then save. Click to edit your profile again and change 'Preferred calendar' back to 'Gregorian' then save. Click to edit your profile again and check it displays '16th August 2013'. Click to edit your profile again and change 'Preferred calendar' back to 'Hijri' then save. Visit <yoursite>/user/profile/index.php. Click to edit the profile field you created. Check that the 'Short name' is set to '1369'. Check that the 'End year' is set to '1441'. Click 'Save changes'. Click to edit the profile field you created. Check that the 'Short name' is set to '1369'. Check that the 'End year' is set to '1441'. Visit the edit profile page and change 'Preferred calendar' back to 'Gregorian'. Visit the settings for the profile field you created. Check that the 'Short name' is set to '1950'. Check that the 'End year' is set to '2020'. Test 5 - Test dates are saved correctly. Log in as an administrator. Visit the edit profile page and set 'Preferred calendar' to 'Hijri'. Visit a course and create an assignment. Set the name and description to whatever you want and save. Go back to edit your profile and change 'Preferred calendar' to 'Gregorian'. Click to view the assignment and check that dates displayed are today's date. Test 6 - Test that the course calendar setting takes preference. Log in as a student. Go to edit your profile and set the 'Preferred calendar' to 'Hijri'. As an administrator do the same to your profile. As an administrator visit the course settings for a course that student belongs to and set 'Force calendar' to 'Gregorian'. Visit that course and create an assignment and ensure the dates you can select are in Gregorian. Click on the assignment as the student and ensure the dates are displayed in Gregorian. Change the course setting 'Force calendar' to 'Do not force'. Repeat steps 5-6 but instead check the dates are in Hijri. Test 7 - Test that uninstalling the calendar does not break everything. Log in as an administrator. Visit the edit profile page and set your 'Preferred calendar' to 'Hijri'. Do the same as a student. Visit a course and set 'Force calendar' to 'Hijri'. As the admin visit <yoursite>/admin/plugins.php and scroll down to 'Calendar types'. Check that you cannot uninstall the Gregorian calendar type. Click to uninstall the Hijri calendar. Once you see the screen saying that it was successful remove the directory calendar/type/hijri. Click on continue. Visit your profile page and ensure dates are shown in Gregorian with no warnings. Visit the course page and ensure dates are shown in Gregorian with no warnings.
    • Difficulty:
      Difficult
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-18375_master
    • Story Points:
      100
    • Rank:
      133
    • Sprint:
      BACKEND Sprint 4

      Description

      I implemented a patch to add the capability of working with other calendar systems than just Gregorian.
      I'll provide the instructions to apply the patch in following post

      1. foodle_calendar241.txt
        101 kB
        Shamim Rezaie
      2. patch.txt
        49 kB
        Shamim Rezaie
      3. patch.txt
        48 kB
        Shamim Rezaie
      1. calendar_block_bug.png
        9 kB

        Issue Links

          Activity

          Hide
          Shamim Rezaie added a comment - - edited

          0. This SQL command should be executed first:
          ALTER TABLE `mdl_user` ADD COLUMN `calendar_system` VARCHAR(20) DEFAULT NULL AFTER `lang`;

          1. The calendar system directory (can be found in the attached archived file) should be copied to the Moodle root directory. In this directory there is a file named calendar_system.class.php. This file contains the definition of calendar_system_base abstract class (interface) and also a factory function named calendar_system_factory which is used to "manufacture" an instance of desired calendar system. New Calendar systems are located in a sub-directory in the mentioned directory. They have to implement the mentioned base class. I implemented and included Gregorian and Persian calendar systems.

          2. In /lib/moodlelib.php:
          2.1. The functions make_timestamp( ), userdate( ), and usergetdate( ) should be renamed to make_timestamp_old( ), userdate_old( ), and usergetdate_old( ) respectively. New implementations for the above functions are presented.
          2.2. All calls to mktime( ) function should be replaced with $CALENDAR_SYSTEM->mktime( )
          2.3. In some places the numbers 1971 and 2035 are used as the minimum and maximum year. They should be replaced with $CALENDAR_SYSTEM->get_min_year( ) and $CALENDAR_SYSTEM->get_max_year()
          2.4. New implementation are presented for days_in_month( ) and dayofweek( ), dayofweek( ) functions.

          3. /lib/setup.php should be slightly changed.
          2.1. $CALENDAR_SYSTEM should be defined as a global object somewhere at the beginning of the file.
          2.2. /calendar_system/calendar_system.class.php should be included
          2.3. the following snippet should be inserted at the and of the file:
          if (empty($USER->calendar_system))

          { $USER->calendar_system = ''; }

          $CALENDAR_SYSTEM = calendar_system_factory::factory($USER->calendar_system);

          4. The following line should be added to /config.php (I do confirm that it would be nice to have a global setting in the moodle interface to change the above config through it):
          $CFG->calendar_system = 'gregorian';

            • It just specifies the default calendar system. Each user can override it. **

          5. In the file /lib/weblib.php, the implementation of print_date_selector( ) should be slightly changed.

          6. I stop describing changes more since they can be found in the patch file.

          Here is the list of modified files:

          • /admin/roles/assign.php
          • /calendar/event.php
          • /calendar/export.php
          • /calendar/lib.php
          • /calendar/view.php
          • /enrol/authorize/config_form.php
          • /lang/en_utf8/moodle.php
          • /lib/bennu/iCalendar_rfc2445.php
          • /lib/form/dateselector.php
          • /lib/form/datetimeselector.php
          • /lib/accesslib.php
          • /lib/moodlelib.php
          • /lib/setup.php
          • /lib/weblib.php
          • /user/extendenrol.php
          • /user/groupextendenrol.php
          • /user/editlib.php
          Show
          Shamim Rezaie added a comment - - edited 0. This SQL command should be executed first: ALTER TABLE `mdl_user` ADD COLUMN `calendar_system` VARCHAR(20) DEFAULT NULL AFTER `lang`; 1. The calendar system directory (can be found in the attached archived file) should be copied to the Moodle root directory. In this directory there is a file named calendar_system.class.php. This file contains the definition of calendar_system_base abstract class (interface) and also a factory function named calendar_system_factory which is used to "manufacture" an instance of desired calendar system. New Calendar systems are located in a sub-directory in the mentioned directory. They have to implement the mentioned base class. I implemented and included Gregorian and Persian calendar systems. 2. In /lib/moodlelib.php: 2.1. The functions make_timestamp( ), userdate( ), and usergetdate( ) should be renamed to make_timestamp_old( ), userdate_old( ), and usergetdate_old( ) respectively. New implementations for the above functions are presented. 2.2. All calls to mktime( ) function should be replaced with $CALENDAR_SYSTEM->mktime( ) 2.3. In some places the numbers 1971 and 2035 are used as the minimum and maximum year. They should be replaced with $CALENDAR_SYSTEM->get_min_year( ) and $CALENDAR_SYSTEM->get_max_year() 2.4. New implementation are presented for days_in_month( ) and dayofweek( ), dayofweek( ) functions. 3. /lib/setup.php should be slightly changed. 2.1. $CALENDAR_SYSTEM should be defined as a global object somewhere at the beginning of the file. 2.2. /calendar_system/calendar_system.class.php should be included 2.3. the following snippet should be inserted at the and of the file: if (empty($USER->calendar_system)) { $USER->calendar_system = ''; } $CALENDAR_SYSTEM = calendar_system_factory::factory($USER->calendar_system); 4. The following line should be added to /config.php (I do confirm that it would be nice to have a global setting in the moodle interface to change the above config through it): $CFG->calendar_system = 'gregorian'; It just specifies the default calendar system. Each user can override it. ** 5. In the file /lib/weblib.php, the implementation of print_date_selector( ) should be slightly changed. 6. I stop describing changes more since they can be found in the patch file. Here is the list of modified files: /admin/roles/assign.php /calendar/event.php /calendar/export.php /calendar/lib.php /calendar/view.php /enrol/authorize/config_form.php /lang/en_utf8/moodle.php /lib/bennu/iCalendar_rfc2445.php /lib/form/dateselector.php /lib/form/datetimeselector.php /lib/accesslib.php /lib/moodlelib.php /lib/setup.php /lib/weblib.php /user/extendenrol.php /user/groupextendenrol.php /user/editlib.php
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Shamim,

          nice functionality (and patch) ! B-)

          IMO is a good idea to move all the calendar/dates thing to one OOP like that, 100%. More with potential changes coming in Moodle 2.0 related with switching from our "custom" timezones support to the "native" one offered by PHP since some time ago. Doing that under one OOP umbrella will enable to do it easier, sure!

          In the other side, I think we cannot include that in the middle of one stable release (1.9.x) because changes of functionality like that aren't normally accepted in stable versions.

          So, perhaps, the way to handle this could be:

          1) Keep it as a patch for Moodle 1.9.x, ppl needing it will be able to do it.
          2) Apply it to HEAD (perhaps something will change in the final OOP implementation, but the goal of supporting multiple calendar systems will remain).
          3) Add a new set of PHP5-time-zoned calendars as alternative to the "custom-time-zoned" ones
          4) Decide if we keep both sets of calendars or we migrate to PHP5 ones or we keep using current ones.

          That way we'll end in Moodle 2.0 with multiple-calendars support available and with various problems sorted in the timezones stuff out too. And, at the same time, all the date/time/calendar stuff will be OOP, allowing new calendars to come and so on...

          In any case, very interesting stuff, thanks!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Shamim, nice functionality (and patch) ! B-) IMO is a good idea to move all the calendar/dates thing to one OOP like that, 100%. More with potential changes coming in Moodle 2.0 related with switching from our "custom" timezones support to the "native" one offered by PHP since some time ago. Doing that under one OOP umbrella will enable to do it easier, sure! In the other side, I think we cannot include that in the middle of one stable release (1.9.x) because changes of functionality like that aren't normally accepted in stable versions. So, perhaps, the way to handle this could be: 1) Keep it as a patch for Moodle 1.9.x, ppl needing it will be able to do it. 2) Apply it to HEAD (perhaps something will change in the final OOP implementation, but the goal of supporting multiple calendar systems will remain). 3) Add a new set of PHP5-time-zoned calendars as alternative to the "custom-time-zoned" ones 4) Decide if we keep both sets of calendars or we migrate to PHP5 ones or we keep using current ones. That way we'll end in Moodle 2.0 with multiple-calendars support available and with various problems sorted in the timezones stuff out too. And, at the same time, all the date/time/calendar stuff will be OOP, allowing new calendars to come and so on... In any case, very interesting stuff, thanks! Ciao
          Hide
          Shamim Rezaie added a comment - - edited

          improved version is attached.
          there is no need to modify config.php anymore.
          if you are applying the patch on an installed moodle, just execute this sql command:

          • ALTER TABLE `mdl_user` ADD COLUMN `calendar_system` VARCHAR(20) DEFAULT NULL AFTER `lang`;
            but if you are installing moodle from patched code, the is no need to execute the above sql command
          Show
          Shamim Rezaie added a comment - - edited improved version is attached. there is no need to modify config.php anymore. if you are applying the patch on an installed moodle, just execute this sql command: ALTER TABLE `mdl_user` ADD COLUMN `calendar_system` VARCHAR(20) DEFAULT NULL AFTER `lang`; but if you are installing moodle from patched code, the is no need to execute the above sql command
          Hide
          Iman Zamani added a comment -

          Hi

          Can you explain more about use this patch?

          Show
          Iman Zamani added a comment - Hi Can you explain more about use this patch?
          Hide
          Iman Zamani added a comment -

          Hi

          Can you explain more about use this patch in new version (2.0)?

          Best Regards

          Show
          Iman Zamani added a comment - Hi Can you explain more about use this patch in new version (2.0)? Best Regards
          Hide
          Shamim Rezaie added a comment -

          Iman,
          Please post your question in the related thread in the Persian Community Forum at http://moodle.org/course/view.php?id=1008

          Show
          Shamim Rezaie added a comment - Iman, Please post your question in the related thread in the Persian Community Forum at http://moodle.org/course/view.php?id=1008
          Hide
          Iman Zamani added a comment -

          Hello

          I send it there but I don't receive any answer

          Show
          Iman Zamani added a comment - Hello I send it there but I don't receive any answer
          Hide
          Shamim Rezaie added a comment -

          That's the way a forum work.
          You ask something and someone may answer. However in this case you already can find good stuff in the existing discussions.

          Show
          Shamim Rezaie added a comment - That's the way a forum work. You ask something and someone may answer. However in this case you already can find good stuff in the existing discussions.
          Hide
          Iman Zamani added a comment -

          It is so necessary for me and I have a project that I should do it with this system.
          If it is possible for you please send me your yahoo id or other id that I can chat with you
          Best Regards

          Show
          Iman Zamani added a comment - It is so necessary for me and I have a project that I should do it with this system. If it is possible for you please send me your yahoo id or other id that I can chat with you Best Regards
          Hide
          Shamim Rezaie added a comment -

          Sorry but this is not a kind of a request to be asked in Moodle Tracker.

          Show
          Shamim Rezaie added a comment - Sorry but this is not a kind of a request to be asked in Moodle Tracker.
          Hide
          Iman Zamani added a comment -

          I see
          But do you think this is good for us that this CMS don't have this module until now?!
          If you really need to help please provide an popular module that all of Iranian peoples can use it easily and don't wander them!

          Show
          Iman Zamani added a comment - I see But do you think this is good for us that this CMS don't have this module until now?! If you really need to help please provide an popular module that all of Iranian peoples can use it easily and don't wander them!
          Hide
          Shamim Rezaie added a comment -

          I'm sorry if you feel wandered.
          We're all in the same boat. you also can contribute and help others to not wander.

          Show
          Shamim Rezaie added a comment - I'm sorry if you feel wandered. We're all in the same boat. you also can contribute and help others to not wander.
          Hide
          Iman Zamani added a comment -

          I see this module on http://foodle.org.But you don't need to public it's module ?!

          Show
          Iman Zamani added a comment - I see this module on http://foodle.org.But you don't need to public it's module ?!
          Hide
          Shamim Rezaie added a comment -

          foodle uses the same code that has been shared here with a few minor customisations.
          PLEASE continue the discussion in a moodle forum. Tracker is not a forum.

          Show
          Shamim Rezaie added a comment - foodle uses the same code that has been shared here with a few minor customisations. PLEASE continue the discussion in a moodle forum. Tracker is not a forum.
          Hide
          Iman Zamani added a comment -

          Sorry

          Show
          Iman Zamani added a comment - Sorry
          Hide
          Iman Zamani added a comment -

          Hi
          Can you attach the patch file for improved version(Vrsion 2.x)?

          Show
          Iman Zamani added a comment - Hi Can you attach the patch file for improved version(Vrsion 2.x)?
          Hide
          aya added a comment -

          hi friends , i follow this functional work , but when using jalali calendar , i observation gregorian month name instead jalali month name.
          where is my problem ?
          thanks .

          Show
          aya added a comment - hi friends , i follow this functional work , but when using jalali calendar , i observation gregorian month name instead jalali month name. where is my problem ? thanks .
          Hide
          Shamim Rezaie added a comment -

          Aya,
          You can find good discussion here: http://moodle.org/mod/forum/discuss.php?d=115912
          Feel free to continue the discussion should you need to.

          Show
          Shamim Rezaie added a comment - Aya, You can find good discussion here: http://moodle.org/mod/forum/discuss.php?d=115912 Feel free to continue the discussion should you need to.
          Hide
          Taher Wusaibie added a comment -

          thank you for your efforts.
          does this support also Umm Al Qura Calendar? the problem I don't understand Farsi so I can't follow up in your discussion boards.
          thank you

          Show
          Taher Wusaibie added a comment - thank you for your efforts. does this support also Umm Al Qura Calendar? the problem I don't understand Farsi so I can't follow up in your discussion boards. thank you
          Hide
          Shamim Rezaie added a comment -

          Taher,

          I'm glad that you're interested in having multi calendar support in your Moodle.
          It CAN support any calendar system including Umm Al Qura. I assume it is the same as the Hijri calendar; right?
          As you can see I implemented the Hijri calendar here: http://foodle.org

          I'd be happy to participate in other forums as well. probably if you direct me to a discussion in your community forum.

          Show
          Shamim Rezaie added a comment - Taher, I'm glad that you're interested in having multi calendar support in your Moodle. It CAN support any calendar system including Umm Al Qura. I assume it is the same as the Hijri calendar; right? As you can see I implemented the Hijri calendar here: http://foodle.org I'd be happy to participate in other forums as well. probably if you direct me to a discussion in your community forum.
          Hide
          Taher Wusaibie added a comment -

          Thank you Shamim again for your kind reply. yes, Umm Al Qura is the same as Hijri Calendar.
          please let me know if this works with Moodle 2.2?
          I have started a new topic at this link:
          http://moodle.org/mod/forum/discuss.php?d=201994
          please help me with your valuable comments.
          Thank you very much!

          Show
          Taher Wusaibie added a comment - Thank you Shamim again for your kind reply. yes, Umm Al Qura is the same as Hijri Calendar. please let me know if this works with Moodle 2.2? I have started a new topic at this link: http://moodle.org/mod/forum/discuss.php?d=201994 please help me with your valuable comments. Thank you very much!
          Hide
          Ali Rajabpour Sanati added a comment -

          hello shamim and thank you for your nice and sharp replies,
          does the patch you have provided here work on moodle 2.2.2+ (Build: 20120419)?
          thank you in advance

          Show
          Ali Rajabpour Sanati added a comment - hello shamim and thank you for your nice and sharp replies, does the patch you have provided here work on moodle 2.2.2+ (Build: 20120419)? thank you in advance
          Hide
          Shamim Rezaie added a comment - - edited

          Taher, Ali,

          Sorry, but this doesn't work for Moodle 2.x
          However, you can read the patch file and try to follow the same logic for Moodle 2.2

          Show
          Shamim Rezaie added a comment - - edited Taher, Ali, Sorry, but this doesn't work for Moodle 2.x However, you can read the patch file and try to follow the same logic for Moodle 2.2
          Hide
          Taher Wusaibie added a comment -

          Thank you Shamim for your reply.
          Can you please give a link to the latest patch?
          I'll try to understand it and build one for moodle 2
          Can you give me a brief description of how your code works so I can make sense of what needs to be done to build it for moodle 2.
          Are you considering doing one for moodle 2 by the way?

          Show
          Taher Wusaibie added a comment - Thank you Shamim for your reply. Can you please give a link to the latest patch? I'll try to understand it and build one for moodle 2 Can you give me a brief description of how your code works so I can make sense of what needs to be done to build it for moodle 2. Are you considering doing one for moodle 2 by the way?
          Hide
          Shamim Rezaie added a comment -

          am about to reply your post in http://moodle.org/mod/forum/discuss.php?d=201994

          Show
          Shamim Rezaie added a comment - am about to reply your post in http://moodle.org/mod/forum/discuss.php?d=201994
          Hide
          jamal haykal added a comment -

          Dear Shamim and Taher,

          can you please help me if you find the solution for hijri calender for moodle2.2

          Best regards,
          Jamal Haykal

          Show
          jamal haykal added a comment - Dear Shamim and Taher, can you please help me if you find the solution for hijri calender for moodle2.2 Best regards, Jamal Haykal
          Hide
          nicolas zimo added a comment -

          hi,is there sth for moodle 2.4?

          Show
          nicolas zimo added a comment - hi,is there sth for moodle 2.4?
          Hide
          Shamim Rezaie added a comment -

          I added a new patch for Moodle 2.4.1 onward.

          If you're applying this patch on an already installed Moodle, don't forget to execute these 2 database queries:

          ALTER TABLE `mdl_course` ADD COLUMN `calendarsystem` VARCHAR(20) NULL DEFAULT ''  AFTER `lang` ;
          
          ALTER TABLE `mdl_user` ADD COLUMN `calendarsystem` VARCHAR(20) NULL DEFAULT ''  AFTER `lang` ;
          

          But if you are going to install Moodle after the patch is applied, you don't need mentioned queries. Everything will be done during the installation.

          Show
          Shamim Rezaie added a comment - I added a new patch for Moodle 2.4.1 onward. If you're applying this patch on an already installed Moodle, don't forget to execute these 2 database queries: ALTER TABLE `mdl_course` ADD COLUMN `calendarsystem` VARCHAR (20) NULL DEFAULT '' AFTER `lang` ; ALTER TABLE `mdl_user` ADD COLUMN `calendarsystem` VARCHAR (20) NULL DEFAULT '' AFTER `lang` ; But if you are going to install Moodle after the patch is applied, you don't need mentioned queries. Everything will be done during the installation.
          Hide
          Taher Wusaibie added a comment -

          Thank you Shamim for providing us this patch.
          I'm still confused on how to use it. I tried it on a test installation of moodle 2.4 and I broke it
          please clarify what we need exactly? Do we need Calendar System folder? and can you please give us the latest version? and what command do we run on the shell to run this patch?

          Thank you for great help.

          Show
          Taher Wusaibie added a comment - Thank you Shamim for providing us this patch. I'm still confused on how to use it. I tried it on a test installation of moodle 2.4 and I broke it please clarify what we need exactly? Do we need Calendar System folder? and can you please give us the latest version? and what command do we run on the shell to run this patch? Thank you for great help.
          Hide
          Shamim Rezaie added a comment -

          Hi Taher,

          Everything is included in the new patch. you don't need the old calendar_system directory.
          Please see How to apply a patch for instructions to apply patches.

          Show
          Shamim Rezaie added a comment - Hi Taher, Everything is included in the new patch. you don't need the old calendar_system directory. Please see How to apply a patch for instructions to apply patches.
          Hide
          Michael de Raadt added a comment -

          Thanks for continuing to work on this, Shamim.

          Show
          Michael de Raadt added a comment - Thanks for continuing to work on this, Shamim.
          Hide
          Mark Nelson added a comment -

          Hi Shamim,

          Thanks for all your hard work on this feature, it is much appreciated. I am going to take what you have done and make some adjustments to finally get this into core! However, since it is such a huge change it will not be backported into earlier versions, but will be introduced in 2.6.

          Regards,

          Mark

          Show
          Mark Nelson added a comment - Hi Shamim, Thanks for all your hard work on this feature, it is much appreciated. I am going to take what you have done and make some adjustments to finally get this into core! However, since it is such a huge change it will not be backported into earlier versions, but will be introduced in 2.6. Regards, Mark
          Hide
          Mark Nelson added a comment -

          Hi Shamim, I applied your patch to the latest master branch and fixed all conflicts. I have added you as the author of the initial commit and have set your email to support@foodle.org, is this email address fine, or is there another one you would prefer to use?

          Show
          Mark Nelson added a comment - Hi Shamim, I applied your patch to the latest master branch and fixed all conflicts. I have added you as the author of the initial commit and have set your email to support@foodle.org, is this email address fine, or is there another one you would prefer to use?
          Hide
          Shamim Rezaie added a comment -

          Finally I heard the good news I was waiting for more than 4 years
          Thanks for taking care of this. The email address is fine.

          Show
          Shamim Rezaie added a comment - Finally I heard the good news I was waiting for more than 4 years Thanks for taking care of this. The email address is fine.
          Hide
          Ruhollah Damavandi Kamali added a comment -

          Congratulation Shamim
          Thanks for your attention and tenacity in these four years.

          Show
          Ruhollah Damavandi Kamali added a comment - Congratulation Shamim Thanks for your attention and tenacity in these four years.
          Hide
          Mohammad ali Akbari added a comment -

          So when it will available as a stable version in moodle core, in 2.6?

          Show
          Mohammad ali Akbari added a comment - So when it will available as a stable version in moodle core, in 2.6?
          Hide
          Mark Nelson added a comment -

          Hi Mohammad, assuming all goes well this should get into 2.6.

          Show
          Mark Nelson added a comment - Hi Mohammad, assuming all goes well this should get into 2.6.
          Hide
          Shamim Rezaie added a comment -

          I'm trying to upload a new patch for this issue but Jira doesn't let me do so.
          After uploading the file it says: Cannot attach file foodle_calendar25_2013051400.08.txt: Unknown server error (403).
          Can someone please check what's wrong
          Thanks

          Show
          Shamim Rezaie added a comment - I'm trying to upload a new patch for this issue but Jira doesn't let me do so. After uploading the file it says: Cannot attach file foodle_calendar25_2013051400.08.txt: Unknown server error (403). Can someone please check what's wrong Thanks
          Hide
          Mohammad ali Akbari added a comment -

          Try with change file name! remove first dot.

          Show
          Mohammad ali Akbari added a comment - Try with change file name! remove first dot.
          Hide
          Shamim Rezaie added a comment - - edited

          tried with a.txt as well. same issue.
          this can only be fixed by someone who has access to server logs

          Edit: apparently it was due to file size. I compressed the file and uploaded it successfully

          Show
          Shamim Rezaie added a comment - - edited tried with a.txt as well. same issue. this can only be fixed by someone who has access to server logs Edit: apparently it was due to file size. I compressed the file and uploaded it successfully
          Hide
          Shamim Rezaie added a comment -

          New version attached.

          changes:

          • added setting to the Course default settings page
          • fixed a small bug in calendar/export_execute.php
          • considering that not all calendar systems might have 12 months
          • considering that the maximum number for day in day selectors should not be always 31. for example the longest month in the Hijri calendar has 30 days. So 2 new methods added to calendar_system_base: 2 new methods are introduces:
            get_max_day() and get_number_of_months()
          • added support for $date["yday"] in usergetdate for Persian and Hijri calendar
          • fixed a bug related to windows servers. added a new method to get localewincharset instead of reading it from lang file
          • minor improvements

          patch file is built using Moodle version 2013051400.08

          there is something about datetime user profile field:
          The "start year" and "end year" are always stored in Gregorian in DB. This might need some discussion.

          Show
          Shamim Rezaie added a comment - New version attached. changes: added setting to the Course default settings page fixed a small bug in calendar/export_execute.php considering that not all calendar systems might have 12 months considering that the maximum number for day in day selectors should not be always 31. for example the longest month in the Hijri calendar has 30 days. So 2 new methods added to calendar_system_base: 2 new methods are introduces: get_max_day() and get_number_of_months() added support for $date ["yday"] in usergetdate for Persian and Hijri calendar fixed a bug related to windows servers. added a new method to get localewincharset instead of reading it from lang file minor improvements patch file is built using Moodle version 2013051400.08 there is something about datetime user profile field: The "start year" and "end year" are always stored in Gregorian in DB. This might need some discussion.
          Hide
          Mohammad ali Akbari added a comment -

          Hi

          Why this version is very smaller (21KB) than previous version (101KB)?

          Show
          Mohammad ali Akbari added a comment - Hi Why this version is very smaller (21KB) than previous version (101KB)?
          Hide
          Mohammad ali Akbari added a comment -

          Because its a zip file

          Show
          Mohammad ali Akbari added a comment - Because its a zip file
          Hide
          Dan Poltawski added a comment -

          TO INTEGRATOR: sure would be good to require some behat tests with these changes.

          Show
          Dan Poltawski added a comment - TO INTEGRATOR: sure would be good to require some behat tests with these changes.
          Hide
          Shamim Rezaie added a comment -

          Sorry Mark,
          I clicked on the wrong button and this issue got assigned to me. reassigned it to you

          Show
          Shamim Rezaie added a comment - Sorry Mark, I clicked on the wrong button and this issue got assigned to me. reassigned it to you
          Hide
          Frédéric Massart added a comment -

          Hi Mark,

          great work so far, here are some of my comments.

          Design

          1. Is it really necessary to have a factore class? It seems that it does not offer much functionality.
          2. This factory method creates a new instance of a calendar type even if requested before, should we increase the memory footprint to store that instance as a singleton?
          3. I think that all the methods that do calculation should be abstract, I feel like it's dangerous to assume that a developer will extend all the methods and do the proper time manipulation.
          4. The handling of timezone (as buggy as it is in Moodle) should remain. I would assume that a timestamp should be using the timezone before any manipulation happen. This would require some strong analysis, and edit, of the existing functions.
          5. I am not sure that the calendar types should return a string... would it be enough for them to return an array to be used when outputting the date?
          6. I would like to see some validation of the parameters returned by the calendar types. In the Hijri (https://github.com/markn86/moodle-calendartype_hijri/blob/master/classes/structure.php#L164) calendar, the usergetdate returns null for yday. I don't think this should be acceptable, some plugins/code might rely on that value and transforming it to null doesn't make sense.
          7. I don't see userdate and usergetdate as being part of the calendartype, but equivalent could be called in userdate()/usergetdate() to return the required data to proceed with them.

          Type

          1. Does convert_to/from_gregorian returns hour and minute? In any case could you improve the PHP Doc so that I am sure what array keys are returned (hour vs. hours)
          2. Is it good to have a default implementation of convert_year_from_gregorian() which is based on the 1st of Jan? I suspect that other calendars might have a different year depending on the month and date. In the Chinese calendar, the year of the 1st of Jan 2013 is different than the 1st of Jul 2013. I'm not sure that this method has a real interest. At best, it should perhaps accept the same arguments than the convert_from_gregorian, then call gregorian and return the year only, but without any guessing of month and day. (Same applies to convert_year_to_gregorian())
          3. get_days(), get_months(), etc... should they return integers, strings, language strings? Could you add some documentation, I guess that lang_string objects is appropriate.
          4. Should the parameters of ::userdate() become mandatory over userdate()? (The PHPDoc says otherwise by the way)

          Factory

          1. In PHPDoc you write that the class is core_calendar\type_factory, it is \core_calendar\type_factory. It's trivial, but forgetting the leading slash could be confusing as it would seem that the namespace is used.
          2. Please declare your static methods as public/protected/private.
          3. Do not use $COURSE, use a context instead.
          4. I'd assume that the session calendar type takes priority over the course, like when overriding the language in the URL, it becomes a session variable. Maybe I'm missing something here.
          5. You could use lang_string instead of get_string().

          Test

          1. No need to reinclude the files after each test. (cf. setUp())
          2. I don't like set_calendar_type(), core_functions_test(), I feel like it makes it harder to understand what the test does.
          3. While testing userdate and usergetdate, could you add some tests with different parameters?
          4. In convert_unixtime_to_dateselector_test() what exactly are you testing? It uses the old usergetdate() function.

          General

          1. Why are the fields for calendartype limited to 30 characters?
          2. You should always add a leading \ when using \core_calendar\... if a namespace is introduced in the page you're editing it will lead to random errors.
          3. It might be worth adding a general setting to allow calendar types, or checking if there is more than 1 available before adding more settings to the user interface (profile, course, ...).

          Minor

          1. admin/settings/plugins.php:463 I don't think you need $CFG->wwwroot there.
          2. We do not use @author in PHP Doc, the copyright line is used to mention who created the file. http://docs.moodle.org/dev/Coding_style#.40copyright
          3. There is no space between <?php and the license. http://docs.moodle.org/dev/Coding_style#Files
          4. It is slower to use a for() to generate an array of numbers... as it is static you could use a real array here. (get_days/get_months)
          5. There should be the year in the @copyright statement
          Show
          Frédéric Massart added a comment - Hi Mark, great work so far, here are some of my comments. Design Is it really necessary to have a factore class? It seems that it does not offer much functionality. This factory method creates a new instance of a calendar type even if requested before, should we increase the memory footprint to store that instance as a singleton? I think that all the methods that do calculation should be abstract , I feel like it's dangerous to assume that a developer will extend all the methods and do the proper time manipulation. The handling of timezone (as buggy as it is in Moodle) should remain. I would assume that a timestamp should be using the timezone before any manipulation happen. This would require some strong analysis, and edit, of the existing functions. I am not sure that the calendar types should return a string... would it be enough for them to return an array to be used when outputting the date? I would like to see some validation of the parameters returned by the calendar types. In the Hijri ( https://github.com/markn86/moodle-calendartype_hijri/blob/master/classes/structure.php#L164 ) calendar, the usergetdate returns null for yday. I don't think this should be acceptable, some plugins/code might rely on that value and transforming it to null doesn't make sense. I don't see userdate and usergetdate as being part of the calendartype, but equivalent could be called in userdate()/usergetdate() to return the required data to proceed with them. Type Does convert_to/from_gregorian returns hour and minute? In any case could you improve the PHP Doc so that I am sure what array keys are returned (hour vs. hours) Is it good to have a default implementation of convert_year_from_gregorian() which is based on the 1st of Jan? I suspect that other calendars might have a different year depending on the month and date. In the Chinese calendar, the year of the 1st of Jan 2013 is different than the 1st of Jul 2013. I'm not sure that this method has a real interest. At best, it should perhaps accept the same arguments than the convert_from_gregorian, then call gregorian and return the year only, but without any guessing of month and day. (Same applies to convert_year_to_gregorian()) get_days(), get_months(), etc... should they return integers, strings, language strings? Could you add some documentation, I guess that lang_string objects is appropriate. Should the parameters of ::userdate() become mandatory over userdate()? (The PHPDoc says otherwise by the way) Factory In PHPDoc you write that the class is core_calendar\type_factory, it is \core_calendar\type_factory. It's trivial, but forgetting the leading slash could be confusing as it would seem that the namespace is used. Please declare your static methods as public/protected/private. Do not use $COURSE, use a context instead. I'd assume that the session calendar type takes priority over the course, like when overriding the language in the URL, it becomes a session variable. Maybe I'm missing something here. You could use lang_string instead of get_string(). Test No need to reinclude the files after each test. (cf. setUp()) I don't like set_calendar_type(), core_functions_test(), I feel like it makes it harder to understand what the test does. While testing userdate and usergetdate, could you add some tests with different parameters? In convert_unixtime_to_dateselector_test() what exactly are you testing? It uses the old usergetdate() function. General Why are the fields for calendartype limited to 30 characters? You should always add a leading \ when using \core_calendar\... if a namespace is introduced in the page you're editing it will lead to random errors. It might be worth adding a general setting to allow calendar types, or checking if there is more than 1 available before adding more settings to the user interface (profile, course, ...). Minor admin/settings/plugins.php:463 I don't think you need $CFG->wwwroot there. We do not use @author in PHP Doc, the copyright line is used to mention who created the file. http://docs.moodle.org/dev/Coding_style#.40copyright There is no space between <?php and the license. http://docs.moodle.org/dev/Coding_style#Files It is slower to use a for() to generate an array of numbers... as it is static you could use a real array here. (get_days/get_months) There should be the year in the @copyright statement
          Hide
          Shamim Rezaie added a comment -

          Hi Frédéric,

          I just had a brief look at your comments. Thank you for your attention and for good comments.

          (re Design.6) Re what you mentioned about the Hijri calendar and the return value of usergetdate: It was fixed in the latest patch I had uploaded here (foodle_calendar25_2013051400.08.txt)

          (re Type.1) calendar types has nothing to do with time elements (ie hour, minute, second). It is useless to pass time elements to convert_to/from_gregorian methods.

          (re Type.2) I do agree with your point. My +1 for removing convert_year_from_gregorian and convert_year_to_gregorian from calendar type classes. The only use of these 2 methods are inside datetime user profile field functions. I suggest to keep them internal for datetime and not put them inside calendar type classes. It's enough to have convert_to/from_gregorian in the class. convert_year_to/from_gregorian is redundant.

          (re Factory.4) about the priority of session calendar and course calendar: course calendar has always the highest priority if set. just like when a teacher forces his students to use a specific language while they are in his course.

          Show
          Shamim Rezaie added a comment - Hi Frédéric, I just had a brief look at your comments. Thank you for your attention and for good comments. (re Design.6) Re what you mentioned about the Hijri calendar and the return value of usergetdate: It was fixed in the latest patch I had uploaded here (foodle_calendar25_2013051400.08.txt) (re Type.1) calendar types has nothing to do with time elements (ie hour, minute, second). It is useless to pass time elements to convert_to/from_gregorian methods. (re Type.2) I do agree with your point. My +1 for removing convert_year_from_gregorian and convert_year_to_gregorian from calendar type classes. The only use of these 2 methods are inside datetime user profile field functions. I suggest to keep them internal for datetime and not put them inside calendar type classes. It's enough to have convert_to/from_gregorian in the class. convert_year_to/from_gregorian is redundant. (re Factory.4) about the priority of session calendar and course calendar: course calendar has always the highest priority if set. just like when a teacher forces his students to use a specific language while they are in his course.
          Hide
          Frédéric Massart added a comment -

          Thanks for your feedback Shamim.

          • I didn't review the Hijri calendar really, but I used it as a model of what/how things could be done. Thanks for the patch though
          • Mark and I had pretty much the same conclusion while chatting about this project, good thing we have another mind agreeing with that!
          • I didn't think about the forced calendar for courses, that's a good point.

          Cheers!
          Fred

          Show
          Frédéric Massart added a comment - Thanks for your feedback Shamim. I didn't review the Hijri calendar really, but I used it as a model of what/how things could be done. Thanks for the patch though Mark and I had pretty much the same conclusion while chatting about this project, good thing we have another mind agreeing with that! I didn't think about the forced calendar for courses, that's a good point. Cheers! Fred
          Hide
          Mark Nelson added a comment -

          Thanks for the thorough review Fred.

          Design

          Is it really necessary to have a factory class? It seems that it does not offer much functionality.

          • It keeps all the functionality in one location and avoids bulking up other libs. Though, I am not against moving the functionality out if that is what is decided in the end.

          This factory method creates a new instance of a calendar type even if requested before, should we increase the memory footprint to store that instance as a singleton?

          • Hmm, not sure about this. I could create a static instance but not sure if in the end it is beneficial.

          I think that all the methods that do calculation should be abstract, I feel like it's dangerous to assume that a developer will extend all the methods and do the proper time manipulation.

          • Sure, I only made it that way because the hijri example I saw used the same functionality before converting so I wanted it to be easily accessible to those who want to use it. Though, I guess it's better to ensure that the user includes this functionality.

          The handling of timezone (as buggy as it is in Moodle) should remain. I would assume that a timestamp should be using the timezone before any manipulation happen. This would require some strong analysis, and edit, of the existing functions.

          • I am not changing the current logic.

          I am not sure that the calendar types should return a string... would it be enough for them to return an array to be used when outputting the date?
          I would like to see some validation of the parameters returned by the calendar types. In the Hijri (https://github.com/markn86/moodle-calendartype_hijri/blob/master/classes/structure.php#L164) calendar, the usergetdate returns null for yday. I don't think this should be acceptable, some plugins/code might rely on that value and transforming it to null doesn't make sense.

          • That is a WIP and was used for my testing. I agree with what you have said.

          I don't see userdate and usergetdate as being part of the calendartype, but equivalent could be called in userdate()/usergetdate() to return the required data to proceed with them.

          • Sure, I have renamed them to something that is more descriptive.

          Type

          Does convert_to/from_gregorian returns hour and minute? In any case could you improve the PHP Doc so that I am sure what array keys are returned (hour vs. hours)
          Is it good to have a default implementation of convert_year_from_gregorian() which is based on the 1st of Jan? I suspect that other calendars might have a different year depending on the month and date. In the Chinese calendar, the year of the 1st of Jan 2013 is different than the 1st of Jul 2013. I'm not sure that this method has a real interest. At best, it should perhaps accept the same arguments than the convert_from_gregorian, then call gregorian and return the year only, but without any guessing of month and day. (Same applies to convert_year_to_gregorian())

          • I created these functions because the user datetime profile field only takes a year as a setting. I have adjusted the code so that when a user enters a year and saves it will pass the year and a null month and day to convert_to_gregorian which indicates that it should be at the start of the year. I can not hardcode this as 1 as not all calendars start the year on this date.

          get_days(), get_months(), etc... should they return integers, strings, language strings? Could you add some documentation, I guess that lang_string objects is appropriate.

          • Sure, I will add more PHPDocs here.

          Should the parameters of ::userdate() become mandatory over userdate()? (The PHPDoc says otherwise by the way)

          • Users should always be calling userdate, not $calendartype->userdate, so the fields will always be populated. I will remove the phpdocs.

          Factory

          In PHPDoc you write that the class is core_calendar\type_factory, it is \core_calendar\type_factory. It's trivial, but forgetting the leading slash could be confusing as it would seem that the namespace is used.

          • Done.

          Please declare your static methods as public/protected/private.

          • Done.

          Do not use $COURSE, use a context instead.

          • I am copying the exact functionality that the lang selection performs.

          I'd assume that the session calendar type takes priority over the course, like when overriding the language in the URL, it becomes a session variable. Maybe I'm missing something here.

          • Shamim addressed this above.

          You could use lang_string instead of get_string().

          • Heh, maybe you are right. That may be the first time I have used lang_string rather than get_string for some developing.

          Test

          No need to reinclude the files after each test. (cf. setUp())

          • I did not realise this function was called after every test, thanks.

          I don't like set_calendar_type(), core_functions_test(), I feel like it makes it harder to understand what the test does.

          • I think this is because you are Belgian.

          While testing userdate and usergetdate, could you add some tests with different parameters?

          • Sure, i'll expand on these.

          In convert_unixtime_to_dateselector_test() what exactly are you testing? It uses the old usergetdate() function.

          • Everyone should be calling usergetdate, not $calendartype->usergetdate. The function has not been replaced, so of course I have to call this. I am testing that the unixtime is converted into a useable date for the dateselector elements.

          General

          Why are the fields for calendartype limited to 30 characters?

          • This is the length of the folder name, do we really expect a folder name to be longer than this?

          You should always add a leading \ when using \core_calendar\... if a namespace is introduced in the page you're editing it will lead to random errors.

          • Done.

          It might be worth adding a general setting to allow calendar types, or checking if there is more than 1 available before adding more settings to the user interface (profile, course, ...).

          • This would go against the current behaviour for other settings, but I do agree. This feature won't be used that often so I could probably hide it for 99% of Moodle users who would not use an alternative from Gregorian.

          Minor

          admin/settings/plugins.php:463 I don't think you need $CFG->wwwroot there.
          We do not use @author in PHP Doc, the copyright line is used to mention who created the file. http://docs.moodle.org/dev/Coding_style#.40copyright
          There is no space between <?php and the license. http://docs.moodle.org/dev/Coding_style#Files
          It is slower to use a for() to generate an array of numbers... as it is static you could use a real array here. (get_days/get_months)
          There should be the year in the @copyright statement

          • Thanks, i'll address these.
          Show
          Mark Nelson added a comment - Thanks for the thorough review Fred. Design Is it really necessary to have a factory class? It seems that it does not offer much functionality. It keeps all the functionality in one location and avoids bulking up other libs. Though, I am not against moving the functionality out if that is what is decided in the end. This factory method creates a new instance of a calendar type even if requested before, should we increase the memory footprint to store that instance as a singleton? Hmm, not sure about this. I could create a static instance but not sure if in the end it is beneficial. I think that all the methods that do calculation should be abstract, I feel like it's dangerous to assume that a developer will extend all the methods and do the proper time manipulation. Sure, I only made it that way because the hijri example I saw used the same functionality before converting so I wanted it to be easily accessible to those who want to use it. Though, I guess it's better to ensure that the user includes this functionality. The handling of timezone (as buggy as it is in Moodle) should remain. I would assume that a timestamp should be using the timezone before any manipulation happen. This would require some strong analysis, and edit, of the existing functions. I am not changing the current logic. I am not sure that the calendar types should return a string... would it be enough for them to return an array to be used when outputting the date? I would like to see some validation of the parameters returned by the calendar types. In the Hijri ( https://github.com/markn86/moodle-calendartype_hijri/blob/master/classes/structure.php#L164 ) calendar, the usergetdate returns null for yday. I don't think this should be acceptable, some plugins/code might rely on that value and transforming it to null doesn't make sense. That is a WIP and was used for my testing. I agree with what you have said. I don't see userdate and usergetdate as being part of the calendartype, but equivalent could be called in userdate()/usergetdate() to return the required data to proceed with them. Sure, I have renamed them to something that is more descriptive. Type Does convert_to/from_gregorian returns hour and minute? In any case could you improve the PHP Doc so that I am sure what array keys are returned (hour vs. hours) Is it good to have a default implementation of convert_year_from_gregorian() which is based on the 1st of Jan? I suspect that other calendars might have a different year depending on the month and date. In the Chinese calendar, the year of the 1st of Jan 2013 is different than the 1st of Jul 2013. I'm not sure that this method has a real interest. At best, it should perhaps accept the same arguments than the convert_from_gregorian, then call gregorian and return the year only, but without any guessing of month and day. (Same applies to convert_year_to_gregorian()) I created these functions because the user datetime profile field only takes a year as a setting. I have adjusted the code so that when a user enters a year and saves it will pass the year and a null month and day to convert_to_gregorian which indicates that it should be at the start of the year. I can not hardcode this as 1 as not all calendars start the year on this date. get_days(), get_months(), etc... should they return integers, strings, language strings? Could you add some documentation, I guess that lang_string objects is appropriate. Sure, I will add more PHPDocs here. Should the parameters of ::userdate() become mandatory over userdate()? (The PHPDoc says otherwise by the way) Users should always be calling userdate, not $calendartype->userdate, so the fields will always be populated. I will remove the phpdocs. Factory In PHPDoc you write that the class is core_calendar\type_factory, it is \core_calendar\type_factory. It's trivial, but forgetting the leading slash could be confusing as it would seem that the namespace is used. Done. Please declare your static methods as public/protected/private. Done. Do not use $COURSE, use a context instead. I am copying the exact functionality that the lang selection performs. I'd assume that the session calendar type takes priority over the course, like when overriding the language in the URL, it becomes a session variable. Maybe I'm missing something here. Shamim addressed this above. You could use lang_string instead of get_string(). Heh, maybe you are right. That may be the first time I have used lang_string rather than get_string for some developing. Test No need to reinclude the files after each test. (cf. setUp()) I did not realise this function was called after every test, thanks. I don't like set_calendar_type(), core_functions_test(), I feel like it makes it harder to understand what the test does. I think this is because you are Belgian. While testing userdate and usergetdate, could you add some tests with different parameters? Sure, i'll expand on these. In convert_unixtime_to_dateselector_test() what exactly are you testing? It uses the old usergetdate() function. Everyone should be calling usergetdate, not $calendartype->usergetdate. The function has not been replaced, so of course I have to call this. I am testing that the unixtime is converted into a useable date for the dateselector elements. General Why are the fields for calendartype limited to 30 characters? This is the length of the folder name, do we really expect a folder name to be longer than this? You should always add a leading \ when using \core_calendar\... if a namespace is introduced in the page you're editing it will lead to random errors. Done. It might be worth adding a general setting to allow calendar types, or checking if there is more than 1 available before adding more settings to the user interface (profile, course, ...). This would go against the current behaviour for other settings, but I do agree. This feature won't be used that often so I could probably hide it for 99% of Moodle users who would not use an alternative from Gregorian. Minor admin/settings/plugins.php:463 I don't think you need $CFG->wwwroot there. We do not use @author in PHP Doc, the copyright line is used to mention who created the file. http://docs.moodle.org/dev/Coding_style#.40copyright There is no space between <?php and the license. http://docs.moodle.org/dev/Coding_style#Files It is slower to use a for() to generate an array of numbers... as it is static you could use a real array here. (get_days/get_months) There should be the year in the @copyright statement Thanks, i'll address these.
          Hide
          Mark Nelson added a comment -

          Another peer review would be nice.

          Note - I am currently writing testing instructions.

          Show
          Mark Nelson added a comment - Another peer review would be nice. Note - I am currently writing testing instructions.
          Hide
          Frédéric Massart added a comment -

          Hi Mark,

          I had a brief look, but it seems that you commented on what you've done and not done, so I don't see any reason to hold this issue further from integration.

          I just want to point out that I am not sure that the factory method returning the calendartype should be called "factory". Also the limit of 30 characters for the DB fields should perhaps be 28 as it's the limit of a plugin name. And the last thing is that you could probably squash some of your commits.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Mark, I had a brief look, but it seems that you commented on what you've done and not done, so I don't see any reason to hold this issue further from integration. I just want to point out that I am not sure that the factory method returning the calendartype should be called "factory". Also the limit of 30 characters for the DB fields should perhaps be 28 as it's the limit of a plugin name. And the last thing is that you could probably squash some of your commits. Cheers, Fred
          Hide
          Mark Nelson added a comment -

          I found an issue where the results of usergetdate were being passed to make_timestamp which was creating bogus timestamps. I need to find a reasonable way to resolve that without breaking everything.

          Show
          Mark Nelson added a comment - I found an issue where the results of usergetdate were being passed to make_timestamp which was creating bogus timestamps. I need to find a reasonable way to resolve that without breaking everything.
          Hide
          Mark Nelson added a comment -

          Ok, so when creating an assignment you can witness this issue. The assignment mod_form.php file calls $this->apply_admin_defaults() which uses $usermidnight = usergetmidnight(time()); to set the "Allow submissions from" date to today's date and the "Due date" field to 7 days in the future. However, usergetmidnight performs the same action specified in my last comment providing an incorrect timestamp as the year in the Hijri calendar is 1434.

          Show
          Mark Nelson added a comment - Ok, so when creating an assignment you can witness this issue. The assignment mod_form.php file calls $this->apply_admin_defaults() which uses $usermidnight = usergetmidnight(time()); to set the "Allow submissions from" date to today's date and the "Due date" field to 7 days in the future. However, usergetmidnight performs the same action specified in my last comment providing an incorrect timestamp as the year in the Hijri calendar is 1434.
          Hide
          Damyon Wiese added a comment -

          Minor issues:

          Missing "file" phpdocs:
          calendar/classes/type_base.php
          http://docs.moodle.org/dev/Coding_style#Files

          Major issue:
          convert_year_to_gregorian($year) is a nonsense API. Even in your test calendar this could give 2 different values depending on the month and day fields. The year alone is simply not enough information to convert to gregorian.

          Re your comments about usergetmidnight: if that function is not working with your patch - it should be fixed as part of this patch (sorry - I'm not fully sure what the issue is there).

          Also:
          "Sir Ankit" + "Rajey boy" - please remove these from the tests and try not to do annoying things like this in future (non-specific humour is no problem).

          Show
          Damyon Wiese added a comment - Minor issues: Missing "file" phpdocs: calendar/classes/type_base.php http://docs.moodle.org/dev/Coding_style#Files Major issue: convert_year_to_gregorian($year) is a nonsense API. Even in your test calendar this could give 2 different values depending on the month and day fields. The year alone is simply not enough information to convert to gregorian. Re your comments about usergetmidnight: if that function is not working with your patch - it should be fixed as part of this patch (sorry - I'm not fully sure what the issue is there). Also: "Sir Ankit" + "Rajey boy" - please remove these from the tests and try not to do annoying things like this in future (non-specific humour is no problem).
          Hide
          Mark Nelson added a comment - - edited
          1. What do you suggest as an alternative to convert_year_to_gregorian? I can not use convert_to_gregorian because of the issue I stated in my last commit message with the datetime profile field. A user may create a date/time profile field and set the 'Start year' field to '2012'. Another user, using the Hijri calendar, may edit the name of this field. They visit the settings page and in this case the date '1/1/2012' is converted to '7/2/1433' in Hijri. So, the year '1433' is then displayed. The user then changes the name of this field and saves the form. This is where the issue occurs, as the date '1/1/1433' is converted into Gregorian, which converts to the year '26/09/2011', so the year '2011' is saved in the DB, not '2012'. I created those functions so I could fix this in other calendars, see https://github.com/markn86/moodle-calendartype_hijri/commit/3a70806e9d55c333ee0d08b0bc060f26997ac575.
          2. usergetmidnight is working fine, it just means I can not override the function usergetdate (again, addressed in my commit messages).
          3. I pointed these out to Ankit and Raj and they didn't mind. I will remove them if it bothers you that much.
          Show
          Mark Nelson added a comment - - edited What do you suggest as an alternative to convert_year_to_gregorian? I can not use convert_to_gregorian because of the issue I stated in my last commit message with the datetime profile field. A user may create a date/time profile field and set the 'Start year' field to '2012'. Another user, using the Hijri calendar, may edit the name of this field. They visit the settings page and in this case the date '1/1/2012' is converted to '7/2/1433' in Hijri. So, the year '1433' is then displayed. The user then changes the name of this field and saves the form. This is where the issue occurs, as the date '1/1/1433' is converted into Gregorian, which converts to the year '26/09/2011', so the year '2011' is saved in the DB, not '2012'. I created those functions so I could fix this in other calendars, see https://github.com/markn86/moodle-calendartype_hijri/commit/3a70806e9d55c333ee0d08b0bc060f26997ac575 . usergetmidnight is working fine, it just means I can not override the function usergetdate (again, addressed in my commit messages). I pointed these out to Ankit and Raj and they didn't mind. I will remove them if it bothers you that much.
          Hide
          Mark Nelson added a comment -

          Please tell me what I missed in my PHPDocs?

          The requirements are -

          1. Short one-line description of the file
          2. Longer description of the file
          3. @package tag (required)
          4. @category tag (only when everything in the file is related to one of the Core APIs)
          5. @copyright (required)
          6. @license (required)

          Which I have done (except for the category tag). The file is not directly related to any core API. It is not calendar specific as it affects the whole of Moodle, and in fact isn't even implemented by the Calendar system atm when displaying dates (another issue).

          Show
          Mark Nelson added a comment - Please tell me what I missed in my PHPDocs? The requirements are - Short one-line description of the file Longer description of the file @package tag (required) @category tag (only when everything in the file is related to one of the Core APIs) @copyright (required) @license (required) Which I have done (except for the category tag). The file is not directly related to any core API. It is not calendar specific as it affects the whole of Moodle, and in fact isn't even implemented by the Calendar system atm when displaying dates (another issue).
          Hide
          Damyon Wiese added a comment -

          More info about my comments on the API:

          The case for a year field in the user profile fields is effectively saving the a date with the year being X and the month and day being jan 1 in gregorian.

          If this field is edited by a user in a different calendar, they should only be able to modify the year component of this field.

          So when displaying - make a gregorian date from the year + jan 1, convert it to users calendar, display only the year in the users calendar.

          When converting back you need to do the reverse (don't assume month 1, day 1 in the users calendar).

          ie, make a gregorian date from e.g. 2000/01/01, convert to users calendar, modify the year to the new value, convert back to gregorian, take the year component and save it in the DB.

          You can do this with only convert_to/from_gregorian and you do not need the functions convert_year_to/from_gregorian. The reason I am against those functions existing in the API is it encourages developers to do the wrong thing and those functions will lose data in the conversion.

          Show
          Damyon Wiese added a comment - More info about my comments on the API: The case for a year field in the user profile fields is effectively saving the a date with the year being X and the month and day being jan 1 in gregorian. If this field is edited by a user in a different calendar, they should only be able to modify the year component of this field. So when displaying - make a gregorian date from the year + jan 1, convert it to users calendar, display only the year in the users calendar. When converting back you need to do the reverse (don't assume month 1, day 1 in the users calendar). ie, make a gregorian date from e.g. 2000/01/01, convert to users calendar, modify the year to the new value, convert back to gregorian, take the year component and save it in the DB. You can do this with only convert_to/from_gregorian and you do not need the functions convert_year_to/from_gregorian. The reason I am against those functions existing in the API is it encourages developers to do the wrong thing and those functions will lose data in the conversion.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Mark Nelson added a comment -

          Ok, I created new hidden form fields that stored the day and month for the start and end year that was obtained from converting the date from Gregorian into the calendar type being used. If the date was not actually changed then it uses those values in the conversion back to Gregorian which means the date is converted correctly. Submitting back to integration.

          Show
          Mark Nelson added a comment - Ok, I created new hidden form fields that stored the day and month for the start and end year that was obtained from converting the date from Gregorian into the calendar type being used. If the date was not actually changed then it uses those values in the conversion back to Gregorian which means the date is converted correctly. Submitting back to integration.
          Hide
          Mark Nelson added a comment -

          Also, please do not squash this into one commit. It took some time to develop and I do not think having 1 commit is a fair representation of the time involved.

          Show
          Mark Nelson added a comment - Also, please do not squash this into one commit. It took some time to develop and I do not think having 1 commit is a fair representation of the time involved.
          Hide
          Damyon Wiese added a comment -

          The list of commit messages is not about fair or not fair - it is about meaningful git messages that describe logical changes. The number of developer commits is a very non-sense way of measuring a developers work.

          I do not want to have to squash issues in integration - this should be done by the developer before sending for integration. It is a pain to look at an issue for 30 mins, pass all the tests and then find 20 rubbish commit messages. At that point I can either squash them all or reject the issue (which means I will have to re-review it again, and run the tests again when the issue is sent back).

          From this patch - here is a list of commits messages that do not provide any value and IMO should have been squashed before sending to integration:

          187cf746aa4a5939dcf8b9ff73e055e5e5f921f3
          36ff5f87deb7a2897e81e7c54fb114fa8f2000c8
          3abb0409b25cabeda8b2ebef47b0b9326bce7375
          8d1d34f7917909d29e2f4ecf82a22832c814ed91
          33106a67a5dc8fd89040cd702b5a479b09adfc3f
          f5e4b96c91baf3f2c1d461ffa7b255560ef6d504
          8927c8ee22eae1912af9c0eab519b9e8228d8cf9
          7616a0eb55bcad6684acc2f9683875141534c670
          8267fbb88a9785a341b019686dddbf50dfd1ed63

          ie - about half. You should consider squashing them before sending this to integration again.

          Show
          Damyon Wiese added a comment - The list of commit messages is not about fair or not fair - it is about meaningful git messages that describe logical changes. The number of developer commits is a very non-sense way of measuring a developers work. I do not want to have to squash issues in integration - this should be done by the developer before sending for integration. It is a pain to look at an issue for 30 mins, pass all the tests and then find 20 rubbish commit messages. At that point I can either squash them all or reject the issue (which means I will have to re-review it again, and run the tests again when the issue is sent back). From this patch - here is a list of commits messages that do not provide any value and IMO should have been squashed before sending to integration: 187cf746aa4a5939dcf8b9ff73e055e5e5f921f3 36ff5f87deb7a2897e81e7c54fb114fa8f2000c8 3abb0409b25cabeda8b2ebef47b0b9326bce7375 8d1d34f7917909d29e2f4ecf82a22832c814ed91 33106a67a5dc8fd89040cd702b5a479b09adfc3f f5e4b96c91baf3f2c1d461ffa7b255560ef6d504 8927c8ee22eae1912af9c0eab519b9e8228d8cf9 7616a0eb55bcad6684acc2f9683875141534c670 8267fbb88a9785a341b019686dddbf50dfd1ed63 ie - about half. You should consider squashing them before sending this to integration again.
          Hide
          Mark Nelson added a comment -

          You will need to provide new hashes since I have updated it before I read this messages.

          I find the commit messages follow a logic pattern on how this was developed and the term 'useful' seems to be extremely subjective. It appears, rather than just pulling it in (which wouldn't be an issue since commit messages don't matter, right?), you would rather the person rebase, reorder their commits and fix the conflicts just so there was an acceptable number of commits (in your eyes). The issue MDL-39961 contained meaningful messages (imo), yet it was squashed to 1. The same with MDL-39958 which was squashed to 1 (with the wrong MDL number used in the squashed commit message).

          If number of commits is not a measurement why do we have that dashboard that is 'reviewed' after every release?

          Show
          Mark Nelson added a comment - You will need to provide new hashes since I have updated it before I read this messages. I find the commit messages follow a logic pattern on how this was developed and the term 'useful' seems to be extremely subjective. It appears, rather than just pulling it in (which wouldn't be an issue since commit messages don't matter, right?), you would rather the person rebase, reorder their commits and fix the conflicts just so there was an acceptable number of commits (in your eyes). The issue MDL-39961 contained meaningful messages (imo), yet it was squashed to 1. The same with MDL-39958 which was squashed to 1 (with the wrong MDL number used in the squashed commit message). If number of commits is not a measurement why do we have that dashboard that is 'reviewed' after every release?
          Hide
          Mark Nelson added a comment -

          Ok, I have spent a few hours rebasing, reordering and fixing the resulting conflicts in my commits, time well spent. Hopefully now they are deemed useful.

          Show
          Mark Nelson added a comment - Ok, I have spent a few hours rebasing, reordering and fixing the resulting conflicts in my commits, time well spent. Hopefully now they are deemed useful.
          Hide
          Dan Marsden added a comment -

          Hey Mark - I understand your concern over the number of commits

          For example Mayank has over 100 commits in MDL-39910 that were rebased into a single commit - that single commit as a number of commits doesn't represent the work done at all - but there are a large number of advantages in having it as a single commit. Although some things do use a count of commits like ohloh and the dashboard you refer to - it's just not practical for the number of commits in a patch to represent effort. It is more important for the git history to be useful - having a single commit also means that partners/other developers can easily cherry-pick your change onto branches as required - (or revert them if needed)

          Have a chat to Shane about commit numbers.... he's got a bunch in the original CVS branch that were never attributed to him as he just used MD's account to push stuff into cvs.

          Show
          Dan Marsden added a comment - Hey Mark - I understand your concern over the number of commits For example Mayank has over 100 commits in MDL-39910 that were rebased into a single commit - that single commit as a number of commits doesn't represent the work done at all - but there are a large number of advantages in having it as a single commit. Although some things do use a count of commits like ohloh and the dashboard you refer to - it's just not practical for the number of commits in a patch to represent effort. It is more important for the git history to be useful - having a single commit also means that partners/other developers can easily cherry-pick your change onto branches as required - (or revert them if needed) Have a chat to Shane about commit numbers.... he's got a bunch in the original CVS branch that were never attributed to him as he just used MD's account to push stuff into cvs.
          Hide
          Damyon Wiese added a comment -

          Thanks Mark,

          This has been integrated to master only.

          Show
          Damyon Wiese added a comment - Thanks Mark, This has been integrated to master only.
          Hide
          Adrian Greeve added a comment - - edited

          Hi Mark,

          Thanks so much for your hard work on this issue. I've been looking at creating a Japanese calendar type for this plug in. The only problem is that I can't configure the years (There is no get_years() method in the class) to display the Japanese format. I'll create a new issue (and probably a patch) to include this functionality. (MDL-41664)

          Thanks.

          Show
          Adrian Greeve added a comment - - edited Hi Mark, Thanks so much for your hard work on this issue. I've been looking at creating a Japanese calendar type for this plug in. The only problem is that I can't configure the years (There is no get_years() method in the class) to display the Japanese format. I'll create a new issue (and probably a patch) to include this functionality. ( MDL-41664 ) Thanks.
          Hide
          Damyon Wiese added a comment -

          Note - this is failing in the ci server version checker.

          INFO: Correct component found: calendartype_gregorian
          ERROR: Component calendartype_gregorian not valid for that file

          I've added a major version bump to see if this fixes it.

          Show
          Damyon Wiese added a comment - Note - this is failing in the ci server version checker. INFO: Correct component found: calendartype_gregorian ERROR: Component calendartype_gregorian not valid for that file I've added a major version bump to see if this fixes it.
          Hide
          Mark Nelson added a comment -

          Thanks Adrian, commented on your issue.

          Dan Marsden - I agree 100%. Damyon did have the assignment module squashed into one commit when it was integrated. It's just that there is a lot of emphasis on commit numbers among the developers, which should be something discussed outside of tracker. Damyon I apologise if I overreacted.

          Show
          Mark Nelson added a comment - Thanks Adrian, commented on your issue. Dan Marsden - I agree 100%. Damyon did have the assignment module squashed into one commit when it was integrated. It's just that there is a lot of emphasis on commit numbers among the developers, which should be something discussed outside of tracker. Damyon I apologise if I overreacted.
          Hide
          Andrew Davis added a comment -

          Tests 1-3 have passed so far. Testing #4 now.

          Show
          Andrew Davis added a comment - Tests 1-3 have passed so far. Testing #4 now.
          Hide
          Andrew Davis added a comment -

          Raised MDL-41721 although I believe it is unrelated to the changes made in this issue.

          Show
          Andrew Davis added a comment - Raised MDL-41721 although I believe it is unrelated to the changes made in this issue.
          Hide
          Mark Nelson added a comment -

          Hi Andrew, yep I ran into that as well while developing this (should have created an issue - thanks for doing that). You are correct, it is unrelated.

          Show
          Mark Nelson added a comment - Hi Andrew, yep I ran into that as well while developing this (should have created an issue - thanks for doing that). You are correct, it is unrelated.
          Hide
          Andrew Davis added a comment -

          Phew. Ok, all working as described. Passing. Well done.

          Show
          Andrew Davis added a comment - Phew. Ok, all working as described. Passing. Well done.
          Hide
          Damyon Wiese added a comment -

          This issue along with 77 of it's friends has been sent upstream and released to the world.

          Thankyou for your contribution.

          Show
          Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.
          Hide
          Shamim Rezaie added a comment -

          A bug has been introduced during the code refactoring. No matter what the user's choice is, the calendar block shows days based on the Gregorian calendar type.

          Please see the screenshot I attached. As you can see in the attached picture, the calendar block claims that it is displaying Shawwāl 1434, but the fact is that it is displaying September 2013.

          Show
          Shamim Rezaie added a comment - A bug has been introduced during the code refactoring. No matter what the user's choice is, the calendar block shows days based on the Gregorian calendar type. Please see the screenshot I attached. As you can see in the attached picture, the calendar block claims that it is displaying Shawwāl 1434, but the fact is that it is displaying September 2013.
          Hide
          Shamim Rezaie added a comment -

          similar bug shows up when displaying calendar detailed page.

          There are a lot of calls to usergetdate function in the code. So either all these calls should be replaced with $calendartype->timestamp_to_date_array() OR the implementation of usergetdate should be changed to call $calendartype->timestamp_to_date_array() internally (as it was in the original patch) and probably display a debug message saying usergetdate is deprecated and should not be called directly.

          Sorry but this issue has to be re-opened.

          Show
          Shamim Rezaie added a comment - similar bug shows up when displaying calendar detailed page. There are a lot of calls to usergetdate function in the code. So either all these calls should be replaced with $calendartype->timestamp_to_date_array() OR the implementation of usergetdate should be changed to call $calendartype->timestamp_to_date_array() internally (as it was in the original patch) and probably display a debug message saying usergetdate is deprecated and should not be called directly. Sorry but this issue has to be re-opened.
          Hide
          Mark Nelson added a comment -

          Hi Shamim,

          This is a known issue and something that will be worked on in a separate issue. We will not be reopening this one. We can not overwrite the usergetdate function as is not only used to display dates, it is also passed to the function make_timestamp. This means users using another calendar type other than Gregorian will generate incorrect timestamps which may be saved in the DB or used to populate the date_selector and date_time_selector form elements. In order to fix what you have mentioned it requires rewriting parts of the calendar code, not changing how the usergetdate function behaves.

          Regards,

          Mark

          Show
          Mark Nelson added a comment - Hi Shamim, This is a known issue and something that will be worked on in a separate issue. We will not be reopening this one. We can not overwrite the usergetdate function as is not only used to display dates, it is also passed to the function make_timestamp. This means users using another calendar type other than Gregorian will generate incorrect timestamps which may be saved in the DB or used to populate the date_selector and date_time_selector form elements. In order to fix what you have mentioned it requires rewriting parts of the calendar code, not changing how the usergetdate function behaves. Regards, Mark
          Hide
          Mark Nelson added a comment -

          Shamim, please see MDL-41792.

          Show
          Mark Nelson added a comment - Shamim, please see MDL-41792 .
          Hide
          Shamim Rezaie added a comment - - edited

          Dear Mark,

          Thank you so much for spending so much time on this issue.
          To be honest, I still think that the way that the original patch is handling calendar types is more robust universal.

          Using the way that the patch was handling calendar types, when the calendar was set to Hijri, every date elements were in Hijri (unless otherwise were explicitly stated). So usergetdate was dealing with Hijri date elements (day, month,year). Even make_timestamp was accepting Hijri arguments.
          If there was a need, in some very rare cases, to (no matter what the user's preference was) pass Gregorian date to a function, the patch was doing something like $gregorian_calendar_instance->maketimestamp() or $gregorian_calendar_instance->usergetdate().

          I totally understand your concern about some third parties. you had stated in one of your commit messages: "We can not expect third party modules to change the calendar type depending on the format they pass to these functions".

          But I'm fairly confident that there's nothing to be worried about. I have been using this patch (and it's older version which were following the same logic) for about 5 years, and on over 300 Moodle instances and NEVER had any issues with any of the third parties.

          I admit that there "MIGHT BE" a third party that I haven't faced until now that relies on Gregorian dates but frankly speaking... which one is worse? Ending up with a buggy core or having a buggy 3rd party plugin (if there's any) which can be easily patched right after the first bug report?

          Also, lets look at this from a different point. as a developer when I see a function named "usergetdate", I expect the function to get the date using user's preferences. This is what its name suggests to me.

          Sorry if my message got too long. Please look at it as a suggestion and feel free to ignore it.
          Shamim

          Show
          Shamim Rezaie added a comment - - edited Dear Mark, Thank you so much for spending so much time on this issue. To be honest, I still think that the way that the original patch is handling calendar types is more robust universal. Using the way that the patch was handling calendar types, when the calendar was set to Hijri, every date elements were in Hijri (unless otherwise were explicitly stated). So usergetdate was dealing with Hijri date elements (day, month,year). Even make_timestamp was accepting Hijri arguments. If there was a need, in some very rare cases, to (no matter what the user's preference was) pass Gregorian date to a function, the patch was doing something like $gregorian_calendar_instance->maketimestamp() or $gregorian_calendar_instance->usergetdate(). I totally understand your concern about some third parties. you had stated in one of your commit messages: "We can not expect third party modules to change the calendar type depending on the format they pass to these functions". But I'm fairly confident that there's nothing to be worried about. I have been using this patch (and it's older version which were following the same logic) for about 5 years, and on over 300 Moodle instances and NEVER had any issues with any of the third parties. I admit that there "MIGHT BE" a third party that I haven't faced until now that relies on Gregorian dates but frankly speaking... which one is worse? Ending up with a buggy core or having a buggy 3rd party plugin (if there's any) which can be easily patched right after the first bug report? Also, lets look at this from a different point. as a developer when I see a function named "usergetdate", I expect the function to get the date using user's preferences. This is what its name suggests to me. Sorry if my message got too long. Please look at it as a suggestion and feel free to ignore it. Shamim
          Hide
          Mark Nelson added a comment - - edited

          Hi Shamim,

          The use of "$gregorian_calendar_instance->maketimestamp()" and "$gregorian_calendar_instance->usergetdate()" is extremely crazy imo. We can not expect everyone who writes code for core to do this for rare cases, or for third party modules, and the result if they don't is completely messed up timestamps in the DB. You may not have experienced it, but it doesn't mean it won't happen. I personally think we can not risk this.

          I agree 100% that when I look at the function name 'usergetdate' I expect it to return the date in the calendar type being used. However, looking at where usergetdate is used, there are a lot of cases where the result it is then passed to another function (even sometimes PHP's date function!!) which expects Gregorian dates. Overwriting this is way too risky so I decided to go with only altering userdate, which is only used to display the dates. Surely if Moodle was designed from scratch we would design it with multi-calendar support in mind, but unfortunately that is not the case.

          In the mean time, please keep discussion in the other tracker issue where your input will be very much appreciated. In the following days I hope to have some progress which would benefit from your insight.

          Thanks.

          Show
          Mark Nelson added a comment - - edited Hi Shamim, The use of "$gregorian_calendar_instance->maketimestamp()" and "$gregorian_calendar_instance->usergetdate()" is extremely crazy imo. We can not expect everyone who writes code for core to do this for rare cases, or for third party modules, and the result if they don't is completely messed up timestamps in the DB. You may not have experienced it, but it doesn't mean it won't happen. I personally think we can not risk this. I agree 100% that when I look at the function name 'usergetdate' I expect it to return the date in the calendar type being used. However, looking at where usergetdate is used, there are a lot of cases where the result it is then passed to another function (even sometimes PHP's date function!!) which expects Gregorian dates. Overwriting this is way too risky so I decided to go with only altering userdate, which is only used to display the dates. Surely if Moodle was designed from scratch we would design it with multi-calendar support in mind, but unfortunately that is not the case. In the mean time, please keep discussion in the other tracker issue where your input will be very much appreciated. In the following days I hope to have some progress which would benefit from your insight. Thanks.
          Hide
          Mark Nelson added a comment -

          This issue has been split up into two issues. I have created an epic MDL-41866 which lists both of the tasks that need to be done.

          Show
          Mark Nelson added a comment - This issue has been split up into two issues. I have created an epic MDL-41866 which lists both of the tasks that need to be done.
          Hide
          Mark Nelson added a comment -

          Sorry for all the noise.

          Show
          Mark Nelson added a comment - Sorry for all the noise.
          Hide
          Martin Dougiamas added a comment -

          Just been trying this ... why is there no site setting for this? Surely it's going to be much more common to set this for a whole site than just for a user or a course?

          Show
          Martin Dougiamas added a comment - Just been trying this ... why is there no site setting for this? Surely it's going to be much more common to set this for a whole site than just for a user or a course?
          Hide
          Shamim Rezaie added a comment -

          Hi Martin,

          The initial patch had this feature included. It must have been mistakenly overlooked at some point.

          Show
          Shamim Rezaie added a comment - Hi Martin, The initial patch had this feature included. It must have been mistakenly overlooked at some point.
          Hide
          Mark Nelson added a comment -

          Hmm, it seems I may have missed that functionality, sorry. The original patch created a new settings page which I removed completely as there were a lot of unnecessary settings (such as searching for updates, updating the plugins, deleting them) which are all performed by Moodle core. I changed and removed a lot of the original patch, so I apologise for this oversight. I have created MDL-42932 to add this setting in the current settings page.

          Show
          Mark Nelson added a comment - Hmm, it seems I may have missed that functionality, sorry. The original patch created a new settings page which I removed completely as there were a lot of unnecessary settings (such as searching for updates, updating the plugins, deleting them) which are all performed by Moodle core. I changed and removed a lot of the original patch, so I apologise for this oversight. I have created MDL-42932 to add this setting in the current settings page.
          Hide
          Iman Zamani added a comment - - edited

          Hi Mark
          Are you have any idea for change structure.php file in v 2.6 to SHMASI Persian calendar?
          Thanks

          Show
          Iman Zamani added a comment - - edited Hi Mark Are you have any idea for change structure.php file in v 2.6 to SHMASI Persian calendar? Thanks
          Hide
          Mohammad ali Akbari added a comment -

          Is "change calendar" features still available in 2.6? also how can I set default calendar site wide?

          Show
          Mohammad ali Akbari added a comment - Is "change calendar" features still available in 2.6? also how can I set default calendar site wide?
          Hide
          Behrooz Kashaf Rashti added a comment -

          @Iman Zamani & @Mohammad ali Akbari, Follow the instructions as mentioned above.

          Field Tab | Testing Instructions

          Show
          Behrooz Kashaf Rashti added a comment - @Iman Zamani & @Mohammad ali Akbari, Follow the instructions as mentioned above. Field Tab | Testing Instructions

            People

            • Votes:
              138 Vote for this issue
              Watchers:
              48 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile