Updated: Comment #N
core component: datetime.module
From reading the documentation in the source code and from here https://drupal.org/node/1834108
It is possible to create a datetime in array format and the following code should be correct...
$date = new DrupalDateTime(array('year' => 2012, 'month' => 12, 'day' => 31));
This however results in the following error
Warning: date_parse() expects parameter 1 to be string, array given in Drupal\Component\Datetime\DateTimePlus->__construct() (line 300 of core/lib/Drupal/Component/Datetime/DateTimePlus.php).
date 20 Oct 2013
A search of the datetime/drupal/core/modules/datetime/lib/Drupal/datetime/Tests/ directory reveals testing of DrupalDateTime obects
only with a string as the first parameter, the array form is never tested and this is why this error has not been reported until now
The constructor for DrupalDateTime accepts an array as first parameter which is uses to call it parent constructor DateTimePlus
The logic error is that the array is passed to DateTimePlus when only a string is accepted hence the thrown exception
| Issue category | Bug |
|---|---|
| Issue priority | Normal |
| Unfrozen changes | Unfrozen because it only changes API documentation |
| Prioritized changes | The main goal of this issue is DX. |
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | Drupal-DrupalDateTimeDocumentationFix-2116327-6.patch | 1.18 KB | martin107 |
| #5 | Drupal-DrupalDateTimeDocumentationFix-2116327.patch | 1.14 KB | martin107 |
Comments
Comment #1
martin107 commentedComment #2
berdirDateTimePlus has a static createFromArray() and a few similar methods. I guess that's the one that you should use, have you tried DrupalDateTime::createFromArray() ?
Might just need a documentation update and possibly some tests if those methods aren't tested.
Comment #3
martin107 commentedI can confirm the following works
So the path of least resistance is to change the documentation to remove the broken access method and highlight Bedirs DrupalDateTime::createFromArray() suggestion.
We are beyond API freeze and this seems like a hidden API change so I would like advice on how to signal this, especially as the official documentation makes the same mistake I made.
If I can get consesus I will create a patch which strikes the following from
drupal/code/lib/Drupal/Component/Datetime/DateTimePlus.php
class DrupalDateTime extends DateTimePlus {
/**
* Constructs a date object.
*
* @param mixed $time
* A DateTime object, a date/input_time_adjusted string, a unix timestamp,
*
or an array of date parts, like ('year' => 2014, 'month => 4).* Defaults to 'now'.
*
* see also DrupalDateTime::createFromArray()
*
* @param mixed $timezone
* PHP DateTimeZone object, string or NULL allowed.
* Defaults to NULL.
Comment #4
berdirI don't think this is an API change, just a documentation bug. This change happened in #1844956: Optimize date formatting performance, which unfortunately didn't get a change notice yet.
It would be great if you could post a link to this issue there, fix the documentation here and update the change notice at https://drupal.org/node/1834108 (also add a reference to the mentioned and this issue).
Comment #5
martin107 commentedOk so here is the patch for review
Also
Comment added to isssue #1844956.
Documentation update https://drupal.org/node/1834108 with link to this issue in the change log.
As for extending test coverage my only comment is that the tests in DateTimeTestPlus.php always specifies a timezone, and a extra test should be included to show that when the last two OPTIONAL parameters timezone, and settings are omitted then the sensible defaults kick in and no exceptions are thrown.
This is not a strong opinion, or a major improvement in test coverage, BUT it is a trival patch and I will be happy to do it if someone shouts "good catch"
Comment #6
martin107 commentedminor correction
$time the parameter for which the documentation has changed was as described as
@param mixed $time
and now correctly reads
@param string $time
Comment #7
berdir6: Drupal-DrupalDateTimeDocumentationFix-2116327-6.patch queued for re-testing.
Comment #8
jhedstromIIRC documentation bugs are major.
Patch still applies and makes sense to me.
Comment #9
alexpottNot sure about the major-ness of this issue. To me, this issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 000f392 and pushed to 8.0.x. Thanks!
Comment #11
martin107 commentedGreat - Thank you for trawling through old issues :)