-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Refactor datetime class #26538
Refactor datetime class #26538
Conversation
Hi @Tjitse-E. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
@joni-jones the code styling issues are resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the constructor of this class does a call to the DB via the getConfigTimezone() method
doing this calculation in constructor also violates the best practice of using constructors only for assignments so for me, this contribution is very appreciated
Hi @aleron75, thank you for the review.
|
✔️ QA Passed |
Hi @Tjitse-E, thank you for your contribution! |
Description (*)
When creating a console command that uses
\Magento\Framework\Stdlib\DateTime\DateTime
, the integration test suite will fail. This is because the constructor of this class does a call to the DB via thegetConfigTimezone()
method:I propose that we refactor this class, so this call is done in the
getGmtOffset()
method. I don't see any downsides to this, but let me know if I've missed any.Fixed Issues (if relevant)
Related issue: baldwin-agency/magento2-module-url-data-integrity-checker#2
Manual testing scenarios (*)
\Magento\Framework\Stdlib\DateTime\DateTime
in the console command class via DI.Questions or comments
I thought about leaving
private $_offset
intact. But it isn't used anywhere else than ingetGmtOffset()
, so i've removed this.Contribution checklist (*)