Currently if I set a date format for the site, then it will be used with all interface translations. Different languages however have different needs when presenting dates. The Hungarian date format does not allow the day name to be the first and it calls for a dot between the hour and minute (yes, a dot not a colon). This means that Drupal ships with no suitable date format for Hungarians. Also none of the Hungarian date formats will be suitable on an English interface, while none of the English date formats are acceptable on a Hungarian interface on a bilingual site.

The only option is to make date formats translateable. Since these are dynamic t() calls, extractor.php needs to be extended to include the date formats shipped with drupal in the pot output, so translators will get them (much like we do for some numbers and day names). I will take care of extending extractor.php, once this patch gets applied.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Well, the t() needs to be called on the date format set by the admin, so I needed to fix three t() calls. Updated patch attached.

killes@www.drop.org’s picture

*g* I recall that there was a time when you didn't like this approach. Anyway, I fully endorse it and would like to see it in 4.5. There is also no suitable data format for German in core. And yes, that's a bug not a feature request.

Gábor Hojtsy’s picture

Yes, I didn't like it, because these are dynamic t()s. But since the selected format always needs to be presented in the actual language, it needs to be dynamic by nature. We have some of these dynamic t()s already, handled specially in extractor.php.

Steven’s picture

I like this idea a lot, but I don't like the simplistic "wrap it all in t" feature. There are instances where format_date output is intended for script output. Changing those date formats would mean a bug in the code or UI (for example the way a node date is specified and parsed).

Only those strings that are intended for user consumption should be translated. That's why I think we need a more specialized date handling component. It would deal with stuff like variations ('show hour/hide hour'), long/short variations (we need to review the usage of small/medium/long), as well as localizing them.

We would need to revise the formats shipped with Drupal, but this gets tricky. For example would we only ship US-style AM/PM date formats with core? Should we only have M/D/Y ordering, or also D/M/Y ordering? If we move the time format settings to locale (where they do below, in principle) we might get a drop in usability: moving from a simple selection box, to requiring the whole localisation framework 'just to switch from 12 hours to 24 hours'.
In some parts of the world, there is only one format for dates (e.g. chinese/japanese 2004年10月16日 1時00分). For them, the only variations are long vs short (whether to show the year, the time, etc), in other parts of the world all bets are off and people use what they like.

This would mean that if we provide a default set of formats, and let them be translated through PO files, some languages might require more items than provided and some languages would have to use duplicates to fill all available options. On top of that, there is no guarantee that format #3 in French matches format #3 in German, so you'd need to be able to specify this per language.

Ideally, we would let a language define any number of date formats, and let the user choose one of those per language. We could also provide a custom option where you enter your own date format, for the realy picky ones.

The question remains: what do we do for sites without locale enabled? We can't go to requiring locale just to switch the date on a non-localized site. In that case, perhaps we could do this:
- Have locale.module deal with date formats when it is enabled. Have a per-language format setting, selected from a language-specific, po-defined date format list.
- Have system.module implement a 'localization' admin menu item if locale.module is /not/ enabled. This page carries the old selector, which acts like a simplified version of locale's format selector, with the list we current have in Drupal (i.e. variations of 24/12hours, d/m/y, y/m/d, m/d/y, ...). To prevent confusion, we add a link to system.module's version of the localization page, which instructs users to enable locale.module for full localization abilities).

I know it sounds a bit weird (we certainly don't do this anywhere else), but it would be consistent in terms of UI (the date format is "localization" no matter how you look at it) and keep both non-localized site admins and localized site admins happy, without sacrificing too much ease of set up.

Gábor Hojtsy’s picture

Steven, you say:

There are instances where format_date output is intended for script output. Changing those date formats would mean a bug in the code or UI

But the current approach is to translate the month and day names in every case, so format_date *already* assumes that the date it outputs is for human consumption and not for machines. So changing the order of the fields in user date formats should not hurt. Sure it might not be a useability dream, but it does not hurt.

By the way, adjusting date formats based on user language is only a small piece of localisation not covered by Drupal core yet. I need to use one mission statement and site name in all languages, and even my primary and secondary links need to have the same text in all site languages, since they are not translateable. If we are about to find a generic solution for locale covered settings, we need to take these into account too.

Steven’s picture

Version: 4.5.0-rc »

"so format_date *already* assumes that the date it outputs is for human consumption and not for machines."

What I mean is changing the placement of the date components (day, month, year). For numerical dates which are parsed by a program, this will introduce bugs. I'd much rather have a cleaner date format selection method integrated with locale.

Gábor Hojtsy’s picture

Steven, the custom date formats are not translated, only those selected by the admin. And the admin can select date types with spelled out month/day names at any level, so they are not machine parsable anyway in the current Drupal version. *This patch does not introduce bugs.* I admit it does not look good, but there is no system for locale dependant admin settings yet.

Steven’s picture

I'm saying we do need such a system.

With your patch, the format that the admin selects in language A will not necessarily match the one in language B. We should go for locale-defined formats with a locale-neutral list to fallback on.

Pancho’s picture

Version: 6.x-dev »
Assigned: Unassigned » Pancho
Category: feature » bug
Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
14.91 KB

************************
DATEFORMAT-PATCH PACKAGE
************************

2005-03-13 03:07

This 'DateFormat' patch package only works with the latest CVS version of Drupal 4.60 RC.

This 'DateFormat' patch package applies the following changes:

1.
It introduces a more flexible system of standard date and time formats which helps both users and developers while staying

backwards-compatible. We often see special date formats be used in modules, which is only second best, as these can neither

be adapted by the user nor localized. More flexibility allows the developers to use the built-in format functions wherever

possible.

Changes in detail:
- The standard date formats used to be combined date&time-formats. This is now split into separate date formats and time

formats, to allow dates and times to be rendered in a more flexible way.
- Still it's backward compatible: the combined formats - while deprecated - still exist.
- A "Tiny" format is added to the "Small", "Medium" and "Large" formats.

2.
It enables Drupal to show e.g. German and Hungarian date formats (with a colon as separator), which was _not_ possible

before.

3.
It prepares both the Drupal system and module developers for the upcoming locale_dateformat module. This module will fully

localize date and time formats according to the localization of the current user. The locale_dateformat module will be

available within the next weeks.

I'm always happy about suggestions and criticism, if constructive. You can either write a comment on www.drupal.org or an email.

Bye, Pancho

-------------
INSTALLATION:
-------------

The package contains two patches for the latest CVS versions of Drupal 4.60 RC, both of which should be applied:

1. common.inc (patch for v.1.429)
2. system.module (patch for v.1.197)

There are no changes in database structure.

Edit: removed email address

Pancho’s picture

edit: deleted

Pancho’s picture

Title: Translate date formats » Localize date formats
Assigned: Gábor Hojtsy » Pancho
Category: bug » feature
Priority: Normal » Critical

I feel better if I refile the thread as a feature request. The only thing you could describe as a bug was that German & Hungarian date formats were not supported. The rest are definitely feature improvements.

Steven’s picture

Priority: Critical » Normal

- Localizing date formats seems like something that should be solved properly in core once and tied into locale.module. We need a solution which does not pose problems with mixing languages, and which does not require us to keep adding more and more date formats to the list (e.g. what about asian date formats? They don't have spaces.)
Your "if (!module_exist('locale_dateformat'))" solution is unacceptable for core.

- What's the point of the 'tiny' date format if it's not used anywhere? Where would it be used?

- Please don't post patches as zips, this is annoying. Just make one global patch file instead of separate ones. Multiple files can be posted with multiple follow ups.

- You need to see how this affects the date usage around Drupal, e.g. the profile date field formatting, which depends on the date format being in a specific format.

- Code style: check spaces around quotes. Also, avoid trailing spaces on lines, this is untidy.

- This issue is not "critical" IMO. It is a new feature request, and as such should probably wait until after 4.6, as 4.6 is in feature freeze.

- The apostrophes around the separator item seem a bit unnecessary.

Pancho’s picture

First of all: This patch works!
Even it may be not yet ready to be adopted in the core, it is good for individual use.


To Steven:

Okay, Steven, I was asking for criticism... :)

With some of the points you make I can agree:
- This feature should be completely and properly rewritten and not just fixed to have a good solution for a longer period and not only for now.
- The "if (!module_exist('locale_dateformat'))" solution is not good code style at least for core, as well as the spaces. I didn't take enough care about that, as I wanted to present a solution now, before the release of 4.6.
- Sorry about the zipped patches. I didn't expect it to bother anyone.

But on the other hand:
- It's not about adding more and more date formats. You will agree that the German web is a big enough market which should not be left aside anymore. Therefore the feature request is critical from my perspective at least. And the solution I presented doesn't blow up the date format lists, in the contrary it makes them easier to use.
- How can you complain about the tiny version not to be used. If it doesn't exist it can't be used.
- The apostrophees do make sense, because the separator can also be a single space. Nobody would identify that in the menu without apostrophees.

So you were critizising some points which I will fix in a new version of the patch as soon as possible. But you didn't say anything about the split of date and time formats. To me it makes sense, as there have already been more people asking for "just date" or "just time".

While I already started the development of a "locale_dateformat.module", I will put this on hold, until we figured out a good solution altogether. Anyway, I'm ready to help developing this feature within "locale.module" if I'm being asked. Otherwise developing a separate module would at least offer the so needed functionality within this century ;-)

