Test cron expressions against localized time #10432
Conversation
|
Hi @ajpevers |
|
Checks passing. @ishakhsuvarov is this also a valid way to maintain backward compatibility or do you want me to move the object manager fallback back to the constructor and change the unit test? |
This would be a correct solution according to current guidelines. |
| $time = $this->getScheduledAt(); | ||
| $e = $this->getCronExprArr(); | ||
|
|
||
| if (!$e || !$time) { | ||
| return false; | ||
| } | ||
| if (!is_numeric($time)) { | ||
| $time = strtotime($time); | ||
| $time = strtotime($time) + $this->dateTime->getGmtOffset(); |
There was a problem hiding this comment.
We should not calculate the time for another timezone using inline code. In the future, it will be hard to track all such places if there some bugs. Please, use \Magento\Framework\Stdlib\DateTime\TimezoneInterface to convert to date $time to Default timezone.
…o date $time to Default timezone
|
@okorshenko for a few hours I've been struggling with the unit tests that should be rewritten for my latest change. I'm quite new to this and came a long way, but here I'm stuck. Can you connect me with someone that can help me fix the tests? (It would be nice if we can configure the test with a timezone offset while we're at it.) |
| */ | ||
| public function __construct( | ||
| \Magento\Framework\Model\Context $context, | ||
| \Magento\Framework\Registry $registry, | ||
| \Magento\Framework\Model\ResourceModel\AbstractResource $resource = null, | ||
| \Magento\Framework\Data\Collection\AbstractDb $resourceCollection = null, | ||
| array $data = [] | ||
| array $data = [], | ||
| \Magento\Framework\Stdlib\DateTime\TimezoneInterface $timeZone = null |
There was a problem hiding this comment.
This has to be a required argument (to avoid the object manager hack below). Move it up to go before the optional ones.
There was a problem hiding this comment.
Adding new required argument to the class constructor will break backwards compatibility, for those third-party extensions which could have extended from it.
Please refer to the Backwards Compatible Development Guide for more information (this case is covered in Adding a constructor parameter section)
There was a problem hiding this comment.
@ishakhsuvarov
Good to know. The BC policy seems very decremental to the codebase :'(
| */ | ||
| private function getDateTime() | ||
| { | ||
| $this->timeZone = $this->timeZone ?: \Magento\Framework\App\ObjectManager::getInstance() |
There was a problem hiding this comment.
Static access to the object manager is highly discouraged. The interface primarily exists for the unserialization use case. It should not be used outside of that. The object manager will not be necessary if to inject the dependency as required on constructor. See the constructor comment above.
- fixed timezone conversion logic - fixed unit tests
- fixed timezone conversion logic
Description
As mentioned in #4237:
@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.
Fixed Issues (if relevant)
Manual testing scenarios
bin/magento cron:runSELECTscheduled_atFROMcron_scheduleWHEREjob_code= "sitemap_generate";Contribution checklist