Drupal doesn't support other calendars, Solar Calendar(Gregorian calendar)
here is some works for supporting it using a module. However, it needs a custom hook to be added, which means editing common.inc

Drupal did a great job supporting RTL languages and include many translations. Now we need to step it up in 8.x
I basically added the custom hoon into common.inc
If you have a better way please do inform me.
PS: this hook can be used for all kinds of Calendar systems, more info http://drupal.org/project/calendar_systems

CommentFileSizeAuthor
#156 drupal-format_date_alter-1178342-156-8.6.x.patch1.37 KBdeviantintegral
#156 drupal-format_date_alter-1178342-156.patch1.37 KBdeviantintegral
#151 drupal-format_date_alter-1178342-150.patch2.32 KBbgilhome
#149 drupal-format_date_alter-1178342-149.patch2.32 KBbgilhome
#136 drupal-format_date_alter-1178342-136.patch4.98 KBsinasalek
#133 drupal-format_date_alter-1178342-132.patch4.98 KBsinasalek
#131 drupal-format_date_alter-1178342-131.patch4.83 KBsinasalek
#129 drupal-format_date_alter-1178342-128.patch5.87 KBsinasalek
#127 drupal-format_date_alter-1178342-127.patch5.43 KBsinasalek
#124 drupal-format_date_alter-1178342-122.patch4.52 KBsinasalek
#122 drupal-format_date_alter-1178342-122.patch.txt4.52 KBsinasalek
#117 drupal-format_date_alter-1178343-114.patch4.36 KBsepehr.sadatifar
#114 drupal-format_date_alter-1178343.patch4.52 KBsinasalek
#112 drupal-format_date_alter-1178342.patch4.52 KBsinasalek
#106 drupal-format_date_alter-1178342.patch4.52 KBsinasalek
#102 1178342-hook-format-date-alter-D7-do-not-test.patch1.37 KBDave Reid
#85 hook_format_date_alter_4.patch4.1 KBHadi Farnoud
#83 hook_format_date_alter_5.patch4.09 KBHadi Farnoud
#74 ab_before2.txt1.26 KBbdgreen
#74 ab_after1.txt1.26 KBbdgreen
#70 before.d8.xhprof.txt972.84 KBcweagans
#70 after.d8.xhprof.txt972.63 KBcweagans
#70 Screenshot at 2013-05-14 15:43:49.png26.27 KBcweagans
#66 ab2_hook_format_date_alter_4.patch_before.txt1.25 KBbdgreen
#66 ab2_hook_format_date_alter_4.patch_after.txt1.25 KBbdgreen
#63 hook_format_date_alter.patch4.18 KBsinasalek
#60 hook_format_date_alter.patch4.21 KBsinasalek
#58 hook_format_date_alter.patch4.2 KBsinasalek
#55 hook_format_date_alter.patch4.36 KBsinasalek
#51 hook_format_date_alter.patch3.13 KBsinasalek
#49 hook_format_date_alter.patch4.77 KBsinasalek
#43 drupal.1178342.43.hook_format_date_alter.patch4.77 KBGaelan
#43 drupal.1178342.43.hook_format_date_alter.interdiff.txt1008 bytesGaelan
#41 drupal.1178342.39.hook_format_date_alter.patch4.12 KBGaelan
#41 drupal.1178342.39.hook_format_date_alter.interdiff.txt1.26 KBGaelan
#39 drupal.1178342.39.hook_format_date_alter.patch4.73 KBGaelan
#39 drupal.1178342.39.hook_format_date_alter.interdiff.txt1.87 KBGaelan
#36 hook_format_date_alter_test.1178342.36.patch2.45 KBwuinfo - Bill Wu
#36 hook_format_date_alter.1178342.36.patch4.28 KBwuinfo - Bill Wu
#35 hook_format_date_alter.1178342.35.patch4.28 KBwuinfo - Bill Wu
#24 drupal.1178342.24.hook_format_date_alter.patch1.84 KBGaelan
#24 drupal.1178342.24.hook_format_date_alter.interdiff.txt2.18 KBGaelan
#18 core-format_date_alter-1178342-18.patch1.66 KBsinasalek
#10 1178342-core_format_date_hook_0.patch884 bytesHadi Farnoud
#8 1178342-core_format_date_hook.patch975 bytesHadi Farnoud
#2 1178342-core_format_date_hook.patch1008 bytesHadi Farnoud
core_format_date_hook.patch1008 bytesHadi Farnoud
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, core_format_date_hook.patch, failed testing.

Hadi Farnoud’s picture

aspilicious’s picture

Please...

