Timezone handling

There still is probably timezone logic without the Date module port.

A user can their own timezone so presumably we would want to convert all times they see into there own timezone.

Leaving as postponed because there is still a lot of other things to fix.

I think the time has come to pick up this ticket.

Date field handlers actually allow for overriding the user/site timezone, we need to keep this in mind when fetching the timezone.

So at the moment we can find commented out code supporting timezone handling across our codebase. We want to figure out how we can simplify this. A good start would be mapping out in which parts of the codebase timezones are being mentioned right now. So let's try that:

• calendar.theme.inc
• Drupal\calendar\CalendarHelper
• Drupal\calendar\Plugin\views\row\Calendar
• Drupal\calendar\Plugin\views\style\Calendar

We also have a getters and setters for the timezone in CalendarEvent and CalendarDateInfo but there's no logic going on there.

I'm not sure if this is the same issue, but I'm working on a site that needs a calendar and there's something goofy with when an event appears on the calendar.

* Using D8.3.7.
* Configured my site's default timezone to America/NewYork
* Created an Event content type with a date/time format
* Added an event for November 3 @ 10 PM
* The event shows on the calendar, but for the following day (screenshot below)

It seems like the calendar display is using UTC time to determine the date which it's displayed on. Thoughts?

Yup, that's what we'll try to fix here. Sorry, i don't think there's a good way around this for now.

I was having the same issue described in #12, and the patch described here to /src/Plugin/views/row/Calendar.php fixed it: https://www.drupal.org/project/calendar/issues/2895000

This seems like a good start, this patch is based of #2895000: Calender::render doesn't correct for local timezone so @bkat should receive credit for this.
This patch is not based of the dev branch, it works on #2699477: Steps towards handling end dates in Calendar 8 so you should apply that patch before this one.

This works for my use case and seems to work for some other people also. However I think we need some additional testing and cleanup before we can actually merge this one.

DateTimeItemInterface::STORAGE_TIMEZONE was only introduced in 8.5.x, so here's a patch that also supports 8.4.x

I'm unable to apply either one of these patches. Looking through the code of the latest dev release, it looks like a lot of what is being added/removed is not there. What version of calendar 8x is everyone working with?

I was able to get this working on a fresh install of

• 8.5.0
• calendar latest dev (4 Jan 2018 at 14:58 UTC)
• https://www.drupal.org/project/calendar/issues/2699477#comment-12406273
• https://www.drupal.org/files/issues/2604546-16.patch

Would really prefer to not be depending on other issues, though I understand that sometimes it's better to solve things in tandem.

That most recent patch https://www.drupal.org/files/issues/2604546-16.patch didn't apply for, but with some minor adjustments I was able to get it working.

I re-rolled the patch: https://www.drupal.org/files/issues/2018-03-20/2604546-22.patch

Applies on:

* Drupal core 8.5.0
* Calendar module latest dev at commit 73e2979f3ed951b1fb3ad942e2d89d673aa52e1d

Edit, name that file badly.. re-uploading as 2604546-22.patch

Sorry for the pain and repeated post.. trying to get the filename to match the comment. This comment should do it.

That most recent patch https://www.drupal.org/files/issues/2604546-16.patch didn't apply for, but with some minor adjustments I was able to get it working.

I re-rolled the patch: https://www.drupal.org/files/issues/2018-03-20/2604546-26.patch

Applies on:

* Drupal core 8.5.0
* Calendar module latest dev at commit 73e2979f3ed951b1fb3ad942e2d89d673aa52e1d

Looks Good. I just checked that against a Drupal 8.5.0 site with the latest Calendar Dev. My events are showing up on the correct days.

The #26 didn't apply for me in Drupal 8.5.
The #16 apply but don't solve the problem in Drupal 8.5.

@adriancid: Did you apply the patch to the "4 Jan 2018" Dev release of Calendar?

What time zone are you using?

I am using PST (Pacific Standard US/Canada) time zone with success of #26 patch.

@awasson Well I don't know why the patch does not apply when I try to add it with composer, but if I try cloning the last dev version it works.

In the other hand this patch have a conflict with the latest patch in #2699477: Steps towards handling end dates in Calendar 8

@adriancid, ok. I see how that may be a problem. Both patches affect the file /src/Plugin/views/row/Calendar.php

I haven't been following that issue but at this point if it were me, I would probably re-roll this patch #26 to work with a copy of the dev calendar module after applying the latest patch at #2699477: Steps towards handling end dates in Calendar 8. That would at least allow me to assess the fitness of it.

I've been using GIT to patch all of mine rather than Composer.

I'm not sure that I've provided anything useful but hopefully it has.

Cheers,
Andrew

I had to apply #26 on top of #2699477-71: Steps towards handling end dates in Calendar 8. Here is a patch for that, it might be useful if that goes in first.

I can confirm that when using Drupal core 8.5.3 and latest Calendar, #26 does not install with #2699477-71. #33 installs but no change. This timezone issue is driving me bananas.

I can confirm that patch #26 works on Drupal 8.5.6

