The date-time format is hard-coded currently, but I want to fix to editable configuration.
Because the date format has the difference by the country.
For instance, day of the week is not put first in Japanese date format.

I made a simple patch. Please review, improve, and apply it.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ChrisKennedy’s picture

Agreed that this feature should allow greater customization.

Another solution would be to have a "custom" option in the select box, which would then allow the user to specify the date text in a textfield. If jQuery is enabled it could be hidden by default and only shown when custom is selected. Then in the form_submit the custom value is only used if "custom" is selected in the select list.

This would allow most users to use the pre-made options if they desire.

Crell’s picture

Version: 5.x-dev » 6.x-dev
Category: bug » feature

This is a feature request and Drupal 5 is frozen, but I would be in favor of this for Drupal 6.

ChrisKennedy’s picture

Title: date-time format localization issue » Custom date-time formats
FileSize
6.15 KB

Here's an implementation of my proposal in #1 which allows custom formats while continuing to supply the current built-in options.

ChrisKennedy’s picture

FileSize
6.26 KB

Synced.

ChrisKennedy’s picture

Assigned: Unassigned » ChrisKennedy

Assigning.

Darren Oh’s picture

Status: Needs review » Needs work
FileSize
6.52 KB

The patch doesn't work for me. I tried using m/d/Y as the medium date format and 'cu2728o02' was displayed instead of the date.

I'm attaching a patch which provides a link to the date format documentation in the PHP manual for Drupal users who happen not to be programmers.

ChrisKennedy’s picture

Status: Needs work » Needs review
FileSize
6.67 KB

Thanks Darren - it should be fixed now.

Takafumi’s picture

The #7 patch works fine, but it have need to fix for display example.

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for a great patch.

BioALIEN’s picture

+1 for this patch's inclusion into D6 and possibly backported to D5.x?

Dries’s picture

I tried this patch and it works. :) However, it feels a little bit awkward to use and I'm left wondering if we can come up with some simple UI improvements to make it even better. Waiting for a more ideas/feedback ...

m3avrck’s picture

FileSize
40.65 KB

Dries, I don't think this is too cumbersome, but I think the interface could be improved a bit.

See attached. I prefer #1 --- it puts the custom to the right of the select box which makes sense. Removes the redundant "Custom date text" since the select box already says it. Not only that, but it *prepopulates* the box with some custom code, just to give the user an indication of what is required.

BioALIEN’s picture

Hmm, pre-populating the custom date field would require JavaScript enabled, how could we degrade this gracefully while preserving the functionality?

Otherwise, +1 for m3avrck's #1 concept. Simple and straight forward.

m3avrck’s picture

If they are l33t enough to have JS off, then the are prob l33t enough to figure out the PHP custom code, so perhaps it's just blank in that case?

Darren Oh’s picture

I like the side-by-side arrangement. The field could be have the current format as a default value if JavaScript is not enabled.

BioALIEN’s picture

@Darren Oh: this would probably be the best way of doing it. The rest is as m3avrck suggested, all JS driven.

m3avrck’s picture

Status: Reviewed & tested by the community » Needs work
ChrisKennedy’s picture

Status: Needs work » Needs review
FileSize
8.11 KB

Okay here's an updated patch to show the custom input to the right of the regular select, default the custom format to the initial regular format, and hide the labels via jQuery (they should probably still be shown if js is disabled, since the custom format input will always be shown).

Showing the custom format side-by-side makes the CSS a bit trickier, so let me know if there are any suggestions. The only way I could get it not to wrap strangely was to set the width (50%) on the right-hand column.

m3avrck’s picture

Status: Needs review » Needs work

Patch looks good. A few more thoughts:

1. Move the CSS into admin.css where the other settings CSS live

2. The help text "This format is currently set to display as Fri, 03/02/2007 - 19:25." is slightly misleading. The reason for this is if I update the custom PHP code, then that text is completely useless. I would just remove it all together or write some AJAXy to update that part on the fly as you enter the PHP code ;-)

Otherwise code seems to be good.

ChrisKennedy’s picture

Status: Needs work » Needs review
FileSize
9.89 KB

1. Fixed, thanks.
2. Added an Ajax updater.

