format_date() does not respect admin-defined date formats

At a minimum, this would need some updated documentation added to the documentation header for the function to explain what it's supposed to be doing, before it can get an adequate review.

#5: admin_defined_format_date_3.patch queued for re-testing.

This looks very nice, and I think it should be committed. Probably needs another review before it can be RTBC.

I think it's actually a bug in Drupal that format_date() is not respecting types that are defined by the hooks and in the UI, since in D7, 'short', 'medium', and 'long' are no longer supposed to be so special -- they just happen to be the date format type names that system_date_format_types() returns, and should not be treated specially as opposed to any other hook_date_format_types() implementations or any user-defined types.

So I am changing this from a feature request to a bug.

The patch still applies and is a great addition, in my opinion. Then, also, the documentation of hook_date_format_types() won't be wrong anymore.

+++ includes/common.inc	18 Jun 2010 19:20:33 -0000
@@ -1811,8 +1812,14 @@
+      $format = variable_get('date_format_' . $type);

Sorry, but I have troubles to follow... http://api.drupal.org/api/function/system_get_date_types/7 does not use variables to store custom date types. So what is this code for?

Is it possible that this patch does not account for a major new feature in Drupal 7? (Custom date types and formats)

+++ includes/common.inc	18 Jun 2010 19:20:33 -0000
@@ -1811,8 +1812,14 @@
+      if (is_null($format)) {

Should be !isset()

Powered by Dreditor.

Sorry, but I have troubles to follow... http://api.drupal.org/api/function/system_get_date_types/7 does not use variables to store custom date types. So what is this code for?

This is where the selected format finally gets stored, apparently. Just try it out!
Write a module implementing hook_date_format_types(), or create a new date type in the admin UI, apply this patch and you'll be able to use format_date() with your new type.
The fact that people have trouble following what is going on in the date format/type functions is a reason to fix at least part of it, not to hold up this patch. I myself probably wouldn't have found out how to format a date with an admin-created format, without finding this patch.

@sun:

Sorry, but I have troubles to follow [...]

See http://api.drupal.org/api/function/system_date_time_settings/7 (it's a system settings form) and http://api.drupal.org/api/function/locale_init/7.

I think we can do better than that for localized values, at least because now we have http://api.drupal.org/api/function/hook_language_init/7 to initialize multilingual variables. Moreover we might want to store the values in the variables table as a serialized array to avoid querying the DB at each page request.

But this is another issue, I'd say the patch is good and can go in once that is_null is fixed.

OK, good. Here is a revised patch with isset().

Crossposted. I was setting this RTBC too :)

This is a feature request, not a bug report so moving to Drupal 8. I haven't reviewed the actual code yet.

Actually this fixes a core API misbehavior: we have introduced admin-defined date formats but we can't use them through the related API. The code is pretty straightforward and does not introduce any API or other kind of harmful change. Moreover it provides additional test coverage. Please reconsider.

The bug here is that we already have documentation stating that this is possible. We can either fix the documentation, or fix the behaviour right away, and the latter already has an RTBC patch (besides having a far better result for developers). I second plach here.

Given earlier and most recent explanations in this issue, I agree that this is a bug. I was under the impression that the new date formats would work differently, but I was wrong, sorry.

Related issue #939392: Localized date formats are initialized too late.

Actually this allows us to completely remove the special-casing for the core provided date types.
Also revamps the documentation for that function a bit, and makes e.g. the use of "date type" consistent, and makes the parameters consistently "(optional)".

Also: this is definitely a bug.