I can also confirm patch #26 works on Drupal 8.5.6. Nice work!

Note: be sure to clear cache after you update the source.

Re-apply on latest patch in #2699477: Steps towards handling end dates in Calendar 8

I can confirm that #26 does work correctly against the latest Calendar 8.x-1.x-dev (2018-Jul-05)

Patch #37 DOES NOT RUN against the latest DEV.

The Error is as follows:

error: while searching for:
      $granularity = 'month';
      $increment = 1;

      // @todo implement timezone support
      if ($entity->hasField($field_name) && $entity->get($field_name)) {
        $item = $entity->get($field_name)->getValue();
        // @todo handle timezones
//        $db_tz   = date_get_timezone_db($tz_handling, isset($item->$tz_field) ? $item->$tz_field : timezone_name_get($dateInfo->getTimezone()));
//        $to_zone = date_get_timezone($tz_handling, isset($item->$tz_field) ? $item->$tz_field : timezone_name_get($dateInfo->getTimezone()));
//        $item_start_date = new dateObject($item, $db_tz);
        $delta = $row->{$delta_field};
        $item_start_date = new \DateTime($item[$delta]['value']);

        if (!empty($item[$delta]['end_value'])) {
          $item_end_date = new \DateTime($item[$delta]['end_value']);
        }
        else {
          $item_end_date = $item_start_date;

error: patch failed: src/Plugin/views/row/Calendar.php:425
error: src/Plugin/views/row/Calendar.php: patch does not apply

Moving forward, is there any reason Patch #26 can't be marked as tested by the community so that we can move past this?

Similar to #2844969: [PP-1] Use core's timezone handling for datetime fields when it becomes available: #2632040: [PP-1] Add ability to select a timezone for datetime field

I can confirm the approach at #33 works with Drupal 8.6.2 and latest Calendar 8.x-1.x-dev (2018-Jul-05).

If Calendar is already patched with #2699477-71: Steps towards handling end dates in Calendar 8, patch #33 applies cleanly and timezone handling is working.

[edit for spelling]

I can also confirm the same as @waverate in #40.

@aklump and @waverate was curious if the order in which these patches need to be applied is as follows:

First:
https://www.drupal.org/comment/12561037#comment-12561037

Then:
https://www.drupal.org/files/issues/2018-05-09/2604546-33.patch

@TikaL13: correct

Had an issue with undefined index notices on the week display.
Found an issue in the theming function that doesn't seem to take timezones in account when creating array keys.
Attached patch bases on.

1. Calendar 8.x-1.x
2. Patch with: https://www.drupal.org/comment/12561037#comment-12561037
3. Patch with patch from comment 45: #2604546-45: Timezone handling

Please use patch from comment 45: #2604546-45: Timezone handling

Damn, something went south wen generating the patch for #44. Re-created.

Again - attached patch bases on.

1. Calendar 8.x-1.x
2. Patch with: https://www.drupal.org/comment/12561037#comment-12561037 (Comment #71)
3. Patch with attached patch: #2604546-45: Timezone handling

@KarlShea I haven't used your patches as of your comment in #2699477-92: Steps towards handling end dates in Calendar 8

I have to roll this all back for a client site, it's just so incredibly broken. Every fix breaks something else.

I can confirm that patch #26 (only!) works on Drupal 8.6.12

#45 works for me but the calendar month view is still displaying wrong, which it looks like it's displaying the events back in the UTC timezone.
Attached is the patch to correct this.

I can also confirm that #26 does work correctly against the latest Calendar 8.x-1.x-dev (2018-Jul-05) and fixes my timezone problems.

I did not try the patches after #26.

But I have one small problem: #26 (also #45) removed the usage of BaseFieldDefinition:
-use Drupal\Core\Field\BaseFieldDefinition;
I do not understand why this is done - BaseFieldDefinition is used in function render():

        if ($field_definition instanceof BaseFieldDefinition) {
          $storage_format = 'U';
        }

If we do not use Drupal\Core\Field\BaseFieldDefinition this check will never be true (I think).

Added patch is the same as #26 just without removing the usage line.
It does not contain any changes proposed after #26, especially not the one adressed in #47 (I did not experience the problem metioned by mmaranao).

I can confirm that using just patch #26 works on Drupal 8.7.8 with latest calender dev.

I'm still experiencing the issue on a fresh Drupal 8.7.8 install.

Steps to reproduce:
- Apply patch #26.
- Set site's Timezone GMT-5 Ecuador or Eastern time (UTC).
- Create a date field on a contentype DATE ONLY (dont select date and time!).
- Create a Calendar view based on calendar template and make sure you select that created date field.
- After 7PM GMT-5 time the calendar view shows the nodes a day before the date is set, meaning if i have a node with date 2019-11-18 it will show on the 2019-11-17 box.

Has anyone been able to get patch 26 to install with composer. I have been trying to do things the new composer way lately but I cant get the patch to install. It applies with git cleanly.

@bloomt

What do you have in your composer.json? What is the error message?

Are you using "cweagans/composer-patches"

Do you have something like this in the extra section?

"extra": {
...
        "patches": {
            "drupal/calendar": {
                "596929694 https://www.drupal.org/project/calendar/issues/2604546": "https://www.drupal.org/files/issues/2019-06-22/calendar-timezone-2604546-48.patch"
            }
        }
    }

Looks like this was committed to 8.x-1.x dev branch in this commit.

@philltran and all other contributors: yes, this was added to the combined patch from Issue #3079917 which was committed two weeks ago. But since there were multiple issues involved in this I forgot to revisit this thread. Also, I'm a new maintainer dealing with a large backlog...

So I think we can finally close this issue as fixed. New issues deserve their own threads. Also I think it's important to keep up with Issue #2632040 since this deals with better handling of timezone for datetime fields in core in general.

Posted at #3079917: release? but reposting here for visibility:

For future reference, this kind of "combined patch" is not really a best practice. It confuses 2 of our most important tools: Git and Drupal.org issue queues. Now, when we're trying to understand the history of a given line of code and use 'git blame', we see a bunch of stuff intertangled in a single commit. Then we have to somehow try to figure out which *actual* issue that line of code came from, then read the comments there to understand why it was added.

If you have 4 distinct issues, please commit each patch separately, where each commit points to the issue that patch came from. Then it's much easier for everyone in the future (yourself included) to make sense of what changed and why.

Thanks!
-Derek

@solide-echt Thanks for taking over maintaining this module and committing patches!

Re: #57: Yes, that, too! ;) Sorry if my comment comes across poorly. I am thankful someone is maintaining this project again. Only trying to be supportive of best practices for everyone's benefit. :)

Cheers,
-Derek

Derek, no apologies necessary, you're absolutely right.
The combined patch was made almost half a year ago. Most of my projects are single person where the granularity of commits is not all that important.
For me Drupal is a way of learning something new everyday, most people my age refer to sudoku's to entertain their brains ;-)

Closing this issue as fixed.

@solide-echt I think it would be appropriate to acknowledge contributors to the combined patch by issuing retroactive credit.

Please see the section in Commit messages - providing history and credit: Providing Retroactive Credit

For example to date the following would be committed:

git commit --allow-empty -m 'Issue #2604546 by thursday_bw, das-peter, geertvd, gngn, mmaranao, plach, KarlShea, robshambaugh, bkat: Timezone handling' --author="gngn <gngn@1074656.no-reply.drupal.org>"

Notice the --allow-empty flag.

This commit would show up in Git history, and everyone will get credit on Drupal.org properly. Git itself will not record any changes to any files in the repo.

Drupal Core itself has done this many times.

But I encourage you to go through this issue and other issues and identify any users not present in this list.

Apologies, my understanding was the credits are allocated automatically based on the Credit & committing section at the bottom of the issue. But it perfectly makes sense this is only done when the commit involves the issue number.

Note: I specifically am *NOT* talking about issue credit. Project maintainers can absolutely toggle credit on an issue via the checkboxes at the bottom of the form. It doesn't matter what the Git commit messages say on that level. You can credit people who don't show up in the commit message.

What I *AM* talking about is 'git blame' to understand a line of code. Empty commit messages to give "credit" don't help that at all. You still end up with git blame on hundreds of lines of (unrelated) code all pointing to the same message, either:

"Issue #3079917 by solide-echt, paul_ch, gngn, daniel.matcau, yepa et al: Combined patch"
or
"Multiple issues, see release notes for 8.x-1.0-alpha2"

I see a handful of other commits have been pushed since then, so it's getting harder to untangle this. Probably we'll just live with it and move on. But If you really wanted to help future developers understand all this, you could have reverted the above 2 commits, and then separately committed each patch from each issue, where the message made sense and pointed to the right issue for every commit. Then, in the future, if you tried to 'git blame' something, you'd see the correct one of these:

Issue #2604546 ... Timezone handling
Issue #2730747 ... Custom Entity not displayed
Issue #2959610 ... There is no way to bind a calendar View for your own field
Issue #3034293 ... Multi-day event shows in one column only
...
Not just "Issue #3079917 ... Combined patch"

/shrug

Cheers,
-Derek

Agreed and hope to proceed accordingly from alpha2 onwards. However, to my defence: adding patches individually wouldn't have worked since they had multiple conflicts between them, were sometimes already dependent on other patches and used different commits to start from. Getting all of the patches working together in the combined patch took quite some effort...

So the "royal" way forward would probably have been to rewrite all the patches against the commit from July 5th 2018 and then commit them individually.

Eric

Indeed, that's a bit of a mess. Too much trouble to have to untangle all yourself. Hopefully now that things are moving again, it'll be easier to keep individual patches from conflicting as much and to get things committed in a more granular fashion.

Speaking of best practices: you're familiar with the [#x] notation for referencing issues on d.o, right?

[#2632040] becomes:
#2632040: [PP-1] Add ability to select a timezone for datetime field

Very handy, and more useful than just #2632040.

There's a new ticket that seems to be related to this ticket caused by the recent commit.