ChrisKennedy’s picture

FileSize
9.89 KB

Oops, I had an extra parameter in the Ajax call that didn't do anything.

forngren’s picture

Marked http://drupal.org/node/58708 as dupe of this. I haven't reviewed the patch, but +1 for the idea.

forngren’s picture

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

Patch works.

Steven’s picture

Status: Reviewed & tested by the community » Needs work

We avoid hiding things onready, and use the JS-triggered CSS class approach instead. See the handbook.

Steven’s picture

In fact, that patch needs a rewrite. Code properly, or not at all:

    // Double encode format string so that backslashes are sent properly.
    var url = Drupal.settings.datetime.lookup +"/"+ encodeURIComponent(encodeURIComponent(input.val()));

This sort of code is never, ever acceptable. Besides, there are no problems with backslashes and URIcomponents, only forward slashes. If you are referring to forward slashes, there is nothing in core that says you have to split your arguments by slash. You are free to interpret "my_path/foo/bar" as "my_path" and "foo/bar". This is what search.module does for example.

And, you need to use Drupal.encodeURIComponent() instead.

... datelongchoices ... datetime ...

Variable naming. We use different conventions in PHP and JS to match the environment. Apply them properly.

  $("#edit-date-format-short").change(function() {
    if ($(this).val() == "custom") {
      $("#short-custom-div").show();
    }
    else {
      $("#short-custom-div").hide();
    }
  });

  $("#edit-date-format-medium").change(function() {
    if ($(this).val() == "custom") {
      $("#medium-custom-div").show();
    }
    else {
      $("#medium-custom-div").hide();
    }
  });

  $("#edit-date-format-long").change(function() {
    if ($(this).val() == "custom") {
      $("#long-custom-div").show();
    }
    else {
      $("#long-custom-div").hide();
    }
  });

This can be simplified into a single block by making the id for the form item and the linked show/hide element similar. Also, putting 'div' in the id is useless. You don't name functions "function_foo".

  // Hide the labels to simplify the UI.
  $("#short-custom-div .form-item label").html(" ");
  $("#medium-custom-div .form-item label").html(" ");
  $("#long-custom-div .form-item label").html(" ");

This is sloppy and repeated across the patch.

For one thing, a single selector could be used, regardless of the underlying structure (just comma separate them), but more importantly is that each of those items should have a class meaning "date selector" and "short/medium/long" rather than a single ID that encodes both things. Then you can affect all of them with a single selector.

Also, using html(' ') as a hack for hiding the label while retaining the space it keeps is silly. Use 'visibility: hidden' instead in CSS, that's what it's for. However, I thought we had "container-inline" for exactly this purpose, which used a much more logical underlying markup?

  echo drupal_to_js(array('response' => $result));
// and
      data = Drupal.parseJson(data);

If you're not going to use any of parseJson's abilities re: malformed content and status flags, you might as well use jQuery's built-in JSON parsing.

This is not ready to be committed by far, and if you're not going to do a proper core-quality review of a patch, don't use the 'ready to be committed' status as it will cause bad code to go in.

Steven’s picture

And another trick that can make this patch simpler:

if ($(this).val() == "custom") {
  $("#long-custom-div").show();
}
else {
  $("#long-custom-div").hide();
}

can be written as

$("#long-custom-div")[$(this).val() == "custom" ? 'show' : 'hide']();
ChrisKennedy’s picture

Status: Needs work » Needs review
FileSize
10.7 KB

Thanks for the helpful review Steven. I tried to fix as much as I could but I wasn't sure about a few things.

"We avoid hiding things onready, and use the JS-triggered CSS class approach instead." Originally I was hiding it onready because I wanted it to work if JS was disabled in a css-capable browser, but I have now switched the css to have it as display:none by default and then show the layers in Javascript. Is that what you meant or is there a better way?

Variable naming: my fault about datetime, but datelongchoices/etc. in PHP was already like that. I fixed it in this patch though.

I tried using container-inline but couldn't get it to display correctly. I'm not sure exactly how it would be used.

So this should be more along the lines of RTBC with some possible improvements remaining.

