Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cron times in the database have a double timezone correction #4237

Closed
AntonEvers opened this issue Apr 20, 2016 · 30 comments
Closed

Cron times in the database have a double timezone correction #4237

AntonEvers opened this issue Apr 20, 2016 · 30 comments
Labels
bug report Fixed in 2.2.x The issue has been fixed in 2.2 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development

Comments

@AntonEvers
Copy link
Contributor

The cron schedule times in the database are local datetimes and not UTC whereas the other datetimes in the system are saved as UTC in the database.

Is this something worth changing?

@melnikovi
Copy link
Member

For columns of type TIMESTAMP MySQL returns date formatted according to timezone from config (server timezone by default). After you connect to the database, run query SET time_zone = '+0:00' to have it return dates in UTC. Let me know if this doesn't help

@piotrekkaminski
Copy link
Contributor

@melnikovi it's rather an inconsistency issue. All dates should be saved in one format and displayed accordingly to user preference/needs. @ajpevers do you have any specific issues resulting out this - as in promotions getting live to early etc?

@piotrekkaminski piotrekkaminski self-assigned this May 24, 2016
@AntonEvers
Copy link
Contributor Author

I have my env.php setup with 'initStatements' => 'SET NAMES utf8, time_zone="+00:00";', so that works just fine.

No there are no specific issues as of yet. But as an extension developer I foresee problems here. You will encounter unexpected behaviour if a plugin developer decides to work with MySQL date functions.

@melnikovi
Copy link
Member

@ajpevers how do you know that cron schedule in database is not in UTC? Also, you don't need to use initStatements in env.php to set timezone for connection, it's handled by framework.

@kitsor
Copy link

kitsor commented Jun 9, 2016

@melnikovi
Since default timezone has been hardcoded into bootrap.php
https://github.com/magento/magento2/blob/develop/app/bootstrap.php#L49

date_default_timezone_set('UTC');

You have big issue with getting scoped timestamp in Timezone.php#L209

...
@date_default_timezone_set($timezone);
$date = date('Y-m-d H:i:s');
@date_default_timezone_set($currentTimezone);
return strtotime($date);

You are getting STRING dateTime in scoped timezone, then reverting into default (UTC) and calculating TIMESTAMP from string ("scoped") using default (UTC) timezone.

Because of this all cron's $currentTime is incorrect.
In my case I see -08:00 in cron_schedule instead of correct -04:00 (EDT)

So fix should be like this:

    @date_default_timezone_set($timezone);
    $date = date('Y-m-d H:i:s');
    $retVal = strtotime($date);
    @date_default_timezone_set($currentTimezone);
    //return strtotime($date);
    return $retVal;

And... Since we are talking about cron..
I've found another odd code.
According to devDoc cron should be executed with defined .ini, but you are running child processes w/o any .ini: ProcessCronQueueObserver.php#L173

...
$phpPath = $this->phpExecutableFinder->find() ?: 'php';
...
$phpPath . ' %s cron:run --group=' . $groupId . ' --' . CLI::INPUT_KEY_BOOTSTRAP . '='

Is it correct?

@AntonEvers
Copy link
Contributor Author

@kitsor is correct, I also saw cron times with 4 hours difference. Some of my dev servers have php's time zone at UTC and mysql's global time zone at Europe/Amsterdam. To prevent any differences there I set the mysql timezone with the init statement. This had the result of only 2 hours difference in the cron times, but still the cron times are saved as Amsterdam time in a UTC database from UTC php code.
So effectively the Amsterdam time becomes UTC time in the DB. But without the time correction.

In a multi time zone system like Magento all times should be - in my opinion - saved in UTC and displayed according to the viewers time zone where appropriate.

@melnikovi
Copy link
Member

@ajpevers you don't need to use initStatements, as Magento sets timezone for connection in \Magento\Framework\DB\Adapter\Pdo\Mysql::_connect. We also set timezone in PHP in app/bootstrap.php.

Magento works with dates as you described. All dates stored in UTC and being formatted to user's timezone on frontend. Created internal task MAGETWO-54382 to double check if cron stores time in UTC.

When you connect to the database from terminal or MySQLWorkbench, do you set timezone for connection? Because if not, MySQL will return you dates with offset.

@AntonEvers
Copy link
Contributor Author

Hi @melnikovi thanks for the very quick response.
I do think I see the times with correction in my terminal. However, there is a difference between the times on an order for instance and the times in the cron_schedule table:

Just placed an order, time I see in the DB: 2016-06-14 16:10:46
Just opened cron_schedule, last job finished at: 2016-06-14 18:11:13
Local time here: 2016-06-14 16:10:46 CET
UTC: 2016-06-14 14:10:46 UTC

So in any case it can never be that the cron times in the DB are ahead of my time in CET. If anything I would expect them to be equal or 2 hours behind.

So I think the cron times are not UTC because they are different than the sales entity times.

I removed the timezone from the env.php init statements and that does indeed not make a difference. Thanks for that tip.

@kitsor
Copy link

kitsor commented Jun 14, 2016

@ajpevers Use my solution.
You don't need to use initStatements.
The core issue in Timezone.php on calculating scopeTimeStamp.

Just make a small fix:

$timezone = $this->_scopeConfig->getValue($this->getDefaultTimezonePath(), $this->_scopeType, $scope);
$currentTimezone = @date_default_timezone_get();
@date_default_timezone_set($timezone);
$date = date('Y-m-d H:i:s');
+ $retVal = strtotime($date);
@date_default_timezone_set($currentTimezone);
- //return strtotime($date);
+ return $retVal;

It works for me as expected.

@AntonEvers
Copy link
Contributor Author