+++ b/includes/common.incundefined
@@ -1853,6 +1853,19 @@ function format_interval($timestamp, $granularity = 2, $langcode = NULL) {
+// Custom hook for other calendar supports (Arabian, Persian, etc.) originally written by Sina Salek

Come on are you giving yourself credit in the code?

+++ b/includes/common.incundefined
@@ -1853,6 +1853,19 @@ function format_interval($timestamp, $granularity = 2, $langcode = NULL) {
+          foreach (module_implements('format_date') AS $module) {
+            if ($module!='date') {
+                $function = $module .'_format_date';
+                $r=$function($timestamp, $type, $format, $timezone, $langcode);
+
+                if ($r!=false) {
+                    return $r;
+                }
+            }
+          }

- This is formatted very badly.
- AS with capital?
- Why excluding an non core module like date?
- Can't this be done through contrib modules like date?

Powered by Dreditor.

aspilicious’s picture

http://drupal.org/project/calendar_systems is working why need it to be in core?

Hadi Farnoud’s picture

its working if you add a custom hook in common.inc
this patch is not calendar_systems. it's just a hook to make it work. Not possible with Date module unfortunately!

Hadi Farnoud’s picture

I'm giving Sina credit not myself, does it really matter?
Date does not support other calendars

aspilicious’s picture

1) Yes its bad to credit someone in core.
2) It needs to follow the coding standards (don't use tabs, AS ==> as, ...)
3) the date hack has to go, if this becomes an official hook date module has to change their function. Or you should choose another hook name.

Hmm so that module only works if you hack core... Sounds bad...

Hadi Farnoud’s picture

I think a core module that supports other calendar systems could be a great boost in Drupal multilingual support.
I agree modifying core files is a bad idea for a module, but I couldn't think of better (proper) way of doing this (since the date in DB should always be Gregorian).

PS: 1 and 2 fixed.

aspilicious’s picture

