Improve documentation for date-related functions and hooks in system.module

Thanks!

The latest patch needs a "the" in this line:
+ * A date format array containing following keys:
(the following keys)

Otherwise, I think the patch is good.

Actually, http://api.drupal.org/api/function/system_date_format_types/7 defines long, medium, and short types. So I think it is not a bad idea to leave these strings in the documentation?

Also, since you are fixing things up, let's get everything right. Here are some remaining problems after your patch:

a) Maybe we should be more specific about the "attributes" that system_get_date_types() returns, in its @return? All it says now is "an array of date types", which doesn't tell us much.

b) system_get_date_formats - one-line description still refers to the "format length", which is incorrect.

c) "id" is a psychological term (ego, superego, and id). The abbreviation for identifier should always be ID. Check your dictionary... so this needs fixing:
+ * Gets the format details for a particular id.

d) This could be expanded to say what the "details" are:
+ * An array of date format details.

e) This needs a verb (is):
* The format string, or NULL if no matching format found.

f)

 /**
- * Delete a date type from the database.
+ * Deletes a date type from the database.
  *
  * @param $date_format_type
- *   The date type name.
+ *   A string containing the date type name.
  */
 function system_date_format_type_delete($date_format_type) {

Date type or date format type? I think there is a difference?

How about saying something like "For example, the system module defines 'long', 'medium', and 'short'; other types can be defined in the user interface and by other modules."

Regarding the date types vs. date format types... There is a difference. A date format is something like what's in system_default_date_formats(). A date type is something like what's in system_date_format_types(). They can be associated with each other, but they are not the same thing at all. The terminology needs to be made consistent. There are two hooks too: hook_date_formats() and hook_date_format_types().

Looking just at the date-related functions in system.module, there are actually three different types of things that are sometimes called "date formats".

1) Results of hook_date_format_types -- key/label pairs, along with attributes like the module they came from. No PHP date formats are part of this structure, so I think the name "date format type" is really stupid. They are things like 'long' => t('Long'). Functions that deal with these:
- system_get_date_types()
- _system_date_format_types_build()
- hook_date_format_types() (and system_date_format_types(), which implements this hook)
- hook_date_format_types_alter()
- system_date_format_type_save()

2) PHP date format strings (with keys). Functions that deal with these:
- hook_date_formats() (and system_date_formats(), which implements this hook)

3) The associations between #2 items and #1 items. Functions that deal with these:
- system_get_date_formats()
- _system_date_formats_build()
- system_date_formats_rebuild()
- system_date_format_save()
- system_date_format_delete()
- system_get_date_format()
- system_date_format_locale()
- system_date_format_type_delete() [primarily for deleting a #1 item, but also deletes the associations]

We need a consistent nomenclature for these three types of items.

Not to mention that the function format_date in common.inc is still hard-wiring 'long', 'short', and 'medium'. Can that possibly be correct?

This looks like a real mess, and it is not only documentation.

See also
#826486: format_date() does not respect admin-defined date formats

+++ modules/system/system.module	11 Aug 2010 17:36:41 -0000
@@ -3286,22 +3286,33 @@ function system_run_automated_cron() {
-  $date_format_types = &drupal_static(__FUNCTION__);
+function system_get_date_types($date_type = NULL) {
+  $date_types = &drupal_static(__FUNCTION__);
...
-  return $type ? (isset($date_format_types[$type]) ? $date_format_types[$type] : FALSE) : $date_format_types;
+  return $date_type ? (isset($date_types[$date_type]) ? $date_types[$date_type] : FALSE) : $date_types;

I don't understand why you renamed all instances of $type to $date_type? I prefer $type and would prefer to see you undo those changes. Ditto for changing $format into $date_format.

+++ modules/system/system.module	11 Aug 2010 17:36:41 -0000
@@ -3330,45 +3335,66 @@ function system_date_formats() {
+ *   - dfif: The date format ID.

This should be 'dfid' instead of 'dfif'. This typo is repeated a few times.

+++ modules/system/system.module	11 Aug 2010 17:36:41 -0000
@@ -3330,45 +3335,66 @@ function system_date_formats() {
+ *   - dfif: The date format ID.

Is that correct? Shouldn't 'dfif' be 'dfid'? This typo is repeated a few times.

+++ modules/system/system.module	11 Aug 2010 17:36:41 -0000
@@ -3381,35 +3407,37 @@ function system_date_formats_rebuild() {
-function system_date_format_locale($langcode = NULL, $type = NULL) {
-  $formats = &drupal_static(__FUNCTION__);
+function system_date_format_locale($langcode = NULL, $date_type = NULL) {
+  $date_formats = &drupal_static(__FUNCTION__);

I don't understand why you renamed $type to $date_type. $type is cleaner IMO.

Powered by Dreditor.

I agree with tstoeckler on #15. There are three different kinds of information floating around with date formats, and I think they need to be distinguished. Having different variable names for each kind of information will help in that effort.

That works for me... Though maybe $format_info or $format_item would be more descriptive for the array of format information?

Let's try it out in the form of a patch and see if it flies. I actually haven't reviewed the previous patch yet...

Here's a new patch.

It has fewer local variable name changes than the previous patch, and more changes to the documentation. I've attempted to be very clear about what is a PHP date format string, what is an array of information, and what is the name of a date type.

#21: 854396-21.patch queued for re-testing.

Can you provide just $type?

This is looking pretty good. A few mostly minor suggestions:

a)

+ *   - 'locales': (optional) an array of 2 and 5 character locale codes; for

"An" should be capitalized (list items should start with capital letters, and the rest of this list is fine).

b)

 /**
- * Alters date types and formats declared by another module.
+ * Alters date formats declared by another module.

Alters -> Alter. Hooks are written as if to say "You can use this hook to...".

c)
Looking at the code for http://api.drupal.org/api/drupal/modules--system--system.module/function... -- it looks like it returns an object and not an array? It's using fetch(), which I think should be returning an object.

I was going to fix up these few problems... but I'm having another problem with the patch. After patching, for some reason my system.api.php file is ending up with Windows rather than Unix line endings. The other two files are not having that problem... could be an artifact of Eclipse I suppose, but I'm not sure. So I guess I'll leave this to someone else to fix up.

This looks good to me, thanks tstoeckler!

Please note that although it looks like code changes occasionally in this patch, it's really just local variable names and doc blocks that are changing. There should be no API or functionality change. It should be clearer what's going on in the date format world after this patch, though.

Committed to CVS HEAD. Feel free to re-open based on #31. Thanks all.

Related issue filed today:
#989366: format_date() doesn't use programmatically created format.