format_date() doesn't use programmatically created format.

Don't confuse a format type (ie. 'short', 'medium', 'long') with a format (short:'Y-m-d H:i' or short:'m/d/Y - H:i'). The 'date_format_' . $type does the mapping between the two.

That means that there is no bug here.

just a quick sanity check on #2 ... your functions are actually called "mymodule_date_formats" not "hook_date_format", right?

So it seems to work if you go to 'admin/config/regional/date-time' and choose 'save'. Essentially just having #2 in a module isn't enough to get date_format_agenda and date_format_article added to {variable}.

Also, I'm not seeing anything appear in {date_format_locate} which seems like a bug.

This is pretty much by design.

hook_date_format_types() declares new format types, and hook_date_formats() declares new formats to use with a format type. The date_format_[type] variable and the {date_format_locale} table do the mapping between the two.

No facility is provided to define a default format for use with a format type in those hooks: your module will have to set the date_format_[type] variable if you want to set the default format.

Well if no code changes are going to happen, the docs for hook_date_format_types() and hook_date_formats() need to describe how a module can set a default date_format_[type] variable in the module's install code.

The documentation of these date-related functions was just updated last night on
#854396: Improve documentation for date-related functions and hooks in system.module
But I think it could still be improved. It's stated now that format_date() takes the name of a date type defined by hook_date_format_types(), but it looks like the person who filed this issue knew that.

But as noted above, this depends on
$format = variable_get('date_format_' . $type, '');
returning a format string, which is not automatic. It happens when you visit the admin page for defining date formats and assign the formats, but not just by implementing the hooks.

So, I agree we should add to the documentation of the two hooks, to explain how to connect a date format and a date type together. This is therefore a normal documentation bug, not a major base system support request.

This is clearly a bug we need to fix on the code-level, not on the documentation level.

If someone uses a custom date type with format_date() somewhere on their site, then submitting the date admin page will change the format of displayed dates. I don't think that is acceptable.
For each date type we should be getting the variable, and if it isn't set just take the first one in the list of possible formats. That doesn't really doesn't count as an API change, because #826486: format_date() does not respect admin-defined date formats which introduced this behavior, was fixed just a week ago. I volunteer to roll a patch in the next few days.

I'm not sure what exactly you're advocating, but I'm sure that communicating it in the form of a patch will make that clear. :)

We have this function called system_get_date_formats() that seems to be building a correspondence between locales and date format types. Should we perhaps be using that? It appears it is only used to build the admin screens, and then never again, which is a bit strange.

Well, here's a patch.
It doesn't seem to break anything, but calling a system.module function from common.inc didn't really feel right.
If I get a thumbs up for this, I'll write some tests.

No #14, this is definitely not what we want.

As already stated, there is no bug here. The only thing we need to document is that it's up to the module defining a new date/time format type to register the corresponding variable that sets the default format for this format type.

OK, here's a purely doc patch, which hopefully clarifies things a bit. There are additions to the doc for format_date() in common.inc, and hook_date_format_types() and hook_date_formats() (in that order) in system.api.php.

subscribing

I still consider this to be a bug, even if #14 is not how to solve it. It just makes the code flow ridiculous, which is proven by the docs patch in #16.

Before #826486: format_date() does not respect admin-defined date formats you had to do the following:

$format = variable_get("date_format_$type", NULL);
if (empty($format)) {
  // Find out the default format, see #14 for an example.
}
format_date($timestamp, 'custom', $format);

(at least 5 LOC)


With that landing we wanted to be able to do:

format_date($timestamp, $type);

(1 LOC)


But now we realized we instead have to do:

$format = variable_get("date_format_$type", NULL);
if (empty($format)) {
  // Find out the default format, see #14 for an example.
}
variable_set("date_format_$type", $format);
format_date($timestamp, $type);

(at least 6 LOC)


