Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bugster’s picture

If you don't pass a DateTimeZone object but the string timezone, it does not fail. DateObject also accepts the string version.

Guess the documentation is a bit misleading @param object $timezone.

jojonaloha’s picture

Status: Active » Needs review
FileSize
1.79 KB

This bug was introduced in #2261395: date_now is not respecting changes to timezone.

The reason is DateTimeZone doesn't have a __toString() method and the issue assumed it was always a string, even though the code comment suggests it is always an object.

Attached patch updates the code comment to better reflect the accepted parameters (same as the DateObject constructor), and fixes the bug by handling DateTimeZone objects when building the static variable name.

Status: Needs review » Needs work

The last submitted patch, 2: date-datetimezone-converted-to-string-2325207-2.patch, failed testing.

jojonaloha’s picture

Status: Needs work » Needs review
FileSize
1.79 KB

Re-rolled patch attached.

Yurii Krysiuk’s picture

Assigned: Unassigned » Yurii Krysiuk
Yurii Krysiuk’s picture

Assigned: Yurii Krysiuk » Unassigned

moymilo’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +dcuacs2015

Reviewed patch and manualy tested, looks like all work fine.

podarok’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/date_api/date_api.module
@@ -1844,11 +1845,18 @@ function date_format_interval($date, $granularity = 2, $display_ago = TRUE) {
+  else {
+    $static_var = __FUNCTION__ . $timezone;
+  }

Please, use Early define pattern here
Example:
https://github.com/propeoplemd/cibox/wiki/PHP-JavaScript-SCSS-SASS-best-...

v1nk’s picture

Assigned: Unassigned » v1nk
Status: Needs work » Needs review
FileSize
1.38 KB
626 bytes
andypost’s picture

Status: Needs review » Needs work
+++ b/date_api/date_api.module
@@ -1844,11 +1845,16 @@ function date_format_interval($date, $granularity = 2, $display_ago = TRUE) {
+  $static_var = __FUNCTION__ . $timezone;
+  if ($timezone instanceof DateTimeZone) {
+    $static_var = __FUNCTION__ . $timezone->getName();

should be in else
if (instance) use getName()
else concatenation

v1nk’s picture

@andypost
Removed else regarding @podarok comment. So how it should be right ?

  • podarok committed b7fa095 on 7.x-2.x authored by v1nk
    Issue #2325207 by jojonaloha, v1nk, podarok: DateTimeZone could not be...
podarok’s picture

Status: Needs work » Fixed
Issue tags: +CodeSprintUA

Thanks @v1nk , #11 committed

jojonaloha’s picture

Status: Fixed » Needs work

Sorry, but #11 does not fix the issue, it just adds more code that suffers from the same problem. I think a test is needed.

StevenWill’s picture

The patch from #5 fixed the issue for me.

podarok’s picture

@jojonaloha can you please be more specific with a real example of the problem?
Thanks in advance

jojonaloha’s picture

Attached is a patch with a simpletest. This is my first time writing a SimpleTest test, and the tests are purely sanity checks, but do fail on the exception.

First patch contains only the test, second patch contains the test and a re-roll of my originally proposed fix.

Status: Needs review » Needs work

The last submitted patch, 19: date-datetimezone-converted-to-string-2325207-19.patch, failed testing.

jojonaloha’s picture

Status: Needs work » Needs review
FileSize
3.39 KB

Let's try this again. This one should fail.

Status: Needs review » Needs work
jojonaloha’s picture

Status: Needs work » Needs review
FileSize
4.4 KB

This one should pass.

Sebastien M.’s picture

Here is an update of the #24 patch, but matching to date-7.x-2.9 release.
Hope it helps,
Thanks

Status: Needs review » Needs work

The last submitted patch, 26: date-datetimezone-converted-to-string-2325207-26.patch, failed testing.

jojonaloha’s picture

Assigned: v1nk » Unassigned
Status: Needs work » Needs review

Patch #24 is still passing and the one to be used against 7.x-2.x-dev. Moving back to needs review.

Sebastien M.’s picture

What about this issue ?

Sebastien M.’s picture

#26 is only provided to work on 7.x-2.9.

RoSk0’s picture

Status: Needs review » Fixed

Assuming fixed because included in 7.x-2.10 release.

@Sebastien @Actualys Please create a follow up issue if you think that there is something to be fixed.

Status: Fixed » Closed (fixed)

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

jojonaloha’s picture

This is not fixed. If you applied the patch in #22, which only includes a test, and ran the tests you would see the exception described in this ticket still. See attached screenshot (against a fresh Drupal 7.54 install and Date 7.x-dev + the patch in #22).

I've also re-ran the tests with the patch in #24 and it passes. So #24 is still the patch to use to fix this issue. Please re-open this issue as it is not fixed, 7.x-2.10 only included the patch in #11 which still suffers from this bug, as does the latest dev version.

jojonaloha’s picture

I've opened #2880395: Fatal error: DateTimeZone could not be converted to string as a follow-up to this issue since this one is closed and I cannot re-open it.