Problem/Motivation

I suppose there is a bug about time zone handling with the Publication plugin. The dates are saved as UTC (DateTimeItemInterface::STORAGE_TIMEZONE) in the DB. But processSchedulerQuery() use a plain new DrupalDateTime() for comparing start and end dates. This is using the timezone from the current user which can be different than UTC. This is especially reproducible if using a datetime field (with this patch: #3184191: scheduler_field_type_publication not working if scheduled on same date) and not a date only field. But should happen for date only field as well in being to early or too late on publishing depening on given timezone.

Steps to reproduce

  • Use a timezone different from UTC
  • Use scheduler_field publication plugin on an entity
  • Set a schedule for the publication
  • Watch the publication being to late or too early with the offset of timezone used

Proposed resolution

Use UTC time zone for comparing, since database is also using UTC time.

Remaining tasks

create a patch

User interface changes

none

API changes

none

Data model changes

none

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

stefan.korn created an issue. See original summary.

stefan.korn’s picture

Assigned: stefan.korn » Unassigned
Status: Active » Needs review
StatusFileSize
new1.12 KB
stefan.korn’s picture

This needs some changes in the process-callback too, see this patch

interx’s picture

Thanks, exactly what I needed.

Some remarks:

  • The patch changed the fielditem names from value and value end to start_date and end_date. That looks incorrect?
  • In the field item value you can get a string or a null value. The code compares this value directly to a DrupalDateTime object, this seems dangerous with $date_end < $current_date as $date_end can be NULL and the comparison of NULL < NOW will always be true.

You removed the loading of the dates as DrupalDateTime objects. I think it would be better to keep that when the value is not null, e.g.

DrupalDateTime::createFromFormat(DateTimeItemInterface::DATETIME_STORAGE_FORMAT, $start_date, DateTimeItemInterface::STORAGE_TIMEZONE);

Wouldn't it be easier in the conditions checks to see if date_begin or date_end are null first, in that case nothing needs to happen?

stefan.korn’s picture

@interX: Thanks for looking into this.

the scheduler_field typ is extending from Core DateRangeItem. DateRangeItem is providing start_date and end_date computed properties which return a DrupalDateTime. So there is no need for loading this again with DrupalDateTime::createFromFormat() from the field values.

One could check for date_begin or date_end being NULL, but as far I can tell an entity with empty date_begin (for publishing) or empty date_end (for unpublishing) should never reach the process callback, because they would not get passed through processSchedulerQuery callback.
That said, there might be an edge case if someone removes the date while the entity has not yet been processed. Or otherwise if something went wrong. So for this case it might make sense to check for empty dates here too.

interx’s picture

Apologies, you are right, this is indeed correct, ignore my earlier comment.

I tested the patch and it seems to work fine!

interx’s picture

Status: Needs review » Reviewed & tested by the community
giuseppe87’s picture

From my test, the patch doesn't work when in case of a paragraph which needs to be unpublished, if the cron run within 1 hour after the end-date publication time of the paragraph.

I've updated the related issue to consider time-zone, so I think this one could be considered a duplicate of that?

giuseppe87’s picture

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

goz’s picture

Status: Needs work » Reviewed & tested by the community

Commited on duplicate #3184191. Can you confirm the commit solve your issue @stefan.korn ?

stefan.korn’s picture

@goz: somewhat late reply, but I can confirm that this issue is now fixed in HEAD.

Still I would propose some small code changes like described in #5. Basically using the computed properties for getting the start and end date instead of creating them (again) from value

the scheduler_field type is extending from Core DateRangeItem. DateRangeItem is providing start_date and end_date computed properties which return a DrupalDateTime. So there is no need for loading this again with DrupalDateTime::createFromFormat() from the field values.

Putting these and some other small changes in the MR for this issue.

stefan.korn’s picture

Status: Reviewed & tested by the community » Needs review

  • goz committed 161dd689 on 1.0.x
    Issue #3250384: Add timezone handling tests for Publication plugin
    
    -...

  • goz committed 39d43818 on 1.0.x authored by stefan.korn
    Issue #3250384 by stefan.korn: Scheduler field plugin Publication -...
goz’s picture

Status: Needs review » Fixed

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

goz’s picture

Issue tags: +Vienna2025

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.