Pancho

Steven’s picture

Firstly:

Even it may be not yet ready to be adopted in the core, it is good for individual use.

The patch queue is really part of the Drupal development process. It does not help us much to work on patches that are not intended for core.

It's not about adding more and more date formats. You will agree that the German web is a big enough market which should not be left aside anymore. Therefore the feature request is critical from my perspective at least. And the solution I presented doesn't blow up the date format lists, in the contrary it makes them easier to use.

I'm just saying, that solutions like the German date format (which has a special notation for the day numbers) or Asian formats (which have no spaces and reversed order) should really be handled through locale IMO, where date formats are picked per language, and can be added manually or through .po files.

How can you complain about the tiny version not to be used. If it doesn't exist it can't be used.

I imagine you introduced this format for a reason, no? Adding new features without knowing exactly where they should be used is silly. If you add a new feature in a patch, you should put in the work to see that this feature is actually used.

I didn't really talk about splitting date/time formats for now, as it is mostly a usability thing. I'd much rather wait for a solution integrated with locale before I comment on it.

Bèr Kessels’s picture

+1 for the idea. +1 for the implementation.
The idea is simple: just pull the format string trough the locales. No new interfaces (+1), no new configuration options (+1) and no additional cruft. po files will then take care of the real format.

However, I see one issue: we should remove the dopdowns in /admin/settings.

The patch still applies.

Uwe Hermann’s picture

Status: Needs review » Needs work

Doesn't apply anymore. Also, please provide _one_ patch (not one for each file you change).

Jaza’s picture

Version: » 6.x-dev
Assigned: Pancho » Jaza

Moving to 6.x-dev queue. This still seems to be a relevant issue.

Jaza’s picture

Assigned: Jaza » Unassigned

Unassigning this issue, as pancho doesn't seem to have been active on drupal.org for some time. The user can come back and reclaim this issue if they are still around and still interested.

Pancho’s picture

<off-topic>
I'm active again and must apologize (especially to Steven) for having been not very sensible nor cogent, back in 2005. I guess, being not really experienced with the Drupal project I didn't really get the difference between core development and just making things work. Be assured that I know better now.
</off-topic>

Anyway, this issue still bugs me (even though standard German date format has been added in the meanwhile). I'll try to come up with a good solution to reopen discussion, let's say within the next month.

Pancho’s picture

Version: » 7.x-dev
Assigned: Pancho » Unassigned
Category: bug » feature
Priority: Critical » Normal
Status: Needs review » Needs work

Moving feature requests to D7 queue. Sorry...

Sutharsan’s picture

Component: base system » usability

Moving all usability issues to Drupal - component usability.

dev13’s picture

