Skip to content

Commit

Permalink
MAGETWO-33637: [GITHUB] Bug generating Sitemap Cron expression magent…
Browse files Browse the repository at this point in the history 
…o#1020

- These are rebased commits.
  • Loading branch information
Safwan Khan committed Mar 19, 2015
1 parent d1ff85c commit 752bacc
Show file tree
Hide file tree
Showing 8 changed files with 128 additions and 82 deletions.
2 changes: 1 addition & 1 deletion app/code/Magento/Cron/Model/Config/Backend/Sitemap.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public function __construct(
public function afterSave()
{
$time = $this->getData('groups/generate/fields/time/value');
$frequency = $this->getData('groups/generate/frequency/value');
$frequency = $this->getData('groups/generate/fields/frequency/value');

$cronExprArray = [
intval($time[1]), //Minute
Expand Down
76 changes: 49 additions & 27 deletions app/code/Magento/Cron/Model/Observer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ class Observer
*/
protected $_shell;

/**
* @var \Magento\Framework\Stdlib\DateTime\TimezoneInterface
*/
protected $timezone;

/**
* @param \Magento\Framework\ObjectManagerInterface $objectManager
* @param ScheduleFactory $scheduleFactory
Expand All @@ -95,6 +100,7 @@ class Observer
* @param \Magento\Framework\App\Config\ScopeConfigInterface $scopeConfig
* @param \Magento\Framework\App\Console\Request $request
* @param \Magento\Framework\ShellInterface $shell
* @param \Magento\Framework\Stdlib\DateTime\TimezoneInterface $timezone
*/
public function __construct(
\Magento\Framework\ObjectManagerInterface $objectManager,
Expand All @@ -103,7 +109,8 @@ public function __construct(
\Magento\Cron\Model\ConfigInterface $config,
\Magento\Framework\App\Config\ScopeConfigInterface $scopeConfig,
\Magento\Framework\App\Console\Request $request,
\Magento\Framework\ShellInterface $shell
\Magento\Framework\ShellInterface $shell,
\Magento\Framework\Stdlib\DateTime\TimezoneInterface $timezone
) {
$this->_objectManager = $objectManager;
$this->_scheduleFactory = $scheduleFactory;
Expand All @@ -112,6 +119,7 @@ public function __construct(
$this->_scopeConfig = $scopeConfig;
$this->_request = $request;
$this->_shell = $shell;
$this->timezone = $timezone;
}

/**
Expand All @@ -128,30 +136,29 @@ public function __construct(
public function dispatch($observer)
{
$pendingJobs = $this->_getPendingSchedules();
$currentTime = time();
$currentTime = $this->timezone->scopeTimeStamp();
$jobGroupsRoot = $this->_config->getJobs();

foreach ($jobGroupsRoot as $groupId => $jobsRoot) {
if ($this->_request->getParam(
'group'
) === null && $this->_scopeConfig->getValue(
'system/cron/' . $groupId . '/use_separate_process',
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
) == 1
) {
if ($this->_request->getParam('group') !== null && $this->_request->getParam('group') != $groupId) {
continue;
}
if (($this->_request->getParam('standaloneProcessStarted') !== '1') && (
$this->_scopeConfig->getValue(
'system/cron/' . $groupId . '/use_separate_process',
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
) == 1
)) {
$this->_shell->execute(
'%s -f %s -- --group=%s',
'php -f %s -- --group=%s --standaloneProcessStarted=%s',
[
PHP_BINARY,
BP . '/' . DirectoryList::PUB . '/cron.php',
$groupId
$groupId,
'1'
]
);
continue;
}
if ($this->_request->getParam('group') !== null && $this->_request->getParam('group') != $groupId) {
continue;
}

foreach ($pendingJobs as $schedule) {
$jobConfig = isset($jobsRoot[$schedule->getJobCode()]) ? $jobsRoot[$schedule->getJobCode()] : null;
Expand All @@ -160,12 +167,14 @@ public function dispatch($observer)
}

$scheduledTime = strtotime($schedule->getScheduledAt());
if ($scheduledTime > $currentTime || !$schedule->tryLockJob()) {
if ($scheduledTime > $currentTime) {
continue;
}

try {
$this->_runJob($scheduledTime, $currentTime, $jobConfig, $schedule, $groupId);
if ($schedule->tryLockJob()) {
$this->_runJob($scheduledTime, $currentTime, $jobConfig, $schedule, $groupId);
}
} catch (\Exception $e) {
$schedule->setMessages($e->getMessage());
}
Expand Down Expand Up @@ -213,11 +222,14 @@ protected function _runJob($scheduledTime, $currentTime, $jobConfig, $schedule,
);
}

$schedule->setExecutedAt(strftime('%Y-%m-%d %H:%M:%S', time()))->save();
$schedule->setExecutedAt(strftime('%Y-%m-%d %H:%M:%S', $this->timezone->scopeTimeStamp()))->save();

call_user_func_array($callback, [$schedule]);

$schedule->setStatus(Schedule::STATUS_SUCCESS)->setFinishedAt(strftime('%Y-%m-%d %H:%M:%S', time()));
$schedule->setStatus(Schedule::STATUS_SUCCESS)->setFinishedAt(strftime(
'%Y-%m-%d %H:%M:%S',
$this->timezone->scopeTimeStamp()
));
}

/**
Expand Down Expand Up @@ -253,7 +265,7 @@ protected function _generate($groupId)
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
);
$schedulePeriod = $rawSchedulePeriod * self::SECONDS_IN_MINUTE;
if ($lastRun > time() - $schedulePeriod) {
if ($lastRun > $this->timezone->scopeTimeStamp() - $schedulePeriod) {
return $this;
}

Expand All @@ -273,7 +285,12 @@ protected function _generate($groupId)
/**
* save time schedules generation was ran with no expiration
*/
$this->_cache->save(time(), self::CACHE_KEY_LAST_SCHEDULE_GENERATE_AT . $groupId, ['crontab'], null);
$this->_cache->save(
$this->timezone->scopeTimeStamp(),
self::CACHE_KEY_LAST_SCHEDULE_GENERATE_AT . $groupId,
['crontab'],
null
);

return $this;
}
Expand Down Expand Up @@ -324,7 +341,7 @@ protected function _cleanup($groupId)
'system/cron/' . $groupId . '/' . self::XML_PATH_HISTORY_CLEANUP_EVERY,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
);
if ($lastCleanup > time() - $historyCleanUp * self::SECONDS_IN_MINUTE) {
if ($lastCleanup > $this->timezone->scopeTimeStamp() - $historyCleanUp * self::SECONDS_IN_MINUTE) {
return $this;
}

Expand All @@ -350,7 +367,7 @@ protected function _cleanup($groupId)
Schedule::STATUS_ERROR => $historyFailure * self::SECONDS_IN_MINUTE,
];

$now = time();
$now = $this->timezone->scopeTimeStamp();
/** @var Schedule $record */
foreach ($history as $record) {
if (strtotime($record->getExecutedAt()) < $now - $historyLifetimes[$record->getStatus()]) {
Expand All @@ -359,7 +376,12 @@ protected function _cleanup($groupId)
}

// save time history cleanup was ran with no expiration
$this->_cache->save(time(), self::CACHE_KEY_LAST_HISTORY_CLEANUP_AT . $groupId, ['crontab'], null);
$this->_cache->save(
$this->timezone->scopeTimeStamp(),
self::CACHE_KEY_LAST_HISTORY_CLEANUP_AT . $groupId,
['crontab'],
null
);

return $this;
}
Expand Down Expand Up @@ -387,19 +409,19 @@ protected function getConfigSchedule($jobConfig)
*/
protected function saveSchedule($jobCode, $cronExpression, $timeInterval, $exists)
{
$currentTime = time();
$currentTime = $this->timezone->scopeTimeStamp();
$timeAhead = $currentTime + $timeInterval;
for ($time = $currentTime; $time < $timeAhead; $time += self::SECONDS_IN_MINUTE) {
$ts = strftime('%Y-%m-%d %H:%M:00', $time);
if (!empty($exists[$jobCode . '/' . $ts])) {
// already scheduled
continue;
}

$schedule = $this->generateSchedule($jobCode, $cronExpression, $time);
if ($schedule->trySchedule()) {
// time matches cron expression
$schedule->save();
return;
}
}
}
Expand All @@ -416,7 +438,7 @@ protected function generateSchedule($jobCode, $cronExpression, $time)
->setCronExpr($cronExpression)
->setJobCode($jobCode)
->setStatus(Schedule::STATUS_PENDING)
->setCreatedAt(strftime('%Y-%m-%d %H:%M:%S', time()))
->setCreatedAt(strftime('%Y-%m-%d %H:%M:%S', $this->timezone->scopeTimeStamp()))
->setScheduledAt(strftime('%Y-%m-%d %H:%M', $time));

return $schedule;
Expand Down
21 changes: 5 additions & 16 deletions app/code/Magento/Cron/Model/Schedule.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,28 +44,20 @@ class Schedule extends \Magento\Framework\Model\AbstractModel

const STATUS_ERROR = 'error';

/**
* @var \Magento\Framework\Stdlib\DateTime\TimezoneInterface
*/
protected $timezone;

/**
* @param \Magento\Framework\Model\Context $context
* @param \Magento\Framework\Registry $registry
* @param \Magento\Framework\Stdlib\DateTime\TimezoneInterface $timezone
* @param \Magento\Framework\Model\Resource\AbstractResource $resource
* @param \Magento\Framework\Data\Collection\Db $resourceCollection
* @param array $data
*/
public function __construct(
\Magento\Framework\Model\Context $context,
\Magento\Framework\Registry $registry,
\Magento\Framework\Stdlib\DateTime\TimezoneInterface $timezone,
\Magento\Framework\Model\Resource\AbstractResource $resource = null,
\Magento\Framework\Data\Collection\Db $resourceCollection = null,
array $data = []
) {
$this->timezone = $timezone;
parent::__construct($context, $registry, $resource, $resourceCollection, $data);
}

Expand Down Expand Up @@ -111,14 +103,11 @@ public function trySchedule()
if (!is_numeric($time)) {
$time = strtotime($time);
}

$dateWithTimezone = $this->timezone->date($time);

$match = $this->matchCronExpression($e[0], $dateWithTimezone->format('i'))
&& $this->matchCronExpression($e[1], $dateWithTimezone->format('H'))
&& $this->matchCronExpression($e[2], $dateWithTimezone->format('d'))
&& $this->matchCronExpression($e[3], $dateWithTimezone->format('m'))
&& $this->matchCronExpression($e[4], $dateWithTimezone->format('N'));
$match = $this->matchCronExpression($e[0], strftime('%M', $time))
&& $this->matchCronExpression($e[1], strftime('%H', $time))
&& $this->matchCronExpression($e[2], strftime('%d', $time))
&& $this->matchCronExpression($e[3], strftime('%m', $time))
&& $this->matchCronExpression($e[4], strftime('%w', $time));

return $match;
}
Expand Down
32 changes: 21 additions & 11 deletions app/code/Magento/Cron/Test/Unit/Model/ObserverTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ class ObserverTest extends \PHPUnit_Framework_TestCase
*/
protected $_cronGroupConfig;

/**
* @var \Magento\Framework\Stdlib\DateTime\TimezoneInterface
*/
protected $timezone;

/**
* Prepare parameters
*/
Expand Down Expand Up @@ -93,14 +98,17 @@ public function setUp()
['execute']
)->getMock();

$this->timezone = $this->getMock('Magento\Framework\Stdlib\DateTime\TimezoneInterface');
$this->timezone->expects($this->any())->method('scopeTimeStamp')->will($this->returnValue(time()));
$this->_observer = new \Magento\Cron\Model\Observer(
$this->_objectManager,
$this->_scheduleFactory,
$this->_cache,
$this->_config,
$this->_scopeConfig,
$this->_request,
$this->_shell
$this->_shell,
$this->timezone
);
}

