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

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))

$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

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

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

Hi

Can you explain more about use this patch?

Hi

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

Best Regards

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

Hello

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

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.

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

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

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!

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

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

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.

Sorry

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

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 .

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.

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

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.

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!

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

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

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?

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

Dear Shamim and Taher,

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

Best regards,
Jamal Haykal

hi,is there sth for moodle 2.4?

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.

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.

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.

Thanks for continuing to work on this, Shamim.

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

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?

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.

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

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

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

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

Try with change file name! remove first dot.

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

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.

Hi

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

Because its a zip file

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

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

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

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.

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

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.

Another peer review would be nice.

Note - I am currently writing testing instructions.

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

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.

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.

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).

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.

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).

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.

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

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.

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.

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.

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?

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.

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.

Thanks Mark,

This has been integrated to master only.

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.

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.

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.

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

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

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.

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

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

Thankyou for your contribution.

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.

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.

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

Shamim, please see MDL-41792.

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

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.

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.

Sorry for all the noise.

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?

Hi Martin,

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

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.

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

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

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

Field Tab | Testing Instructions