Steven’s picture

  • Having it not work when JS is disabled is not acceptable. The html.js trick is documented in the handbook clearly. Use that.
  • Your jQuery selectors need more optimizing ($('.classname') is the most inefficient query).
  • Wrapping the JSON response in an array with a 'response' key servers no purpose as far as I can tell.
  • Code style (spacing).
ChrisKennedy’s picture

FileSize
10.89 KB

Okay here's another update. The only spacing error I could find/see was the question mark after match().

ChrisKennedy’s picture

FileSize
10.89 KB

Oops, missed a line-break.

RobRoy’s picture

Status: Needs review » Needs work

Not working for me. The response from /lookup is this so it's not parsing.

"Sat, 3/24/2007 - 22:52"

$(document).ready(function() { $("body").append(""); });

And adding that killswitch as inline JS feels weird. Don't those usually work fine at the bottom of the JS file or am I missing something?

With that said, I really like the direction for this patch, just has a few minor kinks it seems.

ChrisKennedy’s picture

Status: Needs work » Needs review
FileSize
10.8 KB

Okay I think I fixed it; there appears to have been a problem when clean URLs is enabled.

This is admin-specific js functionality so the javascript is placed in system.js rather than an individual file like many of the other js scripts. So, the code isn't run automatically from within system.js, as it includes multiple functions, although right now the only other one is the cleanURLs check. That's why the killswitch is placed inline.

ChrisKennedy’s picture

@RobRoy - Actually I think you're experiencing the devel.module Ajax bug that was fairly recently fixed at http://drupal.org/node/123951

Darren Oh’s picture

Chris Kennedy deserves a prize for trying to create patches that fulfill the wish-lists of people who don't bother to contribute patches themselves. Now, let's see some critics contribute patches for the rest of us to criticize.

ChrisKennedy’s picture

FileSize
10.8 KB

Synced post-kitchen sink.

Dries’s picture

That last patch looks clean and elegant to me.

I've tested this patch and it seems to work fine. One thing I noticed though, is that the "Default time zone" setting (on the same page) is now broken. The content of that selection menu is gibberish.

ChrisKennedy’s picture

Status: Needs review » Needs work

Hmm good catch, somehow when the long date format is set to "custom", _system_zonelist's variable_get() also returns 'custom' rather than the actual custom date format as specified in the _submit handler. I'm not sure exactly how to fix this but I might need to tweak the FormAPI data. To be determined....

xmacinfo’s picture

I've created a dupe in http://drupal.org/node/135263. I should have done a Search before.

Gábor Hojtsy’s picture

Wow, so much work went into this patch... I don't want to turn this patch down, but I feel obliged to point on the right way of adding a custom date format page (for developers). We have a small datehu module (in drupal.org CVS) included in the Hungarian locale install profile. The module contains only this one function and an info file accompanied of course:

function datehu_form_alter($form_id, &$form) {
  if ($form_id == 'system_date_time_settings') {
    $hu_formats = array(
      'short' => 'Y. m. d. H.i',
      'medium' => 'Y. F j. H.i',
      'long' => 'Y. F j., l H.i'
    );
    foreach ($hu_formats as $n => $f) {
      $form['date_format_' . $n]['#options'][$f] = format_date(time(), 'custom', $f) . ' (magyar)';
      $form['date_format_' . $n]['#default_value'] = variable_get('date_format_' . $n, $f);
    }
  }
}

That said, it is not required to modify core to add new date formats if you are a developer. Now with this patch applied, anyone can set custom date formats, given the PHP date syntax is known to them. But what is the *actual* requirement driving this new feature? Isn't it that it is not possible to *localize* date formats? Why would every Drupal site admin setting up foreign language sites need to know the proper PHP date codes *and* the gramatically correct date format for their country. At least working with local data, I doubt that Hungarian Drupal site builders know either of the PHP date format markers or the proper grammar for Hungarian date formats (many people are surprised by the date format settings shown above, that we have a dot between the hour and the minute).

