Because of date_now beginning:

function date_now($timezone = NULL, $reset = FALSE) {
  $now = &drupal_static(__FUNCTION__);

A second call to date_now with a different timezone of first call is not respected. For example:

  $now = date_now('Europe/Stockholm');
  dpm($now);
  $now2 = date_now('UTC');
  dpm($now2);

Here $now2 will containing date in Europe/Stockholm timezone. One can set the $reset parameter to true, but you never know if some code has called date_now before you with an other timezone, so to be sure that your date is in correct timezone one would always need to set $reset to true. (and disable static caching), As for the example above it is not 100% sure that $now till be in timezone Europe/Stockolm. As long as no other module or code has called date_now before this code is should be correct.

Found this problem when debugging an other issue #2261377: date_example_date() changed value of date_now()

CommentFileSizeAuthor
#5 2261395-date_now_timezone-5.patch893 bytesminorOffense
#2 2261395-date_now_timezone.patch715 bytesminorOffense
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markus_petrux’s picture

markus_petrux CreditAttribution: markus_petrux commented

Related issue where this optimization was implemented: #1933472: Optimize date_now function ...since then, date_now() caches the date object in static memory.

I think that patch (unintentionally) broke the nature of date_now() function, because now it may happen that it does not return the current date. Anyone, anywhere could have changed the value of the cached date. In other words, date_now() could lie, and you have no control about it.

How about return a clone of the cached date object?

Also, static storage would have to take into account the timezone argument as well, of course.

minorOffense’s picture

minorOffense CreditAttribution: minorOffense commented
FileSize
2261395-date_now_timezone.patch715 bytes

Well since I wrote the original patch, may as well take a stab at fixing the bug(s) it introduced.

Here's a patch to fix the caching with the timezone argument (essentially just change the static cache signature based on timezone).

As for the this comments

I think that patch broke the nature of date_now() function, because now it may happen that it does not return the current date. Anyone, anywhere could have changed the value of the cache date. In other words, date_now() could lie, and you have no control about it.

That's true of any cached item, or any data item for that matter. If something goes in and changes the cached value to something arbitrary, nothing the API can really do for you in that case.

Definitely having the timezone as part of the cache signature is an oversight on my behalf (my bad) but I don't think the nature of the function has really changed.

minorOffense’s picture

minorOffense CreditAttribution: minorOffense commented
Status: Active » Needs review
kristofferwiklund’s picture

kristofferwiklund CreditAttribution: kristofferwiklund commented

I have made some tests. And it might be an idea to return a clone of the static value. Because as it is right now the return value can be changed in a calling function, as the static return value is a reference. So changes to it will alter the value for all sub requests to date_now.

The performance penalty is not so big. I did this test code

function testing() {
  static $result;
  if(empty($result)) {
    $result = new DateTime();
    $result->test = 1;
    echo "Result created\n";
  }
  $clone = clone $result;
  return $clone;
}

for($i = 0; $i<10000000; $i++) {
  $novalue = testing();
}

Without the cloning of the result this code took, 2.5 seconds. But with cloning it took 5.6 seconds. So cloning a DateTime object takes 0,00031ms. And the memory footprint is no change as PHP is doing a lazy cloning. As long as the cloned object is not changed it uses the same memory block.

minorOffense’s picture

minorOffense CreditAttribution: minorOffense commented
FileSize
2261395-date_now_timezone-5.patch893 bytes

Here's a patch with the previous changes and the clone.

kristofferwiklund’s picture

kristofferwiklund CreditAttribution: kristofferwiklund commented
Status: Needs review » Reviewed & tested by the community

I have tested for both cases (caching per timezone and returning a cloned object) and patch works nice.

This could now close this issue
#2261377: date_example_date() changed value of date_now()

  • Commit a8edfd3 on 7.x-2.x authored by minorOffense, committed by podarok: 
    Issue #2261395 by minorOffense | kristofferwiklund: Fixed date_now is...
podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
CreditAttribution: podarok commented
Status: Reviewed & tested by the community » Fixed

#5 commited
Thanks!

Status: Fixed » Closed (fixed)

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