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.
Comment | File | Size | Author |
---|---|---|---|
#76 | drupal.localize-date-formats.patch | 77.07 KB | sun |
#75 | 11623.patch | 77.07 KB | stella |
#72 | 11623.patch | 77.07 KB | stella |
#71 | 11623.patch | 77.07 KB | stella |
#70 | 11623.patch | 76.88 KB | stella |
Comments
Comment #1
Gábor HojtsyWell, 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.
Comment #2
killes@www.drop.org CreditAttribution: killes@www.drop.org commented*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.
Comment #3
Gábor HojtsyYes, 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.
Comment #4
Steven CreditAttribution: Steven commentedI 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.
Comment #5
Gábor HojtsySteven, you say:
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.
Comment #6
Steven CreditAttribution: Steven commentedWhat 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.
Comment #7
Gábor HojtsySteven, 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.
Comment #8
Steven CreditAttribution: Steven commentedI'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.
Comment #9
Pancho************************
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
Comment #10
Panchoedit: deleted
Comment #11
PanchoI 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.
Comment #12
Steven CreditAttribution: Steven commented- 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.
Comment #13
PanchoFirst 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
Comment #14
Steven CreditAttribution: Steven commentedFirstly:
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.
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.
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.
Comment #15
Bèr Kessels CreditAttribution: Bèr Kessels commented+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.
Comment #16
Uwe Hermann CreditAttribution: Uwe Hermann commentedDoesn't apply anymore. Also, please provide _one_ patch (not one for each file you change).
Comment #17
Jaza CreditAttribution: Jaza commentedMoving to 6.x-dev queue. This still seems to be a relevant issue.
Comment #18
Jaza CreditAttribution: Jaza commentedUnassigning 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.
Comment #19
Pancho<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.
Comment #20
PanchoMoving feature requests to D7 queue. Sorry...
Comment #21
Sutharsan CreditAttribution: Sutharsan commentedMoving all usability issues to Drupal - component usability.
Comment #22
dev13 CreditAttribution: dev13 commentedThis can be done currently with the internationalization (i18n) module and Multilingual Variables (http://drupal.org/node/313272). The variables involved are:
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.
Comment #23
KarenS CreditAttribution: KarenS commentedWe 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.
Comment #24
stella CreditAttribution: stella commentedI'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
Comment #25
sunTagging. Would be nice to get this in.
Comment #26
stella CreditAttribution: stella commentedVery rough initial draft, just starts laying the ground work for the localization.
Comment #27
stella CreditAttribution: stella commentedPatch re-roll. Still need to add in localization stuff
Comment #28
stella CreditAttribution: stella commentedComment #29
stella CreditAttribution: stella commentedThe 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.
Comment #30
stella CreditAttribution: stella commentedPatch re-roll with usability improvements.
Comment #31
catchI 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:
No phpdoc.
This should be using #attached now.
If we really need this, it should be a class with associated CSS.
Single quotes (and elsewhere).
I'm on crack. Are you, too?
Comment #32
KarenS CreditAttribution: KarenS commentedI 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?
Comment #33
catchI'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.
Comment #34
stella CreditAttribution: stella commentedPatch 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:
admin/config/regional/settings
and places them underadmin/config/regional/date-time
date_format_types
table.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).hook_date_formats()
for long, medium, short in a newincludes/date_formats.inc
file.date_formats
table and can be re-used by multiple date types.hook_init()
which detects the user's language setting and loads the appropriate date formats.Comment #35
stella CreditAttribution: stella commentedRegional settings page
Add new date type
List of available date types
Add new date format
List of date formats
Language list
Localize date formats for French
Localized date formats demo:
Uploaded with plasq's Skitch!
Comment #36
catchIt's lovely.
Comment #37
seutje CreditAttribution: seutje commentedthis is awesome, how did I never bump into this?
Comment #38
stella CreditAttribution: stella commentedFew minor tweaks.
Comment #39
KarenS CreditAttribution: KarenS commentedI 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 :)
Comment #40
nedjoI 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:
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?
Could/should this be done instead at the validate stage to avoid having to override system_settings_form_submit()?
Why are we hiding this? A comment would be good.
The 'text' should be using Drupal.t() instead in the .js file, since that's how we provide localized strings.
Comment #41
Dries CreditAttribution: Dries commentedThis 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.
Comment #42
stella CreditAttribution: stella commentedCurrently working on simpletests.
To respond to Nedjo's comments:
I've reworked the patch so this line is no longer needed.
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.
Comment #43
heather CreditAttribution: heather commentedShould 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.
Comment #44
stella CreditAttribution: stella commentedRe-rolled patch with simpletests.
Comment #46
stella CreditAttribution: stella commentedComment #48
stella CreditAttribution: stella commentedI can't reproduce this test failure locally. Anyone?
Comment #50
stella CreditAttribution: stella commentedComment #51
stella CreditAttribution: stella commentedResetting to RTBC now that tests pass.
Comment #52
stella CreditAttribution: stella commentedtidied up some tests
Comment #53
sunAll 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.
Does this have to live in a separate include file? Why not in locale.inc or iso.inc?
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.
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.
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)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.
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. :)
Shouldn't these assertions use $node->created instead of REQUEST_TIME?
What's happening here?
I now realize this code has been moved only - squeezing a comment in there would still be neat. :)
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.
Shouldn't this be statically cached?
Also, shouldn't this record + return the implementing 'module' by using module_implements()?
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.
!empty() is sufficient here.
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).
s/For storing/Stores/
That said, shouldn't the third table be in Locale module's schema? Not sure though.
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.
2x "settings" in the description, just remove it at the end. :)
Why does this happen during cron runs? Wouldn't drupal_flush_all_caches() resp. system_flush_caches() be more appropriate?
We can straight return the database query result here.
PHPDoc summary should be on one line ;)
We should use a drupal_static() here and remove the $reset argument.
This review is powered by Dreditor.
Comment #54
Dries CreditAttribution: Dries commentedLet's try to incorporate sun's comments.
Comment #55
KarenS CreditAttribution: KarenS commentedBased on Dries' comment in #41, this qualifies as an exception.
Comment #56
KarenS CreditAttribution: KarenS commentedRe #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.
Comment #57
hass CreditAttribution: hass commentedYeah, a singular table name doesn't work here :-(.
Comment #58
stella CreditAttribution: stella commentedPatch re-roll which should address the majority of the items raised by sun above.
Just a couple of points:
No, it's called from system_get_date_formats() only when its static cache needs to be rebuilt
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
Comment #60
mattyoung CreditAttribution: mattyoung commented.
Comment #61
stella CreditAttribution: stella commentedPatch re-roll
Comment #62
sunTagging 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.
Comment #64
stella CreditAttribution: stella commentedPatch re-roll
Comment #65
sunThis looks much better now. It seems like most of my comments have been addressed.
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/
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?
"An array"? It seems to be an indexed array, right? Elsewhere, we call this simply "a list".
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?
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. :)
$ret is gone :)
You don't need to return something here, just remove the return line.
Hm... this hook isn't documented, so I'm starting to get even more confused. :-/
We now have a stale comment about 'is_new' here, it seems.
This review is powered by Dreditor.
Comment #66
stella CreditAttribution: stella commentedOk 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
Comment #67
sun(and elsewhere) Albeit, grammatically correct in some slangs, we do not use double-spaces between sentences in PHPDoc. See http://drupal.org/node/1354
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?
(and elsewhere) Please remove the trailing periods.
@see #178629: Properly use @see
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. :)
Wrong indentation here.
I'm on crack. Are you, too?
Comment #68
stella CreditAttribution: stella commentedComment #69
sunFinal pass. :)
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.
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).
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.
However, since this is a standard form theme function, you can as well remove the @param declaration.
Very minor: missing trailing comma.
Minor: uhm, why not simply
$formats[$id]['locales'][] = 'en-ca';
?
Won't hold up this patch.
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.
Comment #70
stella CreditAttribution: stella commentedI 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.
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.
Comment #71
stella CreditAttribution: stella commentedOops, attached the wrong version of the patch.
Comment #72
stella CreditAttribution: stella commentedOk 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 :)
Comment #73
sunI hope the tests will pass on this one.
Comment #75
stella CreditAttribution: stella commentedForgot to update the tests for the change in the paths. Fixed now.
Comment #76
sunTestbot hangs on re-test.
Comment #77
Dries CreditAttribution: Dries commentedI 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.
Comment #78
Dries CreditAttribution: Dries commentedI 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! :)
Comment #80
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedMarked #320039: Make custom date possibility more obvious as a duplicate.