This can be done currently with the internationalization (i18n) module and Multilingual Variables (http://drupal.org/node/313272). The variables involved are:

date_format_short
date_format_short_custom
date_format_medium
date_format_medium_custom
date_format_long
date_format_long_custom

I got this tip off http://openflows.com/blog/mvc/2008/10/03/drupal-6-i18n-basics (I have no affiliation with them).

I think this way is actually cleaner concept-wise than passing a setting (not a readable string) through language translation.

KarenS’s picture

Component: usability » base system

We have a nice new system for handling localized date formats in the Date module through the efforts of the Civic Actions crew (the issue is #318008: locale based handling and rendering and a writeup of the results is at http://drupal.org/node/383954).

It results in a system that does not require the i18n module, and it also provides flexible date formats for other uses besides internationalization. You can create as many pre-defined date formats as you like and use them anywhere in the system, so you can create a 'Date-only' format and a 'Time-only' format as well as the 'Long', 'Medium', and 'Short' formats that combine date and time. Then you can create 'French Canadian' and 'Mexican Spanish' and other locale-specific versions of all your time formats that will automatically be used in the right situation.

I'd like to see this system get into core. I don't have a patch, but it would basically involve porting that part of the Date API to D7. Just tagging this issue for now.

stella’s picture

I'd also like to see the new date format system get into Drupal core. I could work on this, but before I do, do people agree with the approach taken?

Cheers,
Stella

sun’s picture

Issue tags: +i18n sprint

Tagging. Would be nice to get this in.

stella’s picture

FileSize
35.2 KB

Very rough initial draft, just starts laying the ground work for the localization.

stella’s picture

FileSize
33.87 KB

Patch re-roll. Still need to add in localization stuff

stella’s picture

FileSize
47.2 KB
stella’s picture

Status: Needs work » Needs review
FileSize
47.2 KB

The patch now contains the localize date format functionality. It still needs tests and some usability improvements. Marking as needs review, but it's likely testing bot will fail it because I haven't updated the tests.

stella’s picture

FileSize
59.24 KB

Patch re-roll with usability improvements.

catch’s picture

I reviewed the original date patch that went into date.module including a lot of click through testing and from reading the code it looks like more or less a straight port to a patch, which is great.

Following is just nitpicks:

+++ includes/date_formats.inc	7 Sep 2009 00:14:22 -0000
@@ -0,0 +1,193 @@
+
+function system_default_date_formats() {

No phpdoc.

+++ includes/locale.inc	7 Sep 2009 00:14:25 -0000
@@ -2726,3 +2726,229 @@ function country_get_list() {
+  else {
+    drupal_add_js(drupal_get_path('module', 'system') . '/system.js');
+    drupal_add_js(array('dateTime' => array('lookup' => url('admin/config/regional/date-time/formats/lookup'))), 'setting');

This should be using #attached now.

+++ includes/locale.inc	7 Sep 2009 00:14:25 -0000
@@ -2726,3 +2726,229 @@ function country_get_list() {
+      '#prefix' =>'<p style="font-size: 1.2em;">',
+      '#suffix' =>'</p>',

If we really need this, it should be a class with associated CSS.

+++ modules/system/system.module	7 Sep 2009 00:14:35 -0000
@@ -820,20 +826,87 @@ function system_menu() {
+    'description' => "Configure display formats for date and time.",

Single quotes (and elsewhere).

I'm on crack. Are you, too?

KarenS’s picture

I hope it's not too late to get this in because it's an important internationalization issue. We started this long ago and tried to bring it home during the code sprint, but I was unable to work on this yesterday because I was traveling. The code functions absolutely correctly for me and it passes tests. I will continue to test and see if I can do a re-roll with catch's changes.

Any chance the maintainers will allow this in if we get it RTBC today?

catch’s picture

I'm happy to RTBC this once the nits are dealt with. Code freeze is 'today' - I'm not sure when the cut-off time is, but afaik any RTBC patch when the snapshot is taken is applicable as long as it's actually RTBC and not put there erroneously.

stella’s picture

FileSize
59.24 KB

Patch re-roll with fixes for catch's review. I also finished the usability improvements and added in the ability to edit date formats, and reset localized date formats.

To summarize what the patch does:

  • Removes the date format settings (i.e. long, medium, short) from admin/config/regional/settings and places them under admin/config/regional/date-time
  • Allows users to create their own date types. Core currently provides the date types "long", "medium", "short". With this patch users can define their own date types, e.g. "year and month", "long text", "my datefield", or whatever they wish to call them. These are stored in a new table date_format_types table.
  • Provides a hook_date_formats() which allows modules to (a) define new date types (b) define their own custom formats for them (or for existing ones) (c) allows them to specify suggested locales (e.g. en, en-us, fr).
  • Core implements the new hook_date_formats() for long, medium, short in a new includes/date_formats.inc file.
  • Users can define custom date formats which are stored in a new date_formats table and can be re-used by multiple date types.
  • If the locale module is enabled, users can override the global site date format settings for each language configured. If no localized date formats are configured, it defaults to the global settings.
  • Which date format to use is determined in an implementation of hook_init() which detects the user's language setting and loads the appropriate date formats.
stella’s picture

Regional settings page

Regional settings page

Add new date type

Add new date type

List of available date types

List of available date types

Add new date format

Add new date format

List of date formats

List of date formats

Language list

Language list

Localize date formats for French

Localize date formats for French

Localized date formats demo:

English node
French node
Uploaded with plasq's Skitch!
catch’s picture

Status: Needs review » Reviewed & tested by the community

It's lovely.

seutje’s picture

this is awesome, how did I never bump into this?

stella’s picture

FileSize
63.94 KB

Few minor tweaks.

KarenS’s picture

I also want to comment that this fits the pattern we are using with imagecache, where you can create presets and then use them in different ways, except that in this case you can also provide specialized versions of the presets for each language. This is very clever code, I can say that because I didn't write it, I just helped get it debugged and tested for Date. And it is in use in the D6 version of Date, so the concept is already in use and has undergone a fair bit of testing. Also, if this is in core, Views can also allow users to select from all these formats for their views of date fields, which would be really awesome.

So I'm hoping this makes it in :)

nedjo’s picture

I reviewed earlier versions of this patch. While this can be done in contrib through a bunch of overrides, it seems to belong in core.

Some minor remaining questions:

+++ includes/date_formats.inc	7 Sep 2009 11:26:47 -0000
@@ -0,0 +1,197 @@
+    'locales' => array('en-gb', 'en-hk', 'en-ie', 'el-gr', 'es-es', 'fr-be', 'fr-fr', 'fr-lu', 'it-it', 'nl-be', 'pt-pt',),
+  );

We're introducing language codes that aren't used elsewhere in core. Of course, assigning formats to two-digit language codes would be fairly pointless (what's needed in English in Canada is different from what's needed in the US or Britain). But should we leave this to contrib until we have regional language codes in core?

+++ modules/system/system.admin.inc	7 Sep 2009 11:27:11 -0000
@@ -1695,97 +1667,244 @@ function system_regional_settings() {
+/**
+ * Process system_regional_settings form submissions.
+ */
+function system_regional_settings_submit($form, &$form_state) {

Could/should this be done instead at the validate stage to avoid having to override system_settings_form_submit()?

+++ modules/system/system.admin.inc	7 Sep 2009 11:27:11 -0000
@@ -1695,97 +1667,244 @@ function system_regional_settings() {
+  hide($form['delete']);

Why are we hiding this? A comment would be good.

+++ modules/system/system.admin.inc	7 Sep 2009 11:27:11 -0000
@@ -2294,3 +2413,183 @@ function theme_system_themes_form($form)
+function system_configure_date_formats_form($edit, $dfid = 0) {
+  $js_settings = array(
+    'type' => 'setting',
+    'data' => array(
+      'dateTime' => array(
+        'date-format' => array(
+          'text' => t('Displayed as'),

The 'text' should be using Drupal.t() instead in the .js file, since that's how we provide localized strings.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

This is a great patch, and it is important enough that we might need to make an exception for it because I don't think it was marked RTBC before the code freeze deadline.

However, I don't want to commit this without having any tests. So, I'm marking this 'code needs work' so we can work on providing good test coverage for it. Given good tests in a timely fashion, this can make it in still.

stella’s picture

Assigned: Unassigned » stella

Currently working on simpletests.

To respond to Nedjo's comments:

+++ modules/system/system.admin.inc 7 Sep 2009 11:27:11 -0000
@@ -1695,97 +1667,244 @@ function system_regional_settings() {
+  hide($form['delete']);

Why are we hiding this? A comment would be good.

I've reworked the patch so this line is no longer needed.

The 'text' should be using Drupal.t() instead in the .js file, since that's how we provide localized strings.

When passing strings to inline javascript, I believe the current implementation is correct and we should be using t() so the translated string will be passed to the Javascript.

heather’s picture

Should this be integrated with a proposed UI text change?

#279570: Make date format choices less ambiguous

Add UI text to make date format choices less ambiguous => Drupal, user interface text, normal, needs review, 6 comments, 4 IRC mentions

I think that looks like an improvement for the reasons mentioned. Moreso now that the dates will be localised. It will be very clear.

stella’s picture

Status: Needs work » Needs review
FileSize
75.25 KB

Re-rolled patch with simpletests.

Status: Needs review » Needs work

The last submitted patch failed testing.

stella’s picture

Status: Needs work » Needs review
FileSize
75.08 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

stella’s picture

Status: Needs work » Needs review

I can't reproduce this test failure locally. Anyone?

Status: Needs review » Needs work

The last submitted patch failed testing.

stella’s picture

Status: Needs work » Needs review
FileSize
75.02 KB
stella’s picture

Status: Needs review » Reviewed & tested by the community

Resetting to RTBC now that tests pass.

stella’s picture

FileSize
74.79 KB

tidied up some tests

sun’s picture

All in all, this looks very nice!

In general though, I'd like to see a large PHPDoc API group (or similar) in system.api.php (or elsewhere), which explains the overall system. I.e., how/when is a developer supposed to define date types/formats in an own module (and when it would be wrong), how date formats are derived from date types, to which extent a site admin can alter formats, and how/when the localization of date formats happens.

+++ includes/date_formats.inc	15 Sep 2009 08:10:17 -0000
@@ -0,0 +1,197 @@
+/**
+ * @file
+ * Initialize the list of date formats and their locales.
+ */

Does this have to live in a separate include file? Why not in locale.inc or iso.inc?

+++ includes/date_formats.inc	15 Sep 2009 08:10:17 -0000
@@ -0,0 +1,197 @@
+function system_default_date_formats() {
...
+  $formats[] = array(
+    'type' => 'short',
+    'format' => 'd/m/Y - H:i',
+    'locales' => array('en-gb', 'en-hk', 'en-ie', 'el-gr', 'es-es', 'fr-be', 'fr-fr', 'fr-lu', 'it-it', 'nl-be', 'pt-pt',),
+  );
+  $formats[] = array(
+    'type' => 'short',
+    'format' => 'Y/m/d - H:i',
+    'locales' => array('en-ca', 'fr-ca', 'no-no', 'sv-se',),
+  );
+  $formats[] = array(
+    'type' => 'short',
+    'format' => 'd.m.Y - H:i',
+    'locales' => array('de-ch', 'de-de', 'de-lu', 'fi-fi', 'fr-ch', 'is-is', 'pl-pl', 'ro-ro', 'ru-ru',),
+  );

nedjo asked this before already - do we want to keep those regional locales in core?

I'd be +1 in general, because it means less confusion/configuration for the site administrator. But OTOH, those definitions do not look complete...? (Maybe I'm mistaken though)

Minor note: There are trailing commas in those single-line 'locales' arrays.

+++ includes/locale.inc	15 Sep 2009 08:10:20 -0000
@@ -2726,3 +2726,217 @@ function country_get_list() {
+function locale_date_format_form_submit($form, &$form_state) {
...
+    locale_date_format_save($langcode, $type, $format);

Shouldn't we move Locale API functionality into locale.inc and only keep Locale UI (mostly forms, blocks) in locale.module?

I know that locale.inc contains much UI functionality already, which really doesn't belong in there, and I know that there is at least one other issue in the queue that wants to fix the existing mess...

Thus, it's not really required for this patch, but it would be nice to separate the functionality cleanly, so a "Better Locale" module in contrib could easily swap out the interface, but still use the API.

+++ modules/locale/locale.module	15 Sep 2009 08:10:21 -0000
@@ -184,10 +184,67 @@ function locale_menu() {
+function locale_init() {
...
+  if (!drupal_match_path($_GET['q'], 'admin/config/regional/date-time/formats')) {

This should be a simple !strpos($_GET['q'], 'admin/config/regional/date-time/formats') === 0, since drupal_match_path() is supposed to match a path in a list of paths (like in visibility options in the Block UI)

+++ modules/locale/locale.module	15 Sep 2009 08:10:21 -0000
@@ -184,10 +184,67 @@ function locale_menu() {
+  include_once DRUPAL_ROOT . '/includes/locale.inc';
+
+  global $conf;
+  global $language;

Is there a reason why the globals are defined not at the beginning?

We can also use one global statement and multiple variables separated by comma here.

+++ modules/locale/locale.module	15 Sep 2009 08:10:21 -0000
@@ -184,10 +184,67 @@ function locale_menu() {
+  // Don't do this on the general date and time formats settings page, as we
+  // want to display the defaults, not the ones specific to the language we're
+  // currently browsing the site in.

It would be nice if this comment wouldn't refer to "Don't do THIS", but instead provide some basic clue about what THIS is about to happen. :)

+++ modules/locale/locale.test	15 Sep 2009 08:10:25 -0000
@@ -1601,3 +1601,73 @@ class UILanguageNegotiationTest extends 
+    // Create node content.
+    $node = $this->drupalCreateNode(array('type' => 'article'));
+
+    // Configure format for the node posted date changes with the language.
+    $this->drupalGet('node/' . $node->nid);
+    $english_date = format_date(REQUEST_TIME, 'custom', 'j M Y');
+    $this->assertText($english_date, t('English date format appears'));
+    $this->drupalGet('fr/node/' . $node->nid);
+    $french_date = format_date(REQUEST_TIME, 'custom', 'd.m.Y');
+    $this->assertText($french_date, t('French date format appears'));

Shouldn't these assertions use $node->created instead of REQUEST_TIME?

+++ modules/system/system.admin.inc	15 Sep 2009 08:10:26 -0000
@@ -1696,97 +1668,243 @@ function system_regional_settings() {
+function system_regional_settings_submit($form, &$form_state) {
+  if ($form_state['values']['date_format_short'] == 'custom') {
+    $form_state['values']['date_format_short'] = $form_state['values']['date_format_short_custom'];
+  }
+  if ($form_state['values']['date_format_medium'] == 'custom') {
+    $form_state['values']['date_format_medium'] = $form_state['values']['date_format_medium_custom'];
+  }
+  if ($form_state['values']['date_format_long'] == 'custom') {
+    $form_state['values']['date_format_long'] = $form_state['values']['date_format_long_custom'];
+  }
+  return system_settings_form_submit($form, $form_state);
+}

What's happening here?

I now realize this code has been moved only - squeezing a comment in there would still be neat. :)

+++ modules/system/system.date.inc	15 Sep 2009 08:10:26 -0000
@@ -0,0 +1,238 @@
+function _system_date_format_types_build() {
...
+function _system_date_formats_build() {
...
+function system_date_format_type_save($date_format_type) {
...
+function system_date_format_type_delete($date_format_type) {
...
+function system_date_format_save($date_format, $dfid = 0) {
...
+function system_date_format_delete($dfid) {

Those API functions really shouldn't live in an include file. They should be callable directly and therefore either live in system.module.

If something could be moved into that include file, then it may be the forms - but then again, all administrative configuration forms for System module already live in system.admin.inc.

+++ modules/system/system.date.inc	15 Sep 2009 08:10:26 -0000
@@ -0,0 +1,238 @@
+function _system_date_formats_build() {
+  $date_formats = array();
...
+  // Get formats supplied by various contrib modules.
+  $module_formats = module_invoke_all('date_formats');
+
+  foreach ($module_formats as $module_format) {
...
+      $format['module'] = '';
...
+  // Allow other modules to modify these formats.
+  drupal_alter('date_formats', $date_formats);

Shouldn't this be statically cached?

Also, shouldn't this record + return the implementing 'module' by using module_implements()?

+++ modules/system/system.date.inc	15 Sep 2009 08:10:26 -0000
@@ -0,0 +1,238 @@
+    // If this format not already present, add it to the array.
+    if (!isset($date_formats[$record->type][$record->format])) {
+      // We don't set 'is_new' as it is already in the db.
+      $format = array();
+      $format['module'] = '';
+      $format['dfid'] = $record->dfid;
+      $format['format'] = $record->format;
+      $format['type'] = $record->type;
+      $format['locked'] = $record->locked;
+      $format['locales'] = array($record->language);
+      $date_formats[$record->type][$record->format] = $format;
+    }
+    // Format already present, so merge in settings.
+    else {
+      $format = array();
+      $format['is_new'] = FALSE; // It's in the db, so override this setting.
+      $format['dfid'] = $record->dfid;
+      $format['format'] = $record->format;
+      $format['type'] = $record->type;
+      $format['locked'] = $record->locked;
+      if (!empty($record->language)) {
+        $format['locales'] = array_merge($date_formats[$record->type][$record->format]['locales'], array($record->language));
+      }
+      $date_formats[$record->type][$record->format] = array_merge($date_formats[$record->type][$record->format], $format);
+    }

Why don't we simply cast the database record to an array and merge that into $date_formats?

Furthermore, it seems like 'is_new' should be set to FALSE in both cases here --- actually, most of $format seems to be identical for both cases; the only difference is that you additionally merge existing properties in one of both cases.

+++ modules/system/system.date.inc	15 Sep 2009 08:10:26 -0000
@@ -0,0 +1,238 @@
+  // Update date_format table.
+  if (isset($date_format_type['is_new']) && !empty($date_format_type['is_new'])) {
...
+  // Update date_format table.
+  if (isset($date_format['is_new']) && !empty($date_format['is_new'])) {

!empty() is sufficient here.

+++ modules/system/system.install	15 Sep 2009 08:10:28 -0000
@@ -691,6 +691,90 @@ function system_schema() {
+  $schema['date_format_types'] = array(
...
+      'type' => array(
+        'description' => 'The date format type, e.g. medium.',
+        'type' => 'varchar',
+        'length' => 200,
...
+  $schema['date_formats'] = array(
...
+      'type' => array(
+        'description' => 'The date format type, e.g. medium.',
+        'type' => 'varchar',
+        'length' => 200,
...
+  $schema['date_format_locale'] = array(
...
+      'type' => array(
+        'description' => 'The date format type, e.g. medium.',
+        'type' => 'varchar',
+        'length' => 200,

200 looks a bit large to me. I guess that 64 or even 32 should be sufficient.

Additionally, we now use singular table names in D7, since the table name refers to an entity. I.e. 'date_format_type', 'date_format', 'date_format_locale' (whereby the last one isn't really self-describing, but I'm lacking a better idea currently).

+++ modules/system/system.install	15 Sep 2009 08:10:28 -0000
@@ -691,6 +691,90 @@ function system_schema() {
+    'description' => 'For storing configured date format types.',
...
+    'description' => 'For storing configured date formats.',
...
+    'description' => 'For storing configured date formats for each locale.',
@@ -2472,6 +2556,103 @@ function system_update_7037() {
+function system_update_7038() {
...
+    'description' => 'For storing configured date format types.',
...
+    'description' => 'For storing configured date formats.',
...
+    'description' => 'For storing configured date formats for each locale.',

s/For storing/Stores/

That said, shouldn't the third table be in Locale module's schema? Not sure though.

+++ modules/system/system.js	15 Sep 2009 08:10:28 -0000
@@ -96,23 +96,21 @@ Drupal.behaviors.copyFieldValue = {
 Drupal.behaviors.dateTime = {
-  attach: function (context, settings) {
...
+  attach: function (context) {
+    for (var value in Drupal.settings.dateTime) {
+      var settings = Drupal.settings.dateTime[value];

settings are passed as second argument to each Drupal behavior. The passed settings may have been extended by an AJAX callback, and Drupal behaviors should always use the passed settings, not the global Drupal.settings.

+++ modules/system/system.module	15 Sep 2009 08:10:33 -0000
@@ -784,20 +790,96 @@ function system_menu() {
   $items['admin/config/regional/settings'] = array(
     'title' => 'Regional settings',
-    'description' => "Settings for how Drupal displays date and time, as well as the system's default time zone.",
+    'description' => "Settings for the site's default time zone and country settings.",

2x "settings" in the description, just remove it at the end. :)

+++ modules/system/system.module	15 Sep 2009 08:10:33 -0000
@@ -2391,6 +2473,9 @@ function system_cron() {
+  // Rebuild list of date formats.
+  system_date_formats_rebuild();

Why does this happen during cron runs? Wouldn't drupal_flush_all_caches() resp. system_flush_caches() be more appropriate?

+++ modules/system/system.module	15 Sep 2009 08:10:33 -0000
@@ -3158,3 +3243,141 @@ function theme_system_run_cron_image($im
+function system_get_date_format($dfid) {
+  $format = db_query('SELECT df.dfid, df.format, df.type, df.locked FROM {date_formats} df WHERE df.dfid = :dfid', array(':dfid' => $dfid))->fetch();
+  return $format;
+}

We can straight return the database query result here.

+++ modules/system/system.module	15 Sep 2009 08:10:33 -0000
@@ -3158,3 +3243,141 @@ function theme_system_run_cron_image($im
+/**
+ * Resets the database cache of date formats, and saves all new date formats to
+ * the database.
+ */
+function system_date_formats_rebuild() {

PHPDoc summary should be on one line ;)

+++ modules/system/system.module	15 Sep 2009 08:10:33 -0000
@@ -3158,3 +3243,141 @@ function theme_system_run_cron_image($im
+function system_date_format_locale($langcode = NULL, $type = NULL, $reset = FALSE) {
+  static $formats;

We should use a drupal_static() here and remove the $reset argument.

This review is powered by Dreditor.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Let's try to incorporate sun's comments.

KarenS’s picture

Issue tags: +Exception code freeze

Based on Dries' comment in #41, this qualifies as an exception.

KarenS’s picture

Re #53, the table must be named date_formats rather than date_format, see #395156: MySQL 5.0.67: Table 'drupal6.date_format' not created where we found that some versions of MYSQL won't correctly create a table name 'date_format'. We had to rename the table when we found that out. Perhaps we should document that in the code.

hass’s picture

Yeah, a singular table name doesn't work here :-(.

stella’s picture

Status: Needs work » Needs review
FileSize
75.27 KB

Patch re-roll which should address the majority of the items raised by sun above.

Just a couple of points:

  • Should _system_date_formats_build be statically cached?
    No, it's called from system_get_date_formats() only when its static cache needs to be rebuilt
  • Which include file to use for hook_date_formats() function: date_formats.inc vs locale.inc vs iso.inc
    It's not an ISO standard so I don't think it should go in the iso.inc file, if it wasn't for that I would have added them there. The only ISO standard I could find was ISO 8601 which specifies international standards, not country specific ones. I didn't add it to locale.inc because it didn't seem to fit in with the other functions in the file (mainly UI stuff). The locales specified in the file were largely taken from http://en.wikipedia.org/wiki/Calendar_date

Cheers,
Stella

Status: Needs review » Needs work

The last submitted patch failed testing.

mattyoung’s picture

.

stella’s picture

Status: Needs work » Needs review
FileSize
75.5 KB

Patch re-roll

sun’s picture

Issue tags: +D7 API clean-up

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

Status: Needs review » Needs work

The last submitted patch failed testing.

stella’s picture

Status: Needs work » Needs review
FileSize
75.54 KB

Patch re-roll

sun’s picture

This looks much better now. It seems like most of my comments have been addressed.

+++ modules/system/system.api.php	10 Oct 2009 13:09:58 -0000
@@ -2362,5 +2362,60 @@ function hook_date_formats(&$action
 /**
+ * Defines additional date types and date formats.
+ *
+ * As well as the 'long', 'medium' and 'short' date types defined in core, any
+ * module can define additional types that can be used when displaying dates.

I'm still missing an introduction to what a date format is, and, what a date type is. Please keep in mind that developers will come from the very limiting concept of just knowing about 'short', 'medium', 'long', and 'custom', so the first thing they want to know is: WTF is that?

It confuses me even more that this hook is called hook_date_formats() and the very first sentence talks about date *types* and now that I read it multiple times, I realize that even the summary contains both terms... what is the difference between a date type and a date format? All of that we need to explain :-)

Also:

- s/As well as/Next to/

+++ modules/system/system.api.php	10 Oct 2009 13:09:58 -0000
@@ -2362,5 +2362,60 @@ function hook_date_formats(&$action
+ * For each type it's possible to specify one or more date format strings, along
+ * with a list of suggested languages and locales.
...
+    array(
+      'type' => 'short',
+      'format' => 'F Y',
+      'locales' => array(),
+    ),

uhm, ok,

1) It seems like one item in this array forms a date type. But it also seems like a date type has only one format string (based on the code), so how does multiple work?

2) It talks about languages and locales, but I only see a 'locales' key...?

3) I wonder why the value in 'type' isn't the key?

+++ modules/system/system.api.php	10 Oct 2009 13:09:58 -0000
@@ -2362,5 +2362,60 @@ function hook_date_formats(&$action
+ *   An array of date formats.  Each date format is an associative array

"An array"? It seems to be an indexed array, right? Elsewhere, we call this simply "a list".

+++ modules/system/system.api.php	10 Oct 2009 13:09:58 -0000
@@ -2362,5 +2362,60 @@ function hook_date_formats(&$action
+ *   - 'type': the date type is a key used to identify which date format to
+ *     display.  It consists of letters, numbers and underscores, e.g. 'long',
+ *     'extra_long', 'short'.

So what happens when two modules define a date type with the same name?

Shouldn't these be prefixed with the module name or something?

+++ modules/system/system.api.php	10 Oct 2009 13:09:58 -0000
@@ -2362,5 +2362,60 @@ function hook_date_formats(&$action
+ *   - 'locales': an array of 2 and 5 character language codes, for example,
+ *     'en', 'en-us'.  The language codes are used to determine which date
+ *     format to display for the user's current language unless overridden via
+ *     admin/config/regional/date-time/locale.

Looking at system_default_date_formats(), there clearly seems to be some kind of fallback handling involved, because I see the same date format being registered multiple times, but for different locales, and a single date format in each of those sets that has no locales defined.

If there is some fallback logic or language negotiation happening, then we need to know. :)

+++ modules/system/system.install	10 Oct 2009 13:10:01 -0000
+function system_update_7040() {
+  $ret = array();
+
...
+  db_create_table($ret, 'date_format_type', $schema['date_format_type']);
+  db_create_table($ret, 'date_formats', $schema['date_formats']);
+  db_create_table($ret, 'date_format_locale', $schema['date_format_locale']);
+
+  return $ret;

$ret is gone :)

+++ modules/system/system.module	10 Oct 2009 13:10:04 -0000
+function system_flush_caches() {
+  // Rebuild list of date formats.
+  system_date_formats_rebuild();
+  return array();

You don't need to return something here, just remove the return line.

+++ modules/system/system.module	10 Oct 2009 13:10:04 -0000
+/**
+ * Implements hook_date_format_types().
+ */
+function system_date_format_types() {
+  return array(
+    'long' => t('Long'),
+    'medium' => t('Medium'),
+    'short' => t('Short'),
+  );
+}

Hm... this hook isn't documented, so I'm starting to get even more confused. :-/

+++ modules/system/system.module	10 Oct 2009 13:10:04 -0000
+function _system_date_formats_build() {
...
+    $format = (array) $record;
+    $format['is_new'] = FALSE; // It's in the db, so override this setting.
+    // If this format not already present, add it to the array.
+    if (!isset($date_formats[$record->type][$record->format])) {
+      // We don't set 'is_new' as it is already in the db.
+      $format['module'] = '';
+      $format['locales'] = array($record->language);
+      $date_formats[$record->type][$record->format] = $format;
+    }

We now have a stale comment about 'is_new' here, it seems.

This review is powered by Dreditor.

stella’s picture

FileSize
76.56 KB

Ok I've updated the patch and have hopefully addressed all the points. The api docs for hook_date_formats() includes example code for defining more than one format per date type.

Cheers,
Stella

sun’s picture

+++ modules/system/system.api.php	11 Oct 2009 00:06:34 -0000
@@ -2362,5 +2362,92 @@ function hook_action_info_alter(&$action
+ * module can define additional types that can be used when displaying dates.  A
+ * date type is a key which can be passed to format_date() to return a date in

(and elsewhere) Albeit, grammatically correct in some slangs, we do not use double-spaces between sentences in PHPDoc. See http://drupal.org/node/1354

+++ modules/system/system.api.php	11 Oct 2009 00:06:34 -0000
@@ -2362,5 +2362,92 @@ function hook_action_info_alter(&$action
+ * To avoid namespace collisions with date types defined by other modules, it is
+ * recommended that each date type starts with the module name.  A date type
+ * can consist of letters, numbers and underscores.

Now that we clarified this, I wonder how date types/formats are supposed to be used. Because, of course, additional date types are only available if the module that provides them is installed. Hence, any other module that wants to implement them (format_date($time, 'mymodule_foo')) needs to add a dependency on the providing module.

Is all of this only intended for custom modules of site builders? If it is, that's fine, but let's clarify! :)

Or will we see a Date module in contrib that implements countless of various date types/formats for mass-consumption?

+++ modules/system/system.api.php	11 Oct 2009 00:06:34 -0000
@@ -2362,5 +2362,92 @@ function hook_action_info_alter(&$action
+ * @see hook_date_formats().
+ * @see format_date().

(and elsewhere) Please remove the trailing periods.

@see #178629: Properly use @see

+++ modules/system/system.api.php	11 Oct 2009 00:06:34 -0000
@@ -2362,5 +2362,92 @@ function hook_action_info_alter(&$action
+/**
+ * Defines additional date formats.
+ *
...
+ * Custom date types are first declared using hook_date_format_types().  It is
+ * then possible to define one or more date formats for each.

That makes it a bit more clear, but I still do not fully grok the relation between date types and formats. For example, do I have to use the same 'type' strings in hook_date_formats() as in hook_date_format_types()? And what happens if I define a format for a non-existing type?

Also, why would I want to define multiple formats for a single type?

I guess the entire point of this is the 'locales' key and thereby the localization... However, the PHPDoc of hook_date_formats() does not mention this overall purpose, which is, however, the very first thing I'd want to know. :)

+++ modules/system/system.module	11 Oct 2009 00:06:39 -0000
@@ -2901,3 +2991,353 @@ function theme_system_run_cron_image($va
+function _system_date_format_types_build() {
...
+    else {
+        $type = array();
+        $type['is_new'] = FALSE;  // Over-riding previous setting.
+        $types[$record->type] = array_merge($types[$record->type], $type);
+    }

Wrong indentation here.

I'm on crack. Are you, too?

stella’s picture

FileSize
76.88 KB
sun’s picture

Final pass. :)

+++ includes/date_formats.inc	11 Oct 2009 21:57:52 -0000
@@ -0,0 +1,197 @@
+function system_default_date_formats() {
...
+  $formats[] = array(
+    'type' => 'short',
+    'format' => 'd/m/Y - g:ia',
+    'locales' => array(),
+  );
+  $formats[] = array(
+    'type' => 'short',
+    'format' => 'Y/m/d - g:ia',
+    'locales' => array(),
+  );
+  $formats[] = array(
+    'type' => 'short',
+    'format' => 'M j Y - H:i',
+    'locales' => array(),
+  );
+++ modules/system/system.api.php	11 Oct 2009 21:58:04 -0000
@@ -2362,5 +2362,99 @@ function hook_action_info_alter(&$action
+/**
+ * Defines additional date formats.
+ *
...
+ * New date types must first be declared using hook_date_format_types(). It is
+ * then possible to define one or more date formats for each.
+ *
...
+ *   - 'locales': (optional) an array of 2 and 5 character language codes, for
+ *     example, 'en', 'en-us'. The language codes are used to determine which
+ *     date format to display for the user's current language. If more than one
+ *     date format is suggested for the same date type and locale, then the
+ *     first one will be used unless overridden via
+ *     admin/config/regional/date-time/locale.
+ */
+function hook_date_formats() {

I now understand the purpose of assigning multiple date formats with different locales.

What I do not understand is the purpose of adding multiple date formats that do not define any locale at all, like the system's default implementation does.

I don't really have a clue. Though what I understand is that from these, also the first will be used unless the administrator configured it differently.

Sorry for all of this stressing on docs. But I really feel that we're adding a major WTF for developers/site builders here, if we do not document it properly.

+++ modules/system/system.admin.inc	11 Oct 2009 21:58:01 -0000
@@ -1700,97 +1672,227 @@ function system_regional_settings() {
+function system_date_time_settings() {
...
+  $form = system_settings_form($form, FALSE);
+  // We will call system_settings_form_submit() manually, so remove it for now.
+  unset($form['#submit']);
+  unset($form['#theme']);
+  return $form;

woohoo... you're unsetting *all* submit handlers here, not just the one for system_settings_form() ;)

What you want is:

unset($form['#submit'][array_search('system_settings_form_submit', $form['#submit'])]);

The #theme, I'd also rather assign explicitly to the custom function instead of unsetting it (which makes it a bit clearer what's going on here).

+++ modules/system/system.admin.inc	11 Oct 2009 21:58:01 -0000
@@ -1700,97 +1672,227 @@ function system_regional_settings() {
+function system_date_time_settings_submit($form, &$form_state) {
+  return system_settings_form_submit($form, $form_state);
+}

uhm? What is the point of removing it and then calling it manually without doing something else?

Either remove it, or squeeze some PHPDoc in there explaining why this is required.

+++ modules/system/system.admin.inc	11 Oct 2009 21:58:01 -0000
@@ -1700,97 +1672,227 @@ function system_regional_settings() {
+ * Theme function for date settings form.
+ *
+ * @variables
+ *   An associative array containing the structure of the form.
...
+function theme_system_date_time_settings($variables) {
@param variables
  An associative array containing:
  - form: An associative array containing the structure of the form.

However, since this is a standard form theme function, you can as well remove the @param declaration.

+++ modules/system/system.admin.inc	11 Oct 2009 21:58:01 -0000
@@ -2342,6 +2444,185 @@ function theme_system_themes_form($varia
+function system_date_time_formats() {
...
+    '#rows' => $rows

Very minor: missing trailing comma.

+++ modules/system/system.api.php	11 Oct 2009 21:58:04 -0000
@@ -2362,5 +2362,99 @@ function hook_action_info_alter(&$action
+function hook_date_formats_alter(&$formats) {
+  foreach ($formats as $id => $format) {
+    array_push($formats[$id]['locales'], 'en-ca');

Minor: uhm, why not simply

$formats[$id]['locales'][] = 'en-ca';

?

Won't hold up this patch.

+++ modules/system/system.module	11 Oct 2009 21:58:12 -0000
@@ -792,20 +798,96 @@ function system_menu() {
+  $items['admin/config/regional/date-time/types'] = array(
...
+  $items['admin/config/regional/date-time/types/add'] = array(
...
+  $items['admin/config/regional/date-time/types/delete/%'] = array(
...
+  $items['admin/config/regional/date-time/formats'] = array(
...
+  $items['admin/config/regional/date-time/formats/add'] = array(
...
+  $items['admin/config/regional/date-time/formats/edit/%'] = array(
...
+  $items['admin/config/regional/date-time/formats/delete/%'] = array(

We should move the dynamic argument before the action (edit/delete) here, which is possible, because the dynamic argument is an integer.

So .../formats/edit/% becomes .../formats/%/edit

This review is powered by Dreditor.

stella’s picture

FileSize
76.88 KB
....
unset($form['#submit']);
unset($form['#theme']);
....
+function system_date_time_settings_submit($form, &$form_state) {
+  return system_settings_form_submit($form, $form_state);
+}

I think this was obsolete left over code from the old system after I removed the old method of configuring custom date formats. I've removed this now.

What I do not understand is the purpose of adding multiple date formats that do not define any locale at all, like the system's default implementation does.

There may be more than one format for the same locale. For example d/m/Y and Y/m/d work equally well in Ireland. In core in particular where there's only scope to define short, medium or long date formats, some users will want to have "d/m/Y" for short, whereas other site builders will want the time included too. Once you get into time, you then leave it open to am / pm, DST and timezone too. So even though it may be d/m/Y in them all, you still need to allow a bit more flexibility.

However at the same time you may wish to define some additional date formats, but aren't really locale specific. For example, you may wish to create a "Y m" date format - but there's no locale specific date format that I know of that doesn't include the day too. There will be use cases like this where it is appropriate to leave locales empty. The 'locales' field is only a suggestion in any case, which will be used unless the site builder overrides it.

Where this change will be really useful is in conjunction with Date CCK fields and Views. You'll no longer be restricted to just short / medium / long, but you can define your own formats and types, and still localize them too.

stella’s picture

FileSize
77.07 KB

Oops, attached the wrong version of the patch.

stella’s picture

FileSize
77.07 KB

Ok and another re-roll with only one space after periods - I really don't agree with this by the way. I've always been taught to use two spaces and it's REALLY hard to break the habit :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

I hope the tests will pass on this one.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

stella’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
77.07 KB

Forgot to update the tests for the change in the paths. Fixed now.

sun’s picture

Testbot hangs on re-test.

Dries’s picture

I tested this patch and like what I see.

The only thing that slightly confused me is the relation between the Types-tab and the Localize-tab when locale module is enabled. Is the Types-tab showing the settings for the default language?

Mentally, I sort of expected the Types-tab to be replaced by the Localize-tab when multiple languages are present. That is, there wouldn't be a Localize-tab, just a more advanced Types-tab.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

I spent another 30 minutes with this patch and I decided to commit it to CVS HEAD. It's a great patch and it fixes a long-standing problem. I did rename date_formats.inc to date.inc though.

The usability issue I described in #77 is real, but we can fix that in a follow-up issue after the Slush deadline.

We're still missing a CHANGELOG.txt entry though so I'm marking this 'code needs work'. Let's also create a follow-up issue for #77.

Good job all! :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

drupal_was_my_past’s picture