+++ includes/common.inc	9 Nov 2010 17:47:48 -0000
@@ -1862,18 +1862,15 @@ function format_date($timestamp, $type =
-      $format = variable_get('date_format_short', 'm/d/Y - H:i');
...
-      $format = variable_get('date_format_long', 'l, F j, Y - H:i');

Are the default variable values already set elsewhere (during installation) ?

+++ includes/common.inc	9 Nov 2010 17:47:48 -0000
@@ -1862,18 +1862,15 @@ function format_date($timestamp, $type =
       break;
...
     default:

There should be a blank line between different switch cases, unless the code intentionally falls through.

However, depending on above mentioned system variable default values, we could as well replace the entire switch statement with a simple if ($type != 'custom') condition.

Powered by Dreditor.

Better title.

The documentation in this patch for the format_date() function is not clear, in my opinion. How about this (may need line wrapping and whitespace cleanup, since I just edited it here in this comment form):

/**
 * Formats a date, using a date type or a custom date format string.
 *
 * @param $timestamp
 *   The date/time to format, as a UNIX timestamp.
 * @param $type
 *   The format to use, one of:
 *   - 'short', 'medium', or 'long' (the corresponding built-in date formats);
 *     'medium' is the default.
 *   - The name of a date type defined by a module in hook_date_format_types().
 *   - The machine name of an administrator-defined date format.
 *   - 'custom', to use $format.
 * @param $format
 *    If $type is 'custom', a PHP date format string suitable for input to date(). Use
 *    a backslash to escape ordinary text, so it does not get interpreted as date
 *    format characters.
 * @param $timezone
 *   (optional) Time zone identifier. Defaults to the user's time zone.
 * @param $langcode
 *   (optional) Language code to translate to. Defaults to the language used
 *   to display the page.
 *
 * @return
 *   A translated date string in the requested format.
 */

subscribe & +1 for getting this fix into D7

Rerolled #12 with jhodgon's PHPdoc from #25.

The whitespace on the doc isn't right. There should be a space before each *, and the indentation in the @param $format is 3 spaces instead of 2.

The code looks right to me though.

Fixed the phpDoc.

Fixed the code logic.

Wasn't happy with the isset($format) check, slightly revised. I'm happy now.

yes, I explicitly added the variable_get() default value of '' (empty string) in order to decrease confusion with the already existing function argument $format, which also defaults to ''.

+1 for RTBC on #31.

No need to be shy ;)

-    case 'medium':
+
     default:
+      $format = variable_get('date_format_' . $type, '');
+      if ($format !== '') {
+        break;
+      }
+      // Fall-through to 'medium', if requested format is undefined.
+    case 'medium':
       $format = variable_get('date_format_medium', 'D, m/d/Y - H:i');
+      break;

Now, this is just weird. We don't do this "default falls through to something specific" anywhere else with a switch statement and neither does php.net; default always comes last. I didn't even know this was possible.

It's not clear to me why it is needed, either. Everywhere else where we want the same behaviour to happen on a specific and default we do:

  case 'specific':
  default:
    // Whatever code.

then..

- *   Time zone identifier; if omitted, the user's time zone is used.
+ *   (optional) Time zone identifier. Defaults to the user's time zone.

Since you decided to kill kittens and make this "fix everything that was ever wrong with format_date() instead of just fixing the bug, let's also specify an example of what is meant here by "time zone identifier." Is this "UTC" or is it "+4" or?

Attached is an updated patch without the switch-fallthrough-weirdness.

Since "time zone identifier" is, as far as I can see, never explained in Drupal but always uses strings like "America/Los_Angeles", I don't think we have to document that here. Otherwise we should probably just add a link to the PHP documentation.

If I just had a cent for every wrong negation in my code …

Code looks good. Documentation should be more explicit about time zone identifiers, as per #36. It might be used consistently throughout core, but for most people this will be the one and only date-relate function they'll ever call in Drupal, so an example can't hurt.

+++ includes/common.inc	28 Nov 2010 16:18:23 -0000
@@ -1815,26 +1815,28 @@ function format_interval($timestamp, $gr
  * @param $timestamp
- *   The exact date to format, as a UNIX timestamp.
+ *   The date/time to format, as a UNIX timestamp.

Both the old and the new reads weird. How about changing to:

"A UNIX timestamp to format."

+++ includes/common.inc	28 Nov 2010 16:18:23 -0000
@@ -1815,26 +1815,28 @@ function format_interval($timestamp, $gr
+ *     'medium' is the default.
...
+ *   Defaults to 'medium'.

Duplicate information about the default. Let's keep the last only.

+++ includes/common.inc	28 Nov 2010 16:18:23 -0000
@@ -1865,15 +1867,25 @@ function format_date($timestamp, $type =
+    case 'medium':
+      $format = variable_get('date_format_medium', 'D, m/d/Y - H:i');
+      break;
...
     default:
-      $format = variable_get('date_format_medium', 'D, m/d/Y - H:i');
+      $format = variable_get('date_format_' . $type, '');
+      if ($format === '') {
+        // Use 'medium' if requested format is undefined.
+        $format = variable_get('date_format_medium', 'D, m/d/Y - H:i');
+      }

The idea was to not duplicate the date_format_medium variable. Let's do:

case 'medium':
default:
  // Retrieve the format of the custom $type passed.
  if ($type != 'medium') {
    $format = variable_get('date_format_' . $type, '');
  }
  // Fall back to 'medium'.
  if ($format === '') {
    $format = variable_get('date_format_medium', 'D, m/d/Y - H:i');
  }
  break;

Powered by Dreditor.

OK, makes sense. Only change here is the documentation for the $timezone parameter.

#43: format-date-locale.42.patch queued for re-testing.

#42 has not been fully addressed.

Also is "Defaults to the user's time zone." really accurate? Which user? And wouldn't that depend on settings -- I think there is a setting for using a site-wide time zone?

Like the patch's file name suggests, I hadn't even seen that comment when creating the patch.
In fact, I just saw it now that you pointed it out. The new patch contains all of sun's comments in #42, as well as Jennifer's from #46.

Thanks!

Looks like all my feedback was addressed, thanks!

Committed to HEAD.

follow-up issue (or at least related issue):
#989366: format_date() doesn't use programmatically created format.

While it's a bug shouldn't this go into D6 too?

I might be completely on crack, but I don't think Drupal 6 allows you to define your own date formats?! (IIRC that was a date.module feature in D6)
Marking back to fixed.