So I suggest either fixing format_date(), and if that is not possible/feasible rolling back #826486: format_date() does not respect admin-defined date formats.
Either way the API documentation should not document to use variable_set(), but simply format_date($timestamp, 'custom', $format), because that's 1. less code 2. less function calls/overhead

Re-categorizing (again).

Marking jhodgdon's documentation patch in #16 RTBC. I agree with @tstoeckler that it isn't perhaps the ideal solution for this issue, it does bring some clarity to the matter. At least this way module developers will know that they'll need to call variable_set('date_format_' . $type, $format) in the module install file (or elsewhere) in order to actually use that date format.

After this documentation patch is committed, perhaps we could return to trying to find a clever solution to avoid this requirement.

Hrm. :\ I can definitely see this being a big WTF. :\

I've committed #16 to HEAD to at least fix the docs.

But this seems like something we should definitely make more intuitive. I can't quite figure out if doing so is a D7 or D8 task though, because I'm not sure what code changes would be required, and Damien seems so dead-set against it for reasons I don't really understand. More clarity would be appreciated.

For now, marking back to "needs work".

I did some thinking... Here's the situation:

a) There are "date types" and "date formats" in the date API.
b) Date types are basically just names. They are defined by hook_date_format_types and hook_date_format_types_alter() (or in the UI). They are just an association between a machine name and a human-readable name for a date type.
c) Date formats are basically PHP date formats, which are restricted to associating with particular date types and locales. They are defined in hook_date_formats (or in the UI).
d) Since any module can define possible formats for any date type name, whether that module "owns" the date type or not, just implementing hook_date_formats doesn't make that association "stick" for a particular locale. Instead, an admin has to choose one of the possible choices from the UI, or alternatively, a module can do the variable_set() that we documented in the patch in #16 to make an association between a date type and a PHP date format.

I think I agree with Damien that this is probably the correct behavior. I think we need a bit more documentation to clarify this though. Here's a patch.

These failures are *not* due to this patch. I just filed:
#1013572: Test failures due to failed user login

I'm attaching the patch again so I don't wipe out the previous test results.

This continues to improve the documentation, but the whole thing does seem a bit of WTF to me as well. Marking the follow-up documentation patch in #23 as RTBC since it does add clarity to how date formats are assigned to date types, especially the note:

+ * To define a date type in a module and make sure a format has been assigned to
+ * it, without requiring a user to visit the administrative interface, use
  * @code variable_set('date_format_' . $type, $format); @endcode

While working on this earlier, I uncovered another bug --- #989886: _system_date_format_types_build improper use of in_array.

Forgot to change status to RTBC, see previous comment.

Thanks for the expanded docs. That definitely helps.

Committed to HEAD.

I guess the correct status for this issue then is fixed if at least two people are convinced it's working properly. Still seems like it might benefit from a DX improvement issue in D8 though.

I didn't mean to imply that it was an excellent API. The whole date API is kind of a mess, and probably some DX cleanup in D8 would be useful.

But I don't think we can really change it at this point, and I don't think that anything in the API is really rising to the level of a bug (aside from that one bfroehle just caught), as long as you look at it from the point of view of how the hooks are documented currently (i.e. that the two date hooks are meant to supply possibilities to the admin UI).

Hello. this code works, it's a real example.

<?php
/**
* Implements hook_date_format_types().
*/
function micolehelp_date_format_types() {
  return array(
    'portada'  => t('portada'),
    
  );
}

/**
* Implements hook_date_formats().
*/
function micolehelp_date_formats() {
  $formats = array();

  $formats[] = array(
    'type'    => 'portada',
    'format'  => 'd|M<\\s\\p\\a\\n>Y</\\s\\p\\a\\n>', // 24|Jun <span>2012</span>
    'locales' => array(),
  );


  // save the ne date fortmas into variables
  foreach ($formats as $format) {

    variable_set('date_format_' . $format['type'], $format['format']);

  }

  return $formats;
}

?>

oskar

It would be best to only do this once in a hook_install() or within a distribution profile or something ;)