Date fields can show timezone twice in custom formats - hardcoded in theme

[indigoxela]

Description of the bug
When using a custom format for a date (core module) field, which includes a timezone name, the resulting format gets pretty weird.

Steps To Reproduce
To reproduce the behavior:

1. Add a date field to a content type (with or without end date doesn't matter)
2. Add a custom date format including the timezone like e:Ymd\THis
3. Use that format in your content type's display settings

Actual behavior
The date for my example shows as: Europe/Vienna:20190328T000000 Europe/Vienna
(Note the timezone added twice)

Expected behavior
I'd expect my custom date format to be respected as-is: Europe/Vienna:20190328T000000

Additional information

• Backdrop CMS version: 1.12.3
• Web server and its version: Apache 2.4, PHP 7.0
• Operating System and its version: Debian stretch

The actual problem happens in date.theme.inc.
Path: core/modules/date/date.theme.inc

    320   // If timezone is already included in format, suppress it from display
    321   if ($format == 'Y-m-d\TH:iO') {$timezone = '';}

and later on...

    326   // Wrap the result with the attributes.
    327   $output = '<span class="date-display-single"' . backdrop_attributes($attributes) . '>' . $date . $timezone . '</span>';

It is assumed, that only the exact format 'Y-m-d\TH:iO' can contain a timezone (line 321).
Then the timezone is added, regardless of a custom format (line 327).

Suggestions for a solution:

1. Do not auto-add timezones at all, as this bypasses custom format settings.
2. if you really need to add it for some reason, be more flexible about the pattern.

---

PR backdrop/backdrop#2555

[klonos]

Interesting...

I am also inclined to say that we should not be appending timezone to date "blindly" or based on assumptions. Having said that though, I would like to see if there is a "smart" way to regex timezone out of the provided text format string. Unfortunately, I most likely won't have time to work on this till next weekend (unless anyone else beats me to it by then).

Thanks for taking the time to report this @indigoxela 👍

[indigoxela]

An interesting question is: why is the timezone added anyway?

Git blame of that file doesn't give me a clue. Is this something, that has been requested, or is there a technical need for it?

[indigoxela]

It's funny, this kind of behavior (timezone added to date) is around for a very long time, since Drupal 6

So there probably really is a need for it.

But it's easy to prevent double timezones.

  // If timezone is already included in date, suppress it from display.
  if (strpos($date, trim($timezone)) !== false) {
    $timezone = '';
  }

Above code prevents double timezones in single dates, date combos possibly need some attention too.

@klonos next weekend sounds like a good plan, no need to stress yourself out.

[indigoxela]

I'm trying to figure out the logic behind that timezone handling.

A possible reason, why it was initially added could be, that date fields can have a timezone set (user/date/site), but there's no conversion when showing them (like core dates have). So the idea behind that automatic adding was possibly to clarify the timezone (hence offset).

Not sure...
For quite a while Drupal had configurable time formats and these can of course contain timezone info.

Double timezone (or offset) info can never be useful and is probably just a side effect.

So my suggestion from my previous comment could be a valid solution, if removing the timezone completely is not possible.

[indigoxela]

Actually this issue here is a follow up of an older issue (#1097).

A more generic solution, which works for me:

  // Remove escaped (literal) characters from format and search for
  // format characters containing timezone or offset information.
  // If already present, don't add it again.
  $tz_offset_chars = array('e', 'O', 'P', 'T', 'Z', 'c', 'r');
  $literals_stripped = preg_replace('/\\\./', "", $format);
  if ($literals_stripped !== str_replace($tz_offset_chars, '', $literals_stripped)) {
    $timezone = '';
  }

Is this the proper approach? Worth a pull request?

[klonos]

Thanks @indigoxela 👍 ...that approach seems good to me, but seems that it could get false positives. And yes, I would try a PR.

By the way, shouldn't I be in the $tz_offset_chars list?

I (capital i) | Whether or not the date is in daylight saving time

[klonos]

...did some googling around, and I came across this, which might be worth taking a look at if we want to avoid using a regex: https://stackoverflow.com/questions/21485281/find-out-if-a-date-time-string-contains-a-timezone

You haven't necessarily lost the information as to whether or not there was a timezone in the date/time string once you have created your DateTime object. If you do a var_dump() on a DateTime object, you will get output like this:

object(DateTime)[1]
  public 'date' => string '2014-01-13 04:22:00' (length=19)
  public 'timezone_type' => int 1
  public 'timezone' => string '-08:00' (length=6)

As you can see, the timezone type and the value of the timezone passed are both there, it is just a matter of fishing them out.

In your case I think that you just need to know if a TZ string was passed in the date/time string. If it was, then the timezone type will be 1, if no string was present, then the default system timezone will have been used and will be of type 3.

You cannot get the timezone type directly, but the following function will get it for you and you can then act accordingly.

function getTZType(\DateTime $date)
{
    ob_start();
    var_export($date);
    $dateString = ob_get_clean();
    preg_match('%=>.\d%', $dateString, $matches);
    return (int)explode(' ', $matches[0])[1];
}

$date = new \DateTime('2014-01-13 04:22 -8');
echo getTZType($date);

The function returns an int 1,2 or 3 that corresponds to the time zone type explained in my answer here.

See it working.

This method will allow you to avoid using regex on date/time strings, the regex that is used is on a string with a known and reliable format.

The solution seems to work for php 5.4 up to 7, but fails in 5.3 (due to short array format used).

[klonos]

...this also seems helpful: https://blog.serverdensity.com/handling-timezone-conversion-with-php-datetime

[klonos]

Seems that whatever solution we figure would be best would need some testing with random/multilingual strings use in custom dates, to see if it works in those cases. So, not as easy a task as it seems ...but not impossible either 😄

[indigoxela]

@klonos many thanks for your feedback.

Thinking about your remarks and searching through the module code - it seems to me, we tried to fix the problem at the wrong place.

function date_formatter_process() and function _get_custom_date_format() in file date.module seem promising. We have more info about the field settings there, than we have in the theme function.

I was wondering, why some formats already had empty timezone info. When playing around with custom formats, it wasn't always predictable, what one gets.
It happens in function date_formatter_process():
"U", "r" and "c" as full formats get filtered out.
Custom formats always seem to get a $timezone variable set.

And yes, I agree, we need some testing for that, as soon as we fully understand the logic. ;)

[klonos]

...as soon as we fully understand the logic. ;)