+++ b/includes/common.incundefined
@@ -1853,6 +1853,19 @@ function format_interval($timestamp, $granularity = 2, $langcode = NULL) {
+
+// Custom hook for other calendar supports (Arabian, Persian, etc.)
+          foreach (module_implements('format_date') as $module) {
+            if ($module!='date') {
+                $function = $module .'_format_date';
+                $r=$function($timestamp, $type, $format, $timezone, $langcode);
+
+                if ($r!=false) {
+                    return $r;
+                }
+            }
+          }

Still to much space on the left side...

Powered by Dreditor.

Hadi Farnoud’s picture

aspilicious’s picture

I stil don't like the date module check, thats nothing we can do in core. And this needs review from some more experienced people. I'll try to get them look at this.

klonos’s picture

Thanx for filling this Hadi and for the provided initial patch.

Hey bram, please give a man a chance:

Hmm so that module only works if you hack core... Sounds bad...

If we reacted like that to every single core alteration, we would never move forth. There must be other examples as well, but simpletest is the one that comes to mind. Not such a bad idea after all huh? That altered core too, but it did happen and it is part of the developing circle now. Bottom line being fear of touching core should not be an argument for not accepting patches/changes.

...Other than that thanx for the code review.

sinasalek’s picture

subscribed

sourcesoft’s picture

I'm really looking forward to this hook since date always stores Gregorian and it's impossible without hacking core. Good job sinasalek with calendar_system, but we still need this at least in D8.

aspilicious’s picture

I still didn't get an answer why there is a date module check.
I have the feeling date_module should provide a hook that accepts other date systems.

Gábor Hojtsy’s picture

Hadi Farnoud’s picture

it is somehow related. they both have the same aim. #1811912: Add pluggable calendar backend to core and centralize date translation is going nowhere?

sinasalek’s picture

Title: Other calendars support » Allowing contributed modules to alter format_date function result
FileSize
1.66 KB

This is not only for supporting multi calendars , it's a generic hook that can used to alter format_date function.
Two other use cases are
1- To convert numbers into native Unicode numbers depending on site's language
2- To replace specific date formats with another ones on the fly

sinasalek’s picture

Assigned: Hadi Farnoud » sinasalek
Status: Needs work » Needs review

Changing issue status

Hadi Farnoud’s picture

Status: Needs review » Reviewed & tested by the community

It looks good to me Sina.

Hadi Farnoud’s picture

Priority: Normal » Major
xjm’s picture

Title: Allowing contributed modules to alter format_date function result » Allow contributed modules to alter the format_date() function result
Priority: Major » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: -calendar, -date API +Needs tests

Thanks @Hadi Farnoud and @sinasalek! This looks like it could be a useful feature. I have a few notes to improve the patch. We should also add an automated test that codifies the expected behavior of the alter hook.

  1. +++ b/core/includes/common.incundefined
    @@ -2007,7 +2007,21 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
    -  return $date_time->format($format, $settings);
    +  $formatted_date = $date_time->format($format, $settings);
    +  ¶
    +  $context = array(
    +    'settings' => $settings,
    +    'date_time' => $date_time,
    +    'timezones' => $timezones,
    +    'timestamp' => $timestamp,
    +    'type' => $type,
    +    'format' => $format,
    +    'timezone' => $timezone,
    +    'langcode' => $langcode,
    +  );
    +  drupal_alter('format_date', $formatted_date, $context);
    +  ¶
    

    There's trailing whitespace in this hunk that needs to be removed.

  2. +++ b/core/modules/system/system.api.phpundefined
    @@ -3946,6 +3946,29 @@ function hook_filetransfer_info_alter(&$filetransfer_info) {
    + * A module may implement this hook in order to alter the formatted date string
    

    This should end in a period.

  3. +++ b/core/modules/system/system.api.phpundefined
    @@ -3946,6 +3946,29 @@ function hook_filetransfer_info_alter(&$filetransfer_info) {
    + * @param $formatted_date
    ...
    + * @param $context
    

    The parameters should have datatype documentation. See: http://drupal.org/node/1354#param-return-data-type

  4. +++ b/core/modules/system/system.api.phpundefined
    @@ -3946,6 +3946,29 @@ function hook_filetransfer_info_alter(&$filetransfer_info) {
    + *   Array of format_date function arguments and defined variables .
    + *   - settings
    + *   - date_time
    + *   - timezones
    + *   - timestamp
    + *   - type
    + *   - format
    + *   - timezone
    + *   - langcode
    

    This documentation is a bit unclear to me. We should probably have documentation for each key of the array explaining what it is and how it's used.

  5. +++ b/core/modules/system/system.api.phpundefined
    @@ -3946,6 +3946,29 @@ function hook_filetransfer_info_alter(&$filetransfer_info) {
    + * @see format_date()
    

    There should be a blank line above the @see.

I will see if I can find someone to help with the documentation. Thanks!

xjm’s picture

Regarding #1811912: Add pluggable calendar backend to core and centralize date translation, I think it still makes sense to put in this alter now since that isn't ready yet.

Gaelan’s picture

FileSize
2.18 KB
1.84 KB

Fixed docs and coding standards. Also removed $context['timezones'] and $context['settings'] because I can't think of any use case for them. If you can think of one, feel free to add them back.

Gaelan’s picture

Status: Needs work » Needs review

I am forgetful.

Gaelan’s picture

Status: Needs review » Needs work

Also, I think $context['timestamp'] and $context['timezone'] are duplicated by the DrupalDateTime object, so maybe they can be removed too. Again, if you can think up a use case for either, please tell me. I'm planning to go through $context and make sure there aren't any more duplicates.

sinasalek’s picture

Tx Gaelan, that's fine i put them there just to be sure.

tim.plunkett’s picture

Issue tags: +needs profiling

This strikes me as something that would be rather slow. Even if I'm wrong, let's back that up with data.

sinasalek’s picture

Issue tags: -needs profiling

This being fast or slow depends on drupal_alter function speed and the modules that implement it
But profiling is always nice :)

tim.plunkett’s picture

Issue tags: +needs profiling

Yes, and if the function is called so often that it causes a performance regression then we don't put an alter there.

wuinfo - Bill Wu’s picture

Any ideas of what the test case should be for this?

wuinfo - Bill Wu’s picture

Failed to apply patch @ #24 to latest code. New code was committed according to this issue: http://drupal.org/node/1571632

Checking patch core/includes/common.inc...
error: while searching for:

  // Call date_format().
  $settings = array('langcode' => $langcode);
  return $date_time->format($format, $settings);
}

/**

error: patch failed: core/includes/common.inc:1930
error: core/includes/common.inc: patch does not apply
Checking patch core/modules/system/system.api.php...
Hunk #1 succeeded at 3809 (offset -141 lines).
sinasalek’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -needs profiling

Status: Needs review » Needs work
Issue tags: +Needs tests, +needs profiling

The last submitted patch, drupal.1178342.24.hook_format_date_alter.patch, failed testing.

wuinfo - Bill Wu’s picture

Assigned: sinasalek » Unassigned
Status: Needs work » Needs review
FileSize
4.28 KB

Base on @sinasalek and @Gaelan's code, I added test code.

wuinfo - Bill Wu’s picture

upload a patch with the tests only followed by the full patch. That way we can see the tests failing and the patch fixing it.

hook_format_date_alter.1178342.36.patch is same as hook_format_date_alter.1178342.35.patch.

sinasalek’s picture

Nice test case @wuinfo, tx

Hadi Farnoud’s picture

Issue tags: -Needs tests
Gaelan’s picture

FileSize
1.87 KB
4.73 KB

This patch removes $context['timestamp']and $context['timezone'] because they are duplicated in the DrupalDateTime object.

Status: Needs review » Needs work

The last submitted patch, drupal.1178342.39.hook_format_date_alter.patch, failed testing.

Gaelan’s picture

Whoops… that contains a core hack that I am using as a workaround for #1856008: install.php doesn't work with symlinks. Here's an updated patch.

Status: Needs review » Needs work

The last submitted patch, drupal.1178342.39.hook_format_date_alter.patch, failed testing.

Gaelan’s picture

config(system.date).timezone got moved to config(system.timezone). That makes our tests fail because the timezone isn't what we expect. Here's an updated patch.

Gaelan’s picture

Status: Needs work » Needs review

I have not ceased to be forgetful.

Status: Needs review » Needs work

The last submitted patch, drupal.1178342.43.hook_format_date_alter.patch, failed testing.

wuinfo - Bill Wu’s picture

+++ b/core/includes/common.incundefined
@@ -1893,7 +1893,17 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
+  $context = array(
+    'date_time' => $date,
+    'type' => $type,
+    'format' => $format,
+    'langcode' => $langcode,
+  );

+++ b/core/modules/system/system.api.phpundefined
@@ -3797,6 +3797,29 @@ function hook_filetransfer_info_alter(&$filetransfer_info) {
+function hook_format_date_alter(&$formatted_date, array $context) {
+  $formatted_date .= ' ' . $context['timezone'];
+}

Function hook_format_date_alter(&$formatted_date, array $context) is still using $context['timezone'] while timezone was taken out from above code.

Gaelan’s picture

Status: Needs work » Needs review

#43: drupal.1178342.43.hook_format_date_alter.patch queued for re-testing.

@wuinfo: Good point. I'll fix that.

klonos’s picture

Title: Allow contributed modules to alter the format_date() function result » Allow contributed modules to alter the format_date() function result.

...how are things looking here? Are we going to have this in D8? Asking because trying to keep core setup to latest and at the same time keep Calendar Systems working is a really painful task of having to (remember to) re-apply the core patch/hack the contrib module provides with each core upgrade.

sinasalek’s picture

corrected according to #46

Status: Needs review » Needs work

The last submitted patch, hook_format_date_alter.patch, failed testing.

sinasalek’s picture

keeping up with head

sinasalek’s picture

Status: Needs work » Needs review
sinasalek’s picture

marcingy’s picture

Status: Needs review » Needs work

Please provide a patch in the correct format ie a diff

sinasalek’s picture

Status: Needs work » Needs review
FileSize
4.36 KB

Here it is

sinasalek’s picture

If anything else needs to be done please let me know

sinasalek’s picture

#55: hook_format_date_alter.patch queued for re-testing.

sinasalek’s picture

Cleaning up the patch

Status: Needs review » Needs work

The last submitted patch, hook_format_date_alter.patch, failed testing.

sinasalek’s picture

Patch created using git

sinasalek’s picture

Status: Needs work » Needs review
wuinfo - Bill Wu’s picture

+++ b/core/modules/system/tests/modules/system_test/system_test.moduleundefined
@@ -380,3 +380,11 @@ function system_test_authorize_init_page($page_title) {
\ No newline at end of file

There are some spaces in the patch needed to be removed and also missing newline at the end of file.

sinasalek’s picture

Mentioned problems fixed, here is the updated version

marcingy’s picture

Status: Needs review » Needs work

This needs profiling.

cweagans’s picture

marcingy, are you sure? I don't think it does. `format_date()` isn't called that many times during a page request. I'm inclined to RTBC this, but I'm interested in your rationale for wanting this profiled.

bdgreen’s picture

No significant difference - see attached ;)

Following the Apache Bench (ab) guidance (Benchmarking and profiling Drupal) there's no significant difference after applying the patch:

Before: 374+/-15.9ms

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   361  374  15.9    370     470
Waiting:      334  346  14.2    342     432
Total:        361  374  15.9    370     470

After patch: 374+/-14.3ms

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   360  374  14.3    370     454
Waiting:      333  345  12.5    342     414
Total:        360  374  14.3    370     454
tim.plunkett’s picture

What page was this on?
How many nodes were there?
Which content type, was the submitted by date showing?
Was there a datetime field in use?

Ideally the answers to those questions would be:
/node
10
article, yes
yes

marcingy’s picture

Request for profiling is based on http://drupal.org/node/1178342#comment-6781274 which had been nicely ignored until now ;)

cweagans’s picture

Assigned: Unassigned » cweagans

I'll have xhprof results in a few minutes. Thanks for pointing that out :)

cweagans’s picture

I used tim's params for this (10 articles nodes viewed on /node with submitted information showing + a date field being displayed on the teaser)

xhprof files attached.

cweagans’s picture

Assigned: cweagans » Unassigned
tim.plunkett’s picture

Was that a complete fluke or are the numbers backwards?

cweagans’s picture

I didn't believe it at first either, so I repeated the whole process 3 or 4 times and got similar results every time. I'm not sure how it can be both more function calls and faster, though.

bdgreen’s picture

FileSize
1.26 KB
1.26 KB

Hmm? I've just repeated the Apache Bench (ab) test again following @tim.plunkett recommendation in #67- there's still no significant difference ... :-S

A fresh install of D8, "best" of five runs of 500 requests with Date field displayed (".../node; 10; article, yes; yes") on requested page (/usr/sbin/ab2 -c1 -n500 http://drupal8sb.tld/node i.e. one request at a time). And, then repeated after patch installed and cache cleared.

Before 586+/-21.2sd ms:

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   561  586  21.2    580     718
Waiting:      534  557  19.9    551     681
Total:        561  586  21.2    580     718

After 588+/-22.5sd ms:

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   561  588  22.5    581     693
Waiting:      534  558  20.9    552     658
Total:        561  588  22.5    581     693
sinasalek’s picture

Thanks @bdgreen & @cweagans for the benchmarks,
@tim.plunkett is there anything else that needs to be done for this patch to land?

sinasalek’s picture

Status: Needs work » Reviewed & tested by the community

I guess there is nothing left to do

Gaelan’s picture

Status: Reviewed & tested by the community » Needs review

Someone that has not worked on the code must mark the issue RTBC.

bdgreen’s picture

Status: Needs review » Reviewed & tested by the community
sinasalek’s picture

Thanks @bdgreen,
@Gaelan anything else must be done?

Gaelan’s picture

A few small things:

+++ b/core/modules/system/lib/Drupal/system/Tests/Datetime/DrupalDateTimeTest.phpundefined
@@ -109,4 +109,29 @@ public function testDateTimezone() {
+   * Test hook_format_date_alter() function and format_date() function.

I think it would make a bit more sense if this read "Test the hook_format_date_alter() and format_date() functions."

+++ b/core/modules/system/system.api.phpundefined
@@ -3569,6 +3569,27 @@ function hook_filetransfer_info_alter(&$filetransfer_info) {
+ * A module may implement this hook in order to alter the formatted date string.

I think that this line is redundant. The summary line already describes what this hook does.

+++ b/core/modules/system/tests/modules/system_test/system_test.moduleundefined
@@ -284,3 +284,11 @@ function system_test_authorize_init_page($page_title) {
+  // Reverse the date string for testing purpose.

testing purposeS

sinasalek’s picture

Tx @Gaelan Keen eyes and thanks everyone for helping out,
Hope someone continues this patch for Drupal 9. It doesn't seem we can ever make it for Drupal 8
I just realized that applying it manually after each update is way much easier indeed :)

Hanno’s picture

@sinasalek we're almost there to implement this in D8! You did a very good job so far. This patch will solve a need for all the people in countries that are dealing with other calendars than the Georgian one, or even not try to use Drupal because it's useless for them. While you can patch core, most can't or shouldn't. When someone improve the comments a bit it's ready to commit.

Hadi Farnoud’s picture

@sinasalek I made the changes @Gaelan asked. This patch is essential to a true multi language Drupal. hopefully it gets committed to D8 soon.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, hook_format_date_alter_5.patch, failed testing.

Hadi Farnoud’s picture

can someone add RTBC tag?

Hadi Farnoud’s picture

Status: Needs work » Needs review
tstoeckler’s picture

+++ b/core/modules/system/system.api.php
@@ -3569,6 +3569,27 @@ function hook_filetransfer_info_alter(&$filetransfer_info) {
+ *   - type: The format to use as passed to format_date().

This should be the date type.

Overall I think it would be better to go for a container-based approach than this, but I won't hold this up.

Status: Needs review » Needs work

The last submitted patch, hook_format_date_alter_4.patch, failed testing.

Hadi Farnoud’s picture

Status: Needs work » Needs review

this patch does not need any more work. comments fixed according to #80. it has been reviewed and tested. can someone add RTBC tag?

tim.plunkett’s picture

#85: hook_format_date_alter_4.patch queued for re-testing.

cweagans’s picture

Status: Needs review » Reviewed & tested by the community

This is great :)

tim.plunkett’s picture

Beware, this will need a reroll once #2003934: Convert format_date to a service goes in.

tstoeckler’s picture

Since that issue is in, can we just mark this won't fix? calendar.module can just swap out the service and provide pluggable calendars, which we can then target for D9 core.

Hadi Farnoud’s picture

why would it be won't fix? it's been a long standing request since D7.

Hadi Farnoud’s picture

Issue tags: -needs profiling
Gaelan’s picture

@Hadi Farnoud: That issue already provides a solution for this.

cweagans’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

You can swap out the date service if you want to override this behavior in Drupal 8, however, this might be worth backporting to Drupal 7.

starchild’s picture

+1 for backporting to drupal 7.

klonos’s picture

Issue tags: +Needs backport to D7

..ok then.

sepehr.sadatifar’s picture

what should be done to continue process of backporting to D7?

sinasalek’s picture

Issue summary: View changes

The patch should be rewritten for D7

Dave Reid’s picture

I still think a simple alter hook should be added to the D8 code. I don't know how many times I've repeated this in D8 core issues: altering is a different use case than swapping services because the last module that wants to swap out the service wins and everyone else loses.

Anyway, here's an updated patch against Drupal 7. I'll work on a revised D8 version too.

sinasalek’s picture

Good point, i'll help getting this in :) i'll review the patch but it should against D8 first to be accepted by core commiters

herom’s picture

I would argue otherwise for D8. The usecase here is to allow using custom calendars, and that can be achieved by swapping the service. I can't think of a usecase where two different modules would want to alter the date format result at the same time; Once the first module changes the formatted date, the other ones would probably break.

Can you explain a use-case where the alter hook can be useful, but we can't do by swapping the service?

sinasalek’s picture

Status: Needs work » Needs review

Let test the patch

sinasalek’s picture

Working patch for Drupal 7 with test coverage attached

Dave Reid’s picture

@sinasalek: I think it's worth leaving $date_time to a separate parameter and not merging it into $context. Could you please revert that change?

@herom: My use case is to have a date format that if the date is less than 24 hours old, outputs with 'x hours ago' instead of the actual time. I wanted this to be available *anywhere* date formats are used. This patch is the only way to make it consistently possible. That cannot be achieved by swapping services, because I might have that use case in several different ways on a site (and on my current site I actually do). I will repeat again: altering is a different use case than swapping services.

sinasalek’s picture

It is pointless to have separate parameter for $date_time since at the stage the alteration is called it's no longer possible to alter the date_time variable, note that you can always access the date_time variable using the $context var.

sinasalek’s picture

@dave-reid , you can achieve your use case by the way using the current path. All you have to do is to data_time object in context and update the date format the way you need.

Patch is ready guys, we just need someone other than me to confirm that it's working

sinasalek’s picture

Status: Needs review » Needs work
sinasalek’s picture

Status: Needs work » Needs review
sinasalek’s picture

Status: Needs review » Needs work

The last submitted patch, 112: drupal-format_date_alter-1178342.patch, failed testing.

sinasalek’s picture

sinasalek’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 114: drupal-format_date_alter-1178343.patch, failed testing.

sepehr.sadatifar’s picture

Status: Needs review » Needs work

The last submitted patch, 117: drupal-format_date_alter-1178343-114.patch, failed testing.

Hadi Farnoud’s picture

things move so slowly in Drupal world. we gave up on it a few years ago.

too bad this issue is still not solved.

stefan.r’s picture

Issue tags: +Drupal 7.60 target

The last submitted patch, 117: drupal-format_date_alter-1178343-114.patch, failed testing.

sinasalek’s picture

Re-roll and tab space fix

sinasalek’s picture

Status: Needs work » Needs review
sinasalek’s picture

Correcting the patch extension

sinasalek’s picture

Status: Needs review » Reviewed & tested by the community

Just tested it on a clean install, applies cleanly and works fine.
I'm marking it as RTBC since we've been only re-rolling it for quite some time now.

Fabianx’s picture

Status: Reviewed & tested by the community » Needs work

Nice work! - Unfortunately it will need a little more work to be ready.

  1. +++ b/includes/common.inc
    @@ -2066,16 +2066,30 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
       // The read-ahead expression ensures that A matches, but not \A.
    +  $original_format = $format;
       $format = preg_replace(array('/\\\\\\\\/', '/(?<!\\\\)([AaeDlMTF])/'), array("\xEF\\\\\\\\\xFF", "\xEF\\\\\$1\$1\xFF"), $format);
    

    The $original_format line should probably be above the comment, else the comment wrongly applies to this line, which does not make sense.

  2. +++ b/includes/common.inc
    @@ -2066,16 +2066,30 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
    +    'format' => $original_format,
    

    Would it make sense to pass both the preg_replaced format and the original format here?

    It seems weird to pass the latter as 'format' key, but not pass the actual format.

  3. +++ b/modules/simpletest/tests/system_test.module
    @@ -569,3 +569,11 @@ function system_test_module_implements_alter(&$implementations, $hook) {
    +/**
    + * Implements hook_format_date_alter().
    + */
    +function system_test_format_date_alter(&$formatted_date, array $context) {
    +  // Reverse the date string for testing purposes.
    +  $formatted_date = strrev($formatted_date);
    +}
    

    system_test is used for many things.

    We only want to affect this one test.

    We typically do that by checking for a variable here:

    if (variable_get('system_test_reverse_date')) {
    // ...
    }

    Else all dates are reversed always, which is not what we want for other tests and which could lead to strange side effects in the future.

  4. +++ b/modules/system/system.test
    @@ -1459,6 +1459,30 @@ class DateTimeFunctionalTest extends DrupalWebTestCase {
    +    $this->drupalGet("node/$node1->nid");
    ...
    +    $this->drupalGet("node/$node2->nid");
    

    Are we sure we use this elsewhere in the code?

    e.g.

    I would expect:

    "node/" . $node1->nid

    or

    "node/{$node1->nid}"

    but not directly putting it in.

    Could we please check what other code / tests are using.

    I might be wrong here though.

  5. +++ b/modules/system/system.test
    @@ -1459,6 +1459,30 @@ class DateTimeFunctionalTest extends DrupalWebTestCase {
    +    module_disable(array('system_test'));
    

    This line is not necessary.

sinasalek’s picture

Status: Needs work » Needs review
FileSize
5.43 KB

Nice review thanks, i applied almost all your suggestions. here are some notes

  • In some cases having access to original_format is important since we're using this alter also for multi calendar support, now both the manipulated and original are passed
  • Removed module_disable, it hasn't been used anywhere else in system.test

Status: Needs review » Needs work

The last submitted patch, 127: drupal-format_date_alter-1178342-127.patch, failed testing.

sinasalek’s picture

Status: Needs work » Needs review
FileSize
5.87 KB

Correction

Fabianx’s picture

Almost there:

Please revert your changes to the other function and keep the "node/$node1->nid" intact as there is pre-existing code using this.

  1. +++ b/modules/system/system.test
    @@ -1282,7 +1282,7 @@ class DateTimeFunctionalTest extends DrupalWebTestCase {
    -
    +	
    

    nit - unrelated whitespace change

  2. +++ b/modules/system/system.test
    @@ -1290,9 +1290,9 @@ class DateTimeFunctionalTest extends DrupalWebTestCase {
    -    $this->drupalGet("node/$node1->nid");
    +    $this->drupalGet("node/" . $node1->nid);
    ...
    -    $this->drupalGet("node/$node2->nid");
    +    $this->drupalGet("node/" . $node2->nid);
    

    As this is pre-existing, there is no need to change it. Lets revert it.

    I had not been sure if this code existed elsewhere already ...

  3. +++ b/modules/system/system.test
    @@ -1459,6 +1459,30 @@ class DateTimeFunctionalTest extends DrupalWebTestCase {
    +    variable_set('system_test_reverse_date', 1);
    

    This could use a comment to enable the reversing of the date.

sinasalek’s picture

1. Removed
2. You are correct, i opened the file with PHPStorm and that code is highlighted which means that PHP recognizes it
3. I did a full code search before adding it, there wasn't any reference to system_test_reverse_date other than what this patch adds. I also did a bit of research and it seems that using variable_set/get is usual for the purpose you suggested. What do you mean by comment? how?

Fabianx’s picture

Two more nits and then we have it :).

Keep up the great work!

  1. +++ b/modules/system/system.api.php
    @@ -4802,6 +4802,26 @@ function hook_filetransfer_info_alter(&$filetransfer_info) {
    + *   - format: If type is 'custom', a PHP date format string suitable for input
    + *     to date().
    

    We miss the original_format in here.

    I did not see that in my last review.

  2. +++ b/modules/system/system.test
    @@ -1459,6 +1459,30 @@ class DateTimeFunctionalTest extends DrupalWebTestCase {
    +    variable_set('system_test_reverse_date', 1);
    

    Can we add a comment before that, that says:

    // Enable reversing of the date format.

sinasalek’s picture

Fabianx’s picture

Issue tags: +Pending Drupal 7 commit
+++ b/modules/system/system.api.php
@@ -4802,6 +4802,28 @@ function hook_filetransfer_info_alter(&$filetransfer_info) {
+ *   - original_format: contains the format parameter exactly as it was passed
+ *     to format_date function

nit - Contains (use uppercase); needs fullstop at end of sentence.

---

Marking for commit, though it might not happen before 7.52 is released and we target 7.60.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
sinasalek’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 136: drupal-format_date_alter-1178342-136.patch, failed testing.

sinasalek’s picture

Status: Needs work » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Thanks for all the work on this issue.

-  return preg_replace_callback('/\xEF([AaeDlMTF]?)(.*?)\xFF/', '_format_date_callback', $format);
+  $formatted_date = preg_replace_callback('/\xEF([AaeDlMTF]?)(.*?)\xFF/', '_format_date_callback', $formatted_date);

...

+  return filter_xss_admin($formatted_date);

The filter_xss_admin() change does not seem related to this issue, and is also probably the wrong thing to do (see discussion in #1892640: XSS in date formats admin page ). Why is it being added here?

Also, I am not sure I get where this issue ended up... If this is swappable in Drupal 8 (but does not have an alter hook) and that is why the issue was moved back from Drupal 8 to Drupal 7, then why are we adding an alter hook to Drupal 7 rather than making it swappable there too? (See drupal_http_request() for an example of how to make a procedural function swappable.) As Dave Reid pointed out in several of his comments above, swappable services and alter hooks are not the same thing... and I'm not sure if his concerns were really addressed.

So it seems to me like either this issue should go back to Drupal 8 to add an alter hook there first, or the Drupal 7 patch should be making the function swappable instead?

Fabianx’s picture

Issue tags: -Pending Drupal 7 commit
sinasalek’s picture

Thanks for the review,
1. filter_xss_admin hasn't been changed, only a variable is used to make alteration possible, please read all the changes in the patch and not only a single line.
2. To backport date service to Drupal 7, service injection should be back ported first! Is there an issue for that?. The patch was first for Drupal 8 but it has been rejected because of swap-able date service.

I agree with you that swapping services is not the same as alter hook and Dupal 8 should also have this hook, but we've been on the right track at the beginning and the patch has to be rewritten for Drupal 8 to catch up with core, the latest Drupal 8 patch is before swap-able service landed in.

I probably can update the patch for Drupal 8 but i need confirmation of the person that's actually going to commit this, We've already spent too much time on this tiny little patch to go wrong once more :D Hopefully you're that person ? :)

David_Rothstein’s picture

Thanks for your work on this patch.

I can make decisions for Drupal 7, but not Drupal 8 :) Adding the "Needs framework manager review" tag can sometimes help get direction on this kind of issue, so let's try that for starters. And let's move the issue to Drupal 8 if you think that makes sense? (From my personal point of view, either method is fine - add hook to both or add swappable method to Drupal 7, just mainly want to make sure they don't get out of sync with Drupal 7 having capabilities that Drupal 8 doesn't.)

1. filter_xss_admin hasn't been changed, only a variable is used to make alteration possible, please read all the changes in the patch and not only a single line.

I don't see an existing filter_xss_admin() in the patch, or in the current code at https://api.drupal.org/api/drupal/includes%21common.inc/function/format_.... Where does it happen in the current code?

2. To backport date service to Drupal 7, service injection should be back ported first! Is there an issue for that?.

I don't think that's necessary; see my comment above about how this can be done for a procedural function using the pattern found in drupal_http_request(). In particular, this part:

function drupal_http_request($url, array $options = array()) {
  // Allow an alternate HTTP client library to replace Drupal's default
  // implementation.
  $override_function = variable_get('drupal_http_request_function', FALSE);
  if (!empty($override_function) && function_exists($override_function)) {
    return $override_function($url, $options);
  }

  // Rest of the main drupal_http_request() function goes here.
}
sinasalek’s picture

can make decisions for Drupal 7, but not Drupal 8 :) Adding the "Needs framework manager review" tag can sometimes help get direction on this kind of issue, so let's try that for starters.

Thanks that would be a good idea.

I don't see an existing filter_xss_admin() in the patch, or in the current code at https://api.drupal.org/api/drupal/includes%21common.inc/function/format_.... Where does it happen in the current code?

I think i misunderstood your point sorry. You were right, filter_xss_admin should not be there.

I don't think that's necessary; see my comment above about how this can be done for a procedural function using the pattern found in drupal_http_request(). In particular, this part:

I see, but as we agreed, swapping service is a difference use case than alter hook. I think it's better to add the hook to Drupal 8.

sinasalek’s picture

David_Rothstein’s picture

Version: 7.x-dev » 8.3.x-dev

I think it's better to add the hook to Drupal 8.

OK, based on that and the fact that this issue basically needs feedback for Drupal 8, let's move this to Drupal 8 for now to see if that can get it moving.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

bgilhome’s picture

Here's a patch for 8.4.x-dev - let me know if it's suitable. I wasn't sure exactly where to put the hook example in core.api.php - I just added it last in the addtohooks section.

Status: Needs review » Needs work

The last submitted patch, 149: drupal-format_date_alter-1178342-149.patch, failed testing. View results

bgilhome’s picture

Whoops - need to set the right version to test against. Patch attached again.

bgilhome’s picture

Note that this is invoked when validating values in a datetime / range field - this should be skipped since altering the value may result in an invalid format. Should we pass something in $context in this case? e.g. $context['mode'] = 'form' / 'display'?

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

MustangGB’s picture

Issue tags: -Drupal 7.60 target +Drupal 7.70 target

Bumping the D7 target until D8 patch is committed.

deviantintegral’s picture

The last patch is failing because it requires the module handler service and it's not injected through the constructor. Best thing I can see without making a significant behaviour change would be to add a ModuleHandlerTrait so tests can at least set or mock the module handler as they see fit. Any other ideas?

deviantintegral’s picture

For now, here's a patch that won't fail tests by checking that a module handler is available before altering.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs change record

#151 added hook documentation, but #156 doesn't.

I'm also a bit confused why we need an alter hook as opposed to a module or site adding its own date formats. Is it for when someone is using a hard-coded custom format instead of a configured date format? But in that case, an issue against the contrib module might be better.

Tagging for issue summary update for the above.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.