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

Fixed PHP_OS check in cron.php #3113

Merged
merged 1 commit into from
Mar 22, 2023

Conversation

elidrissidev
Copy link
Member

@elidrissidev elidrissidev commented Mar 22, 2023

Description (*)

Some improvements were done previously to cron.php in #2380, including a refactor to $isShellDisabled which decides whether or not to use the shell generate the cron schedules and run them.

The problem comes from the condition !str_contains(strtolower(PHP_OS), 'win'), which is the opposite of what it should be and it'll cause the cron events to be dispatched twice from the lines below:

magento-lts/cron.php

Lines 74 to 79 in e2d5fed

if ($isShellDisabled) {
Mage::dispatchEvent('always');
Mage::dispatchEvent('default');
} else {
Mage::dispatchEvent($cronMode);
}

This was likely caused by confusion over the fact that PHP_OS is set to Darwin in macOS (@fballiano's OS), so with the negation operator it's actually resulting in false which is what you'd expect in a non-win system, so I went ahead and checked for that too.

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes cron does run twice because of wrong $isShellDisabled check #3112

Manual testing scenarios (*)

  1. Add a var_dump($isShellDisabled) to cron.php before the try catch block, and observe the value before and after.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

darwindows 😅 what the heck

the problem was only on windows anyway right?

@elidrissidev
Copy link
Member Author

darwindows 😅 what the heck

the problem was only on windows anyway right?

ikr, it was windows all along!

@fballiano fballiano merged commit 7a2acdb into OpenMage:1.9.4.x Mar 22, 2023
fballiano pushed a commit that referenced this pull request Mar 22, 2023
@fballiano fballiano changed the title Fix PHP_OS check in cron.php Fixed PHP_OS check in cron.php Mar 22, 2023
@fballiano
Copy link
Contributor

merged and 20ed.

PS: I'm trying to change the titles because they're used to generate the release notes...

@elidrissidev elidrissidev deleted the fix/cron-os-detection branch March 22, 2023 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cron does run twice because of wrong $isShellDisabled check
3 participants