Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
With #2799987: Datetime and Datelist element's timezone handling is fragile, buggy and wrongly documented we improved and enforced timezone handling for Datetime and Datelist through the #date_timezone property. We should now make sure its documentation reflects the changes.
#3015647: $element['value']['#date_timezone'] is not a string fatal error. is an example of how #date_timezone has been (ab)used before, and after the new change a fatal error is raised due a string being expected rather than a \DateTimeZone object.
The current documentation for #date_timezone reads:
#date_timezone: The local timezone to use when displaying or interpreting dates. Defaults to the value returned by
Without specifying *a string* is expected.
Proposed resolution
Update the documentation.
Remaining tasks
Update the #date_timezone documentation, specifying a timezone name string is expected. Done by documenting this must be a TZID together with an example'Asia/Kolkata'.Understands if more documentation improvements are needed. Can't think of anything else. All changes from [ #2799987] have been covered.
User interface changes
Nope.
API changes
No, however change record https://www.drupal.org/node/2880055 doesn't specify a timezone name string is expected so although this change doesn't alter the API it does affect (aka clarify) its usage as well as its documentation.
Data model changes
No.
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | interdiff-3016064-15-18.txt | 1.95 KB | yogeshmpawar |
| #18 | 3016064-18.patch | 2.48 KB | yogeshmpawar |
| #15 | interdiff_12-15.txt | 1.92 KB | vacho |
| #15 | Datetime-Datelist-element-doc-2799987-15.patch | 2.38 KB | vacho |
| #12 | Datetime-Datelist-element-doc-2799987-12.patch | 2.68 KB | singhkiran |
Comments
Comment #2
gambryFixing CR link.
Comment #3
gambryComment #4
deepanker_bhalla CreditAttribution: deepanker_bhalla as a volunteer and at Srijan | A Material+ Company commentedDoes it works?
Comment #5
msankhala CreditAttribution: msankhala at Srijan | A Material+ Company commentedNo @deepanker_bhalla this will not work. The documentation you modified does not make any sense. You simply added "Without specifying" string in #date_timezone property's documentation. This issue is about specifying that value of the #date_timezone property should be in string format.
Here is the new patch.
Comment #6
gambryI'm worried "in the format returned by date_default_timezone_get()" can confuse developers.
Why don't we use something like:
"The local timezone name string to use when displaying or interpreting dates, i.e. 'Asia/Kolkata'. Defaults to the value returned by drupal_get_user_timezone()." so we reduce the changes to minimum while stressing we need a timezone name string, providing also an example.
Comment #7
msankhala CreditAttribution: msankhala at Srijan | A Material+ Company commentedI don't think this will confuse the developers because
date_default_timezone_get()anddrupal_get_user_timezone()both return string in the same format. Also, there is an example of what should be the format.Comment #8
dpiInstead of "time zone string", "string", "in format returned by date_default_timezone_get()" or similar, I recommended to use something similar to Time Zone Identifier (aka TZID) as defined by TZData / Olson Database.
W3 Working With Time Zones: https://www.w3.org/TR/timezone/#tzids
Also take care as time zone is two words.
Comment #9
gambryYep, agree with #8 as I believe using a PHP-agnostic naming convention it's better.
I suggested "timezone name [string]" (one word + name) because this is what the documentation for \DateTimeZone::__constructor() says.
So something like:
?
Comment #10
msankhala CreditAttribution: msankhala at Srijan | A Material+ Company commentedYes I agree with @dpi in #8. @dpi Are you suggesting to use "time zone" instead of "timezone"? But everywhere it is used as "timezone".
Comment #11
singhkiran CreditAttribution: singhkiran as a volunteer and at Srijan | A Material+ Company for Srijan | A Material+ Company commentedI will work on this in @ContributionWeekend2019.
Comment #12
singhkiran CreditAttribution: singhkiran as a volunteer and at Srijan | A Material+ Company for Srijan | A Material+ Company commentedPlease find above patch and review , i have added space in timezone. Thanks
Comment #14
gambry@singhkiran thanks a lot for your contribution. When updating previous patches is always superuseful to provide also an interdiff, so we can compare what's been changed. See here details about how to create an interdiff.
Setting this to needs work as we still need to changes the instances of "The string representation of time zone in the format returned by date_default_timezone_get(). For example: 'Asia/Kolkata'. This local timezone is used when displaying or interpreting dates. Defaults to the value returned by drupal_get_user_timezone()." to something like "The Time Zone Identifier (TZID) to use when displaying or interpreting dates, i.e. 'Asia/Kolkata'. Defaults to the value returned by drupal_get_user_timezone()."
Comment #15
vacho CreditAttribution: vacho at Skilld commentedI contribute to this refactoring the text to seems more like the current but adding the TZID concept.
Comment #16
gambryThank you @vacho ! We are nearly there. The comments span over the 80chars limit. Can you update those 4 lines?
Comment #17
yogeshmpawarComment #18
yogeshmpawarUpdated the patch as per comment #16 & also added an interdiff.
Comment #20
gambryAll looks good, thanks! Tests failure is not related (see #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest). RTBCing.
Updating remaining tasks section on IS.
Comment #21
alexpottCommitted and pushed 1dc9a511fc to 8.8.x and 00ce4413ca to 8.7.x. Thanks!