Expand All @@ -112,7 +120,7 @@ public function testDispatchNoPendingJobs()
$lastRun = time() + 10000000;
$this->_cache->expects($this->any())->method('load')->will($this->returnValue($lastRun));
$this->_scopeConfig->expects($this->any())->method('getValue')->will($this->returnValue(0));

$this->_request->expects($this->any())->method('getParam')->will($this->returnValue('test_job1'));
$this->_config->expects($this->once())->method('getJobs')->will($this->returnValue([]));

$scheduleMock = $this->getMockBuilder('Magento\Cron\Model\Schedule')->disableOriginalConstructor()->getMock();
Expand Down Expand Up @@ -161,16 +169,17 @@ public function testDispatchCanNotLock()
$lastRun = time() + 10000000;
$this->_cache->expects($this->any())->method('load')->will($this->returnValue($lastRun));
$this->_scopeConfig->expects($this->any())->method('getValue')->will($this->returnValue(0));

$this->_request->expects($this->any())->method('getParam')->will($this->returnValue('test_group'));
$schedule = $this->getMockBuilder(
'Magento\Cron\Model\Schedule'
)->setMethods(
['getJobCode', 'tryLockJob', 'getScheduledAt', '__wakeup']
['getJobCode', 'tryLockJob', 'getScheduledAt', '__wakeup', 'save']
)->disableOriginalConstructor()->getMock();
$schedule->expects($this->any())->method('getJobCode')->will($this->returnValue('test_job1'));
$schedule->expects($this->once())->method('getScheduledAt')->will($this->returnValue('-1 day'));
$schedule->expects($this->once())->method('tryLockJob')->will($this->returnValue(false));

