Problem/Motivation

The DrupalDateTime constructor takes $time as a parameter. It's defined as follows:

   * @param string $time
   *   A DateTime object, a date/input_time_adjusted string, a unix timestamp.
   *   Defaults to 'now'.

Inside the constructor it calls its parent, which goes to the DateTimePlus constructor. That too takes a $time parameter. However, it's defined more strict:

   * @param string $time
   *   (optional) A date/time string. Defaults to 'now'.

So when creating a DrupalDateTime object with a \DateTime (which should be fine according to the docs), you get the following warning:

Warning: date_parse() expects parameter 1 to be string, object given in Drupal\Component\Datetime\DateTimePlus->__construct() (line 257 of /var/www/html/core/lib/Drupal/Component/Datetime/DateTimePlus.php).

Proposed resolution

Update DrupalDateTime constructor to accept \DateTime, strings, and timestamps, per the documentation.

Add test coverage for the constructor.

Remaining tasks

Update constructor.

Add test coverage.

User interface changes

None.

API changes

None, the docblock had the API described but the implementation was incorrect.

Data model changes

None.

Why this should be an RC target

From #6:

Adding rc target triage. This isn't Critical or Major, but it does identify some test holes and may lead to some followup issues that add missing test coverage.

CommentFileSizeAuthor
#21 drupaldatetime-2596043-21.patch2.4 KBmpdonadio
#11 2596043_11.png131.74 KBAnonymous (not verified)
#10 interdiff-03-10.txt2.61 KBmpdonadio
#10 2596043-10.patch2.89 KBmpdonadio
#4 2596043-03.patch2.71 KBmpdonadio
#4 2596043-test-only.patch1.5 KBmpdonadio
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented

pjonckiere created an issue. See original summary.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented

DateHelper does this a couple of times, in DateHelper::daysInYear() for example, thus making it unusable. This is not reported by our automated tests, so we are missing coverage for both DateTimePlus and DateHelper.
I think the DateHelper issues deserve their own tickets, but I thought that it might be useful to know before deciding on what to do here.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Assigned: Unassigned » mpdonadio
Issue tags: +Needs tests

Working on something...

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Assigned: mpdonadio » Unassigned
Status: Active » Needs review
FileSize
2596043-test-only.patch1.5 KB
2596043-03.patch2.71 KB

There are some subtleties with the way the PHP \DateTime object works with timezones, so this needs careful examination.

The last submitted patch, 4: 2596043-test-only.patch, failed testing.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Issue tags: -Needs tests +rc target triage

Adding rc target triage. This isn't Critical or Major, but it does identify some test holes and may lead to some followup issues that add missing test coverage.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Issue summary: View changes
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented
Status: Needs review » Needs work

I manually checked my use case again, and this fixes my problem. Also the test coverage seems solid for the issue at hand, so that's great as well. Thanks!

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
@@ -60,7 +60,16 @@ public function __construct($time = 'now', $timezone = NULL, $settings = array()
+      parent::__construct('@' . $time, NULL, $settings);

Is there a reason in particular why you don't pass $timezone here? According to the test it works as expected, but I'm not really sure why. If it doesn't make sense to use a timestamp/timezone combination, I think it should be documented somewhere. It might be out of scope here though.

+++ b/core/modules/system/src/Tests/Datetime/DrupalDateTimeTest.php
@@ -121,4 +122,25 @@ function testTimezoneFormat() {
+    debug($datetime->getTimestamp());

This will need to be removed.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented

I'll fix #8 tonight.

Re the first comment, this was the subtleties of DateTime::__construct:

The $timezone parameter and the current timezone are ignored when the $time parameter either is a UNIX timestamp (e.g. @946684800) or specifies a timezone (e.g. 2010-01-28T15:00:00+02:00).

The definition of UNIX timestamps are seconds since 1 January 1970 UTC, so the timezone is implicit.

That is also why the format strings exclude the timezone upon creation and use the $timezone parameter.

I am not 100% convinced this is the best long term solution; there may a better way to handle the timezone, but that would entail messing with the way DateTimePlus works (but thankfully I think the test coverage on that is pretty good).

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Status: Needs work » Needs review
FileSize
2596043-10.patch2.89 KB
interdiff-03-10.txt2.61 KB

I kinda of retract my statements in #9. I think this patch is better, but I want a second set of eyes to look at this to make sure we aren't masking a timezone handling problem somewhere else in the code.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented
Issue summary: View changes
FileSize
2596043_11.png131.74 KB

The options aren't endless, so I just tried them all. That would be 'now', a string, a \DateTime() and a timestamp, and that always combined with no timezone, a string or a \DateTimeZone. See the results in the screenshot attached.
Europe/Brussels is my default timezone, which is +1:00 and Australia/Perth is +8:00. So I think this result is what we expect. When nothing is defined it chooses my default timezone, and when something is defined it chooses the timezone as specified. The one exception being the timestamp with no declaration of a timezone, which implicitly is UTC.

The only thing I do want to point out, is that currently every DrupalDateTime has a timezone with a readable name, except when defining it as a timestamp without timezone. I don't think it would have any technical consequences since the getTimeZone() will give back the "+00:00" one, but it might be good to explicitly define something to have more consistency in the rendering of DrupalDateTime objects. I guess that's more of a choice in design.

Edit: removed the embedded image since it came out a bit large

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented

#11, so do you think this is RTBC? What you are describing with timestamps sound more like a followup and not a fix for the bug that was identified?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented
Status: Needs review » Reviewed & tested by the community

Oh yes, indeed. Looks good to me!

The last submitted patch, 4: 2596043-test-only.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2596043-10.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented
Status: Needs work » Reviewed & tested by the community

Seems like that was a random test failure.

The last submitted patch, 4: 2596043-test-only.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes

Adding the proposed reason to make this an RC target to the summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Title: DrupalDateTime constructor throws a warning when using a \DateTime object » DrupalDateTime constructor claims to support \DateTime object - it should not
Issue tags: -rc target triage +rc eligible

Discussed with @catch, @xjm, @effulgentsia and we think that we should just change the documentation on the constructor to match the parent class since it already has the ::createFromDateTime() method. Therefore this just becomes a docs fix and is rc eligible

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

For #19

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Status: Needs work » Needs review
FileSize
drupaldatetime-2596043-21.patch2.4 KB

OK, here is a doc-only fix.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Status: Needs review » Needs work

REDACTED

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Status: Needs work » Needs review

Time to put away the computer for the day. Ignore #22. This can be reviewed.

The last submitted patch, 4: 2596043-test-only.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented
Status: Needs review » Reviewed & tested by the community

I double checked that the DateHelper methods work with DrupalDateTime, and they do. This will at least take away possible confusion, so let's get it in.

I do think we should add the fix from #10 in a later version.

The last submitted patch, 4: 2596043-test-only.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

I'm not convinced that #10 will really be necessary.

Committed 8a3e98e and pushed to 8.0.x. Thanks!

  • alexpott committed 8a3e98e on 8.0.x 
    Issue #2596043 by mpdonadio, pjonckiere: DrupalDateTime constructor...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

The reason I think #10 will probably be unnecessary is that magical constructors that do different things depending on argument type are tricky beasts to review and code with.

Status: Fixed » Closed (fixed)

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