Yep, indeed. That's why Date is one of those "beastly" modules 😅

[indigoxela]

I have to correct myself in two points:

1. date.theme.inc is the right place for bugfixes, the code in date.module is correct
2. we shouldn't wait until we "fully understand the logic" - I'm afraid, this won't happen, at least not on my side. ;)

For theme_date_display_single() I still favor my approach with $tz_offset_chars (slightly revised). I didn't have much luck with your example. Btw. we already have the timezone_type available in BackdropDateTime, but it's not helpful.

For theme_date_display_combination() I need a little more time - it really has some bugs and is hard to understand.

[klonos]

Thanks @indigoxela for your time and effort on this 👍 ...I am currently focused on other tasks (namely #495), but this one is one of my favorite head-scratchers, so will definitely come back to it. If in the meantime you have time to draft a PR, I'd be more than happy to review.

[indigoxela]

Spent some more time on the date module...

We're probably lucky, this never worked:
(date.theme.inc, function theme_date_display_combination)

  if ($timezone) {
    $timezone = ' ' . $timezone;
  }
  $date1 = str_replace($timezone, '', $date1);
  $date2 = str_replace($timezone, '', $date2);

I suppose, it was meant to strip timezones (didn't work because of the space added two lines above).

Update regarding timezone (I removed parts of my comment): the timezone granularity exists and works correctly. I was fooled by something unrelated to date module (see: #3598)

[indigoxela]

Before I even start a PR - it needs more changes than I thought:

indigoxela/backdrop@691701f

Yes, I really removed the "$timezone" from date_display_single().
This seems radical, but there's no other way to prevent date themes from totally messing up custom date formats.

I kept $timezone adding/moving for ranges, but for less cases because:

1. $timezone can be a timezone identifier, but also an offset (or any combination of "TOZPe", whatever date_limit_format() pulls out). The results are pretty unreliable then.
2. Start and end in the same date field can have different offsets, as one can be inside DST, while the other isn't.
3. Offset (if using "Z") can result in too greedy str_replace(), because it might also fit any other part of the date.

Feedback is welcome.

[indigoxela]

@klonos you're maybe pretty busy, but just in case you missed my previous comment

Did you find the time yet to inspect the changes that I consider necessary?

[herbdool]

I took a quick look at the code @indigoxela. It is more involved but might end up being ok. I recommend making a PR and noting that it might still need more work. And let the automated tests run. There's a date timezone test so that might catch any problems.

[indigoxela]

@herbdool many thanks for taking a look.

The pull request already exists: backdrop/backdrop#2555
Tests are passing.

Yes, it might need more work. Any hint is welcome.

[herbdool]

@indigoxela I don't have specific changes at this time. Just looked again and see you've added a test which is great and it's passing.

[herbdool]

@klonos do you feel like testing this one?

[indigoxela]

It seems, no one is eager to test/review my pull request. ;)

Maybe some explanation will help:

While trying to understand the date format handling I had trouble with, it turned out that the code was actually written for Drupal 6. Hence it doesn't make much sense with Drupal 7, even less with Backdrop.

Additionally I figured out, that it doesn't work as intended because of a faulty str_replace (see comment #3575 (comment)).

So I really don't think, my changes will break anything.

[herbdool]

I will try to test this in the next few days. Thanks @indigoxela

[herbdool]

I've checked the code and the automated test. The test looks thorough and passes. Thanks @indigoxela! It's complex stuff so I might be missing something but looks like you've covered the bases. RTBC from me.

[quicksketch]

Looks good to me too. Merged backdrop/backdrop#2555 into 1.x and 1.12.x.

And a big WELCOME to @indigoxela!! This looks like it was your first issue for Backdrop, and what a doozy! Thanks so much!