The second parameter, $type, in a call to format_date() can be 'small', 'medium' or 'large' to indicate which date format to use. However, on the admin page for defining these formats they are referred to as 'short', 'medium' and 'long', and the actual variable names use _short, _medium and _long - see the following code from format_date() in /includes/common.inc:
switch ($type) {
case 'small':
$format = variable_get('date_format_short', 'm/d/Y - H:i');
break;
case 'large':
$format = variable_get('date_format_long', 'l, F j, Y - H:i');
break;
case 'custom':
// No change to format.
break;
case 'medium':
default:
$format = variable_get('date_format_medium', 'D, m/d/Y - H:i');
}
My request is that this switch block simply cater for both 'small' and 'short' in the first block, and 'large' and 'long' in the second block. This means that if a developer wishes to use consistent terminology they can (this is useful when creating variable names from a string parameter) but the code will also perform exactly as is for existing calls.
case 'small':
case 'short':
$format = variable_get('date_format_short', 'm/d/Y - H:i');
break;
case 'large':
case 'long':
$format = variable_get('date_format_long', 'l, F j, Y - H:i');
break;
case 'custom':
// No change to format.
break;
case 'medium':
default:
$format = variable_get('date_format_medium', 'D, m/d/Y - H:i');
}
Just an idea to make things easier, as I have just coded up calls to format_date using 'long' and was wondering why it was not working!
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 368408-format_date-small_large-short_long.patch | 13.97 KB | jeffschuler |
| #1 | _common.formatdate.patch.txt | 466 bytes | jonathan1055 |
Comments
Comment #1
jonathan1055 commentedI have made a patch file for this simple change. Hopefully it will be regarded as a useful change to make drupal date calls easier. I have changed the version to 7.x as i guess you are not making improvements to the 6.x stream any more.
Jonathan
Comment #2
jonathan1055 commentedForgot in the last post to set the status to 'needs review'.
Comment #3
dries commentedGood catch. Let's fix this properly -- no need to preserve backward compatibility so feel free to remove what needs to be removed.
Comment #4
jonathan1055 commentedHi Dries,
Thanks for the response.
However, I'd just like to question your comment that there is no need to preserve backward-compatibility. I've just done a simple scan of my drupal directory and there are 58 different files which contain at least one to call to format_date(). Sure, I know many of these will be calling it without a second parameter, but for the cases which do use 'short' or 'long' do we really want to force the need to change these calls?
I spent some time trying to work out how things could be changed to have a cleaner system without having to change all the existing calls to format_date(). Maybe changing the admin date config page to use the 'small' and 'large' terminology and changing the stored variable names to 'date_format_small' and 'date_format_large'. But then there are still many places where these variable names have been used, and these would all have to be changed too (12 occurences of date_format_short in core and the contrib modules I use, and 10 of date_format_long). But after these changes we would have a consistent terminology, and the use of 'short' and 'long' would simply disappear - apart from in the memory of all the developers who have used them in the past, and would continue to use them and discover their code not working.
This seems to be a lot of work, and fraught with pitfalls, when just catering for both the alternative parameter values in format_date() is a very simple and very safe change.
Your call obviously, but I just thought it was worth working through some of the implications.
Jonathan
Comment #5
jeffschuler@jonathan1055: good catch. Check out On backward compatibility: the drop is always moving.
This patch replaces all
small's andlarge's that refer toformat_date()'stypeparameter withshort's andlong's, respectively.Comment #6
dries commentedThis looks good. Committed to CVS HEAD. Thanks!
I'm marking this 'code needs work' because we'll want to update the upgrade instructions in the handbook here on drupal.org.
Comment #7
jeffschuler#221006: Handbook is not up to date on format_date applies to the documentation for this issue.
I've reopened that for 7.x, changed the title on this issue to be more meaningful, and am marking this one Fixed.
I hope that's an okay way to do this.
Comment #8
jonathan1055 commentedThanks for coming back to this issue, and thank you for pointing me in the direction of the handbook post on backwards compatibility. Having read that article, and also Dries's original on his site, I now understand the reply in #3 and I get the principle of not dragging along code baggage. Thanks for commtting this to 7-x.
In preparation for the change required by module developers and to allow us to start using the new names, would it be helpful to commit to 6-x my original patch in #1 to cater for both versions? I don't know how this kind of change fits with the usual upgrade routes. I am guessing that many changes for converting modules from Drupal6 to Drupal7 cannot be done early within D6, so if this also has to be left out then I accept that. Just thought it worth asking the question.
Jonathan
Comment #9
jeffschulerHi Jonathan,
We change D7 core, then developers change their module code (to maintain compatibility) when upgrading their module to D7.
If we were to make this change in D6, users would upgrade their D6 installation and their modules would no longer work properly -- until module maintainers fixed their code to account for this change.
Since there are essentially no D7 modules in production: we can improve the Drupal API on that branch and not worry about breaking module compatibility.
Check out: Code Freeze in the Drupal Terminology handbook, and Drupal 7 code freeze: September 1st.
Comment #10
jonathan1055 commentedHi Jeff,
Sorry, maybe I wasn't very clear in my last post. I was not suggesting that the whole fix for D7 is also implemented in D6, as I know that would immediately break lots of code. What I was suggesting was to apply my original patch which simply catered for both terms in format_date() so that developers can start using the new strings straight away, and as such prepare for the conversion to D7 early. Also new developers can start using the correct terms without having to re-learn when D7 comes along. I'm sure I've seen other code examples which have a 'transitory' fix in D6 in preparation for a smoother full change required by D7.
Jonathan
Comment #11
jeffschulerMany apologies for not reading very closely, Jonathan.
That's an interesting thought and makes more sense, but still doesn't seem very compelling.
I don't think it would benefit a D6 module to make that change, or realistically make the transition any easier or straightforward. Docs on this change would still be necessary for transition.
And while I appreciate the aim for consistency, I think the confusion added by including an extra choice outweighs.
My very humble opinion is that the issue has been brought to an appropriate resolution, and any further energy should be focused on the transition documentation in the handbook, as Dries suggested...
Comment #12
jonathan1055 commentedYes, that's fine. Thanks for taking the time to reply.
As #221006: Handbook is not up to date on format_date covers the necessary documentation changes then this issue can be left to close after two weeks.
Comment #13
jeffschulerUgh. Going in circles. Sorry for the runaround.
At jhodgdon's suggestion, I'm setting this back to Needs Work as Dries had -- as it needs to be added to Converting 6.x modules to 7.x -- and tagging as Needs Documentation.
Comment #14
yrocq commentedDocumention done
#d7csmtl