AntonEvers commented Jun 15, 2016

Thanks @kitsor,

But my goal is not to patch my installation, it's to make Magento a healthier product. So if this solution works, and does not have a negative impact on other places in the installation, can you make it a pull request?

@AntonEvers
Copy link
Contributor Author

@wert2all Can I turn this into a bug report instead of a question? The datetime in the cron_schedule contains another time zone than the datetime in sales tables.

@AntonEvers
Copy link
Contributor Author

@wert2all friendly reminder

@wert2all
Copy link
Contributor

@ajpevers as I see above the internal task MAGETWO-54382 was created about this issue.

@AntonEvers
Copy link
Contributor Author

Ah sorry, thank you

@piotrekkaminski piotrekkaminski removed their assignment Aug 22, 2016
@PiscesThankIT
Copy link

@ajpevers Hi, I came across the same issue too. My local is UTC+8, the cron job time in database is 8 hours more than local. If local time is 2016/10/7 8:00, the cron job time will be 16:00. However, the order time is local time no matter which local is set. I want to know if this is Magento 2 's design that order time is in local time. Please help me. Thanks in advance!

@AntonEvers
Copy link
Contributor Author

@PiscesThankIT as far as I can see all DB times are UTC as set in \Magento\Framework\DB\Adapter\Pdo\Mysql::_connect

$this->_connection->query("SET time_zone = '+00:00'");

The cron table is the exception, because the dates are changed before they go into the DB. However the cron code is written with this in mind and the jobs execute on time. That is, if you plan a cron job 5 minutes from now it will run 5 minutes from now, regardless of the DB time difference.

If however you plan to use these DB cron times for something other than executing cron jobs, you will run into trouble. For instance when you create a grid with cron jobs in the Magento backend or if you want to show the next execution time for a cron job somewhere in the shop. Or if you want to connect a monitoring service etc...

Let's wait for MAGETWO-54382 to be released and look how it goes from there. Meanwhile keep in mind that you will have to rewrite code that depends on the cron schedule times after the MAGETWO-54382 release.

@veloraven veloraven added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Oct 26, 2016
@PascalBrouwers
Copy link
Contributor

Any update on this? In my db I also see cron's scheduled ahead by 2 hours (because I am in GMT+2), but also executed ahead by 2 hours. Very confusing.

AntonEvers pushed a commit to AntonEvers/magento2 that referenced this issue Jun 14, 2017
Because the cron job uses a localized timestamp instead of a regular
timestamp, all times in the cron_schedule table are not the actual
times, but times with a double offset.

By using the regular timestamp, all times in cron_schedule are correct.
The cron job execution times are not affected by this change because
schedule generation and job execution depend on the same timestamp.
If both are in the same timezone all will go as intended.

Fixes magento#4237
@AntonEvers AntonEvers changed the title Cron times in the database are not UTC Cron times in the database have a double timezone correction Jun 14, 2017
@AntonEvers
Copy link
Contributor Author

@ishakhsuvarov this is fixed with #9943

@korostii
Copy link
Contributor

Hello @magento-team,
Could you please announce in which version is this fix scheduled to ship?
It's a bit confusing to see the issue closed while the latest (2.1.7) release still has the bug inside.

@ishakhsuvarov
Copy link
Contributor

Pull Request was targeted to the develop branch so we expect it to be shipped in 2.2.0.

@AlexanderRakov
Copy link

@ajpevers
I've applyed #9943 to my local environment and found that if I set CST timezone in Magento Configuration, cron work accordingly UTC time zone. So if I create cronjob running at midnight, it will start at midnight by UTC, not CST. Is it correct behavior that PR?

@AntonEvers
Copy link
Contributor Author

@AlexanderRakov I do expect it now you mention it, having the current code change in place. But I would not expect it if I were a shop owner. So now that the times are correct in the cron_schedule table, I need to add a change that converts the crontab expression to the correct unix time stamp (UTC) for the matching time in the admin store view time zone.

@magento-team
Copy link
Contributor

Internal ticket to track issue progress: MAGETWO-69886

@AntonEvers
Copy link
Contributor Author

Is it possible to assign this issue to me?

@AntonEvers
Copy link
Contributor Author

@AlexanderRakov PR submitted.

@magento-team
Copy link
Contributor

Internal ticket to track issue progress: MAGETWO-71359

@magento-engcom-team magento-engcom-team added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development bug report develop Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed labels Sep 11, 2017
@magento-engcom-team magento-engcom-team added the Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed label Sep 20, 2017
@magento-engcom-team
Copy link
Contributor

@ajpevers thank you for your report.
The fix for this issue is already available in release 2.2.0

@magento-engcom-team magento-engcom-team added Fixed in 2.1.x The issue has been fixed in 2.1 release line Fixed in 2.2.x The issue has been fixed in 2.2 release line labels Sep 20, 2017
@hostep
Copy link
Contributor

hostep commented Sep 21, 2017

@magento-engcom-team: I think the label Fixed in 2.1.x is incorrect here, I see no evidence that #9943 has been applied to the 2.1 branch?

@korostii
Copy link
Contributor

Care to comment @magento-engcom-team ?
Please see the last comment by @hostep

@magento-engcom-team magento-engcom-team removed the Fixed in 2.1.x The issue has been fixed in 2.1 release line label Sep 29, 2017
@magento-engcom-team
Copy link
Contributor

@korostii Thanks for pointing out

magento-team pushed a commit that referenced this issue Jun 25, 2019
[TSG] Fixes for 2.3.2 (pr52) (2.3.2-develop)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Fixed in 2.2.x The issue has been fixed in 2.2 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

No branches or pull requests