Anyway, what about letting people doing the interface translation provide a sane default for that language? Plone does this for a few years now :) http://plone.org/development/teams/i18n/datetime In Drupal lingo they do t('date_format_long') to get the date format in the language (they even have the same name as we use it in Drupal for this value :) I would suggest we do t('!longdate l, F j, Y - H:i', array('!longdate' => '')) or something just to provide a default in case the team does not localise the date format yet, and yet provide a hint at the same time on the meaning of that character mess.

I would envision one new option in all three format dropdowns, which would
- not appear if locale module is not enabled
- would appear with the date format and "(language dependent)" attached otherwise

Alternatively it can replace one option from the current list, so a duplicate display is not disturbing in case of the language has no translation for that option...

If the primary role of this patch is not to allow custom date formats because of *language* differences, then maybe I am offtopic here and should encourage a different issue with a patch solving localized date formats, but I bet my concerns fit in here.

Darren Oh’s picture

Some people were interested in localizing the date format, but I got involved because I needed the option of showing just part of the date. It didn't make sense to include options for every combination of parts of the date, so the custom date format was the perfect solution. The point of this patch is flexibility, not localization.

Gábor Hojtsy’s picture

OK then, we need a different issue for localization. Seems like I misunderstood where it got from the localization motivated first patch.

Gábor Hojtsy’s picture

OK, opened a new issue here with localized date formats: http://drupal.org/node/137574

ChrisKennedy’s picture

It will probably be a few more weeks before I can personally work on this patch again, but I will just reiterate that this is not inherently a localization issue.

The essential issue is that there are an infinite number of possible date format strings, so we need some sort of functionality that allows them to be completely customizable. Yes, it could easily be done in contrib on a manual or customizable basis, but this is a simple feature that needs to eventually be in core.

Using the date format syntax is something of a necessary hassle - Darren Oh's change in #6 to include links to the PHP manual helps in this regard. There are other usability improvements that could be made regarding token substitution if anyone is interested.

ChrisKennedy’s picture

Status: Needs work » Needs review
FileSize
11.38 KB

Synced and fixed the timezone bug.

ChrisKennedy’s picture

FileSize
11.23 KB

Hmm, for some reason I now need to manually set the _submit handler, and it removes the need to modify _system_zonelist(). Perhaps due to fapi changes?

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

Works great. Thanks for all the work, Chris.

ChrisKennedy’s picture

Is there anything left to do on this?

Dries’s picture

- Maybe an entry in CHANGELOG.txt? It probably belongs in the usability section of the CHANGELOG.txt.

- Maybe group the settings using fieldsets? One fieldset for locale (timezone + first day of week) and one fieldset for the date format? Not sure about this one but the page fields a bit unstructured. Admittedly, this is not related to your patch. However, consider this to be an invitation to come up with more subtle usability improvements for this page.

- It's been a while since I revisted this page, and for a split second, I was confused by 'Configurable time zones'. I'd suggest renaming this to 'User-configurable time zones'.

- When I configure a custom date format I got the following notice: Array to string conversion in includes/form.inc on line 606. Might be because of the recent formapi 3 patch.

- The vertical spacing between the different form elements is slightly off (the date format settings have more whitespace).

- The 'default timezones' looked broken (after submitting them).

Otherwise, this patch is RTBC. If you could do one more re-roll, then this could go straight in, IMO.

Dries’s picture

I accidentically committed this patch when I tried to commit another patch. I decide not to roll-back the patch as it was basically sane. However, I do apologize for not given proper credit. I'll make sure to get this right, when we commit a follow-up patch.

BioALIEN’s picture

Status: Reviewed & tested by the community » Needs work

Changing status to reflect Dries's comments.

ChrisKennedy’s picture

Status: Needs work » Needs review
FileSize
6.48 KB

Okay here's a follow-up:

1. Grouped into fieldsets ("Locale settings" and "Formatting").
2. "User-configurable time zones"
3. Fixed vertical spacing
4. Fixed FormAPI error
5. Tweaked the timezone description a tad.
6. Fixed an IE6 CSS bug.

xmacinfo’s picture

Tested todays tarball including the accidental commit by Dries.

The new date customization is better than I expected. Great work, Chris.

Dries’s picture

Status: Needs review » Fixed

Great job, Chris. Committed. :)

Anonymous’s picture

Status: Fixed » Closed (fixed)