$abstractModel = $this->getMock('Magento\Framework\Model\AbstractModel', [], [], '', false);
$schedule->expects($this->any())->method('save')->will($this->returnValue($abstractModel));
$this->_collection->addItem($schedule);

$this->_config->expects(
Expand Down Expand Up @@ -198,7 +207,7 @@ public function testDispatchExceptionTooLate()
$lastRun = time() + 10000000;
$this->_cache->expects($this->any())->method('load')->will($this->returnValue($lastRun));
$this->_scopeConfig->expects($this->any())->method('getValue')->will($this->returnValue(0));

$this->_request->expects($this->any())->method('getParam')->will($this->returnValue('test_group'));
$schedule = $this->getMockBuilder(
'Magento\Cron\Model\Schedule'
)->setMethods(
Expand Down Expand Up @@ -262,7 +271,7 @@ public function testDispatchExceptionNoCallback()
);
$schedule->expects($this->once())->method('setMessages')->with($this->equalTo($exceptionMessage));
$schedule->expects($this->once())->method('save');

$this->_request->expects($this->any())->method('getParam')->will($this->returnValue('test_group'));
$this->_collection->addItem($schedule);

$jobConfig = ['test_group' => ['test_job1' => ['instance' => 'Some_Class']]];
Expand Down Expand Up @@ -293,7 +302,7 @@ public function testDispatchExceptionNotExecutable()
];

