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 incorrect datetime in block. #1525

Merged
merged 1 commit into from
Aug 4, 2021
Merged

Conversation

kiatng
Copy link
Contributor

@kiatng kiatng commented Apr 6, 2021

Description (*)

This PR fixes 2 issues:

  1. Incorrect datetime in backend customer page, see issue Strange date time issues in customer view #466.
  2. Enable the ability to formatDate() in the block without converting to store timezone. A use case is when a datetime is stored in the DB in local timezone (or a date such as birthdate, which is independent of timezone), the method formatDate() would convert the datetime and couldn't be used for rendering. This PR fixes that. Now, the same method can be used.

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes Strange date time issues in customer view #466

Manual testing scenarios (*)

After:
image

Questions or comments

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)

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Core Relates to Mage_Core labels Apr 6, 2021
@addison74
Copy link
Contributor

I do not agree changing the code by replacing if/else sequences with ternary operator ?. The code is just fine as it is the only necessary change should be fixing the issue.

Although I understand that you want a compression of the code but it is not beneficial for the reader. It is much easier for me personally to see if/else sequences then ?. If we agree to such changes then we should do so throughout Magento, otherwise we lose the rules initially set.

@fballiano
Copy link
Contributor

I also don't like the 1 line functions, but anyways we need this merged asap

@Flyingmana Flyingmana merged commit e69b326 into OpenMage:20.0 Aug 4, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2021

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
6 runs  +4  6 ✔️ +4  0 💤 ±0  0 ❌ ±0 

Results for commit e69b326. ± Comparison against base commit 42988a6.

@sreichel
Copy link
Contributor

Is this a BC breaking change?

@kiatng kiatng deleted the block_datetime branch August 17, 2022 01:24
@kiatng
Copy link
Contributor Author

kiatng commented Aug 17, 2022

Is this a BC breaking change?

I think so, due to addition of new param $useTimezone in public function formatDate($date = null, $format = Mage_Core_Model_Locale::FORMAT_TYPE_SHORT, $showTime = false, $useTimezone = true). Existing method without $useTimezone that overrides it will run into PHP exceptions.

@fballiano fballiano mentioned this pull request Aug 23, 2022
@sreichel
Copy link
Contributor

IMHO ... this change to formatDate() method should be reverted.

I'd add a new method that covers $useTimezone = true parameter and can be used to fix the issue.

@sreichel sreichel self-assigned this Dec 16, 2022
@fballiano fballiano mentioned this pull request Dec 25, 2022
sreichel added a commit to sreichel/magento-lts that referenced this pull request Oct 2, 2024
kiatng added a commit that referenced this pull request Oct 15, 2024
* backport, ref #1525 #2940

* test coverage for formatTimezoneDate() 100%

* Minor change

* phpcs

* Minor fix

* rector

* Fixed tests

* phpstan l5 fix

* Fixed test .... hour w/o leading zero

---------

Co-authored-by: Ng Kiat Siong <kiatsiong.ng@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Core Relates to Mage_Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants