Problems
- 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 implementhook_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).
- 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
- 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 invokehook_date_format_types()informat_date()and access the default format from there, butformat_date()can be called before full bootstrap, so that isn't possible. - This patch splits
format_date()intoformat_date(), which is responsible for formatting date types, andformat_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 toformat_date_format.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 1092628-8-format_date.patch | 11.97 KB | tstoeckler |
| #5 | 1092628-format_date.patch | 7.38 KB | tstoeckler |
| #1 | format-date-default-3.patch | 5.38 KB | tstoeckler |
| format-date-default-2.patch | 6.25 KB | tstoeckler |
Comments
Comment #1
tstoecklerLess cruft from a previous approach...
Comment #2
tstoecklerMoving 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.
Comment #3
tstoecklerBack to 8.x now that the tests are queued.
Comment #5
tstoecklerNew patch.
See issue summary.
Comment #6
tstoecklerThis 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.
Comment #8
tstoecklerFound 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...
Comment #9
tstoecklerOf course...
Comment #10
rfayMoving 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.
Comment #11
tstoecklerOops, so sorry for that. I have no idea how on earth that could happen, but I'll find out.
Thanks, rfay!
Comment #12
tstoecklerThis got sort of fixed by #1571632: Convert regional settings to configuration system. The ability to provide date types as a module was removed completely...
Comment #12.0
tstoecklerProvide issue summary for new patch.