$exceptionMessage = 'Invalid callback: Not_Existed_Class::notExistedMethod can\'t be called';

$this->_request->expects($this->any())->method('getParam')->will($this->returnValue('test_group'));
$schedule = $this->getMockBuilder(
'Magento\Cron\Model\Schedule'
)->setMethods(
Expand Down Expand Up @@ -349,6 +358,7 @@ public function testDispatchRunJob()
$jobConfig = [
'test_group' => ['test_job1' => ['instance' => 'CronJob', 'method' => 'execute']],
];
$this->_request->expects($this->any())->method('getParam')->will($this->returnValue('test_group'));

$scheduleMethods = [
'getJobCode',
Expand Down Expand Up @@ -430,7 +440,7 @@ public function testDispatchNotGenerate()
)->will(
$this->returnValue(['test_group' => []])
);

$this->_request->expects($this->any())->method('getParam')->will($this->returnValue('test_group'));
$this->_cache->expects(
$this->at(0)
)->method(
Expand Down Expand Up @@ -501,7 +511,7 @@ public function testDispatchGenerate()
],
];
$this->_config->expects($this->at(1))->method('getJobs')->will($this->returnValue($jobs));

$this->_request->expects($this->any())->method('getParam')->will($this->returnValue('test_group'));
$this->_cache->expects(
$this->at(0)
)->method(
Expand Down Expand Up @@ -568,7 +578,7 @@ public function testDispatchCleanup()
)->getMock();
$schedule->expects($this->any())->method('getExecutedAt')->will($this->returnValue('-1 day'));
$schedule->expects($this->any())->method('getStatus')->will($this->returnValue('success'));

$this->_request->expects($this->any())->method('getParam')->will($this->returnValue('test_group'));
$this->_collection->addItem($schedule);

$this->_config->expects($this->once())->method('getJobs')->will($this->returnValue($jobConfig));
Expand Down
Loading

0 comments on commit 752bacc

Please sign in to comment.