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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace strftime with date #2934

Merged
merged 1 commit into from
Jan 16, 2023
Merged

Replace strftime with date #2934

merged 1 commit into from
Jan 16, 2023

Conversation

luigifab
Copy link
Collaborator

@luigifab luigifab commented Jan 14, 2023

Description

This PR replace strftime because it is deprecated since PHP 8.1. Ref #1812.

Hope it's good 馃馃徑.
Partially tested with PHP 7.2, 7.3, 7.4, 8.0, 8.1, 8.2 with OpenMage 20.0.18.

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

@github-actions github-actions bot added Component: CatalogIndex Relates to Mage_CatalogIndex Component: Cron Relates to Mage_Cron Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Log Relates to Mage_Log labels Jan 14, 2023
@luigifab luigifab added the PHP 8.1 Related to PHP 8.1 label Jan 14, 2023
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.

LGTM

app/code/core/Mage/Cron/Model/Schedule.php Show resolved Hide resolved
$this->setCreatedAt(strftime('%Y-%m-%d %H:%M:%S', time()));
$this->setScheduledAt(strftime('%Y-%m-%d %H:%M', (int)$time));
$this->setCreatedAt(date(Varien_Db_Adapter_Pdo_Mysql::TIMESTAMP_FORMAT));
$this->setScheduledAt(date('Y-m-d H:i:00', (int)$time));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$this->setScheduledAt(date('Y-m-d H:i:00', (int)$time));
$this->setScheduledAt(date('Y-m-d H:i', (int)$time));

Copy link
Collaborator Author

@luigifab luigifab Jan 14, 2023

Choose a reason for hiding this comment

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

This is strange to put a value without seconds for me, even if this works.

php > echo date('Y-m-d H:i');
2023-01-14 20:49
php > echo date('Y-m-d H:i:00');
2023-01-14 20:49:00

Copy link
Contributor

Choose a reason for hiding this comment

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

I just compared before/after and this is the only change i found in output. (y, its strange ;) )

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you agree to keep current behavior?

Copy link
Member

Choose a reason for hiding this comment

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

Either way both will save the value with 00 seconds

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have an opinion on this so as you all prefer :-)

@elidrissidev
Copy link
Member

Any chance you could also include gmstrftime to close #2030?

@luigifab
Copy link
Collaborator Author

This is another story.

@fballiano fballiano merged commit a7fdf56 into OpenMage:1.9.4.x Jan 16, 2023
@fballiano
Copy link
Contributor

merged and cherrypicked to v20

@luigifab luigifab deleted the strftime branch January 16, 2023 11:16
justinbeaty pushed a commit to justinbeaty/magento-lts that referenced this pull request Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: CatalogIndex Relates to Mage_CatalogIndex Component: Cron Relates to Mage_Cron Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Log Relates to Mage_Log PHP 8.1 Related to PHP 8.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants