Problems

  1. Date types have default formats. That's a fact. We want format_date($timestamp, 'mytype') to work even if the administrator has not configured a date format for that date type. For that to happen each module that provides a date type currently needs to implement hook_install():
    function mymodule_install() {
      variable_set('date_format_mytype', 'g:i:s \o\r \w\h\a\t\e\v\e\r');
    }
    

    That's very cumbersome and unintuitive (and senseless).

  2. Because of the nature of format_date() a date type with the machine name 'custom' doesn't work. That's because if you specify 'custom' as the $type parameter that allows to specify a custom format in the $format parameter. This is a completely unneccessary limitation.

Solutions

  1. This patch changes the return value of hook_date_format_types():
    // Before.
    mymodule_date_format_types() {
      return array(
        'mytype' => t('My type'),
      );
    }
    
    // After.
    mymodule_date_format_types() {
      return array(
        'mytype' => array(
          'title' => t('My type'),
          'default format' => 'g:i:s \o\r \w\h\a\t\e\v\e\r',
        ),
      );
    }
    

    We use 'title' instead of 'label' here because the title/label is already stored as 'title' in the DB. _system_date_format_types_build() now automatically takes care of setting the variables that had to be set manually before. Ideally we wouldn't set a variable at all, but simply invoke hook_date_format_types() in format_date() and access the default format from there, but format_date() can be called before full bootstrap, so that isn't possible.

  2. This patch splits format_date() into format_date(), which is responsible for formatting date types, and format_date_format(), which is repsonsible for formatting date formats. Examples:
    // Before.
    format_date($timestamp);
    // After.
    format_date($timestamp);
    
    // Before.
    format_date($timestamp, 'mytype');
    // After.
    format_date($timestamp, 'mytype');
    
    // Before.
    format_date($timestamp, 'custom', 'g:i:s \o\r \w\h\a\t\e\v\e\r');
    // After
    format_date_format($timestamp, 'g:i:s \o\r \w\h\a\t\e\v\e\r');
    

    The $timezone and $langcode parameters are retained as before and available in both functions. Internally format_date() just gets the format from the variable (that was saved earlier) and then hands it over to format_date_format.

Comments

tstoeckler’s picture

StatusFileSize
new5.38 KB
new5.38 KB

Less cruft from a previous approach...

tstoeckler’s picture

Version: 8.x-dev » 7.x-dev
Assigned: Unassigned » tstoeckler

Moving to 7.x for testbot. Also assigning to me.

In case it's non-obvious:
While it would be nicer to not have to store a variable for each date format, and then have format_date fetch the default format if no variable has been set, format_date can be called before full bootstrap, so we can't rely on module_invoke_all() which would be necessary for that.

tstoeckler’s picture

Version: 7.x-dev » 8.x-dev

Back to 8.x now that the tests are queued.

Status: Needs review » Needs work

The last submitted patch, format-date-default-3.patch, failed testing.

tstoeckler’s picture

Title: Automatically save date formats for date types » Make module-defined date types usable
Status: Needs work » Needs review
StatusFileSize
new7.38 KB

New patch.
See issue summary.

tstoeckler’s picture

Issue tags: +Needs tests, +API change

This needs (to update the existing) tests, but I want to wait for that until I get some positive feedback on the direction.

Regarding performance: The latest patch adds 1 function call to format_date(), although in certain circumstances variable_get() was called twice before, so its on par. I know format_date() gets called a lot, but I don't think this warrants any performance testing.

Status: Needs review » Needs work

The last submitted patch, 1092628-format_date.patch, failed testing.

tstoeckler’s picture

StatusFileSize
new11.97 KB

Found this and gave this is re-roll.
This became a little harder due to #1183250: Add a theme_datetime() function to consistently theme dates and datetimes. That added some more date types to core, but 'hidden' ones, which shouldn't be editable in the UI. Instead of properly adding that feature to the API, they just lumped those date types and their default formats in format_date(). This patch accounts for that.

PLEASE NOTE: The above sounds like I'm assigning blame but that's not what I'm trying to say. It's just a fact that format_date() and friends really are a mess. It's not the people in the above mentioned issue I'm mad at, it's common.inc/system.module. :)

I'm still in the process of fixing my dev environment so I'm just going to upload this and see what happens. That means this is completely untested, even in terms of fatals or so...

tstoeckler’s picture

Status: Needs work » Needs review

Of course...

rfay’s picture

Status: Needs review » Needs work

Moving to CNW because it seems to have an infinite loop in it. Thanks for the heads-up @tstoeckler.

Edit: When I looked at the testbot it was at 93,000 assertions and working away. The normal total for Drupal is about 36,000 or so these days. So it looks like one of the tests must just keep going forever. We've actually never seen this before. Anyway, after the max runtime of 90 minutes, the testbot resets, and that's what you were seeing.

tstoeckler’s picture

Oops, so sorry for that. I have no idea how on earth that could happen, but I'll find out.
Thanks, rfay!

tstoeckler’s picture

Status: Needs work » Closed (won't fix)

This got sort of fixed by #1571632: Convert regional settings to configuration system. The ability to provide date types as a module was removed completely...

tstoeckler’s picture

Issue summary: View changes

Provide issue summary for new patch.