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, ref #1525 #2854

Closed
wants to merge 3 commits into from
Closed

Fixed incorrect datetime in block, ref #1525 #2854

wants to merge 3 commits into from

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Dec 26, 2022

Description (*)

#1525 was added to v20 only, because it was a BC breaking change. This PR solves that issue w/o changing existing methods signature. (code from 1525)

Not sure about this change ... changing ...

if (!($date instanceof Zend_Date) && $date && !strtotime($date)) {
    return '';
}
if (is_null($date)) {
    $date = Mage::app()->getLocale()->date(Mage::getSingleton('core/date')->gmtTimestamp(), null, null);

Related Pull Requests

  1. See Fixed incorrect datetime in block. #1525

Fixed Issues (if relevant)

  1. Closes Backported #1525 #2850

Manual testing scenarios (*)

  1. see related PR
  2. ...

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: Adminhtml Relates to Mage_Adminhtml Component: Core Relates to Mage_Core documentation labels Dec 26, 2022
@sreichel sreichel requested a review from kiatng December 26, 2022 21:08
@sreichel sreichel marked this pull request as draft December 26, 2022 22:35
@sreichel sreichel marked this pull request as ready for review December 27, 2022 01:10
@sreichel
Copy link
Contributor Author

possibly related to #2739

@fballiano
Copy link
Contributor

@kiatng what do you think about this?

@kiatng
Copy link
Contributor

kiatng commented Dec 28, 2022

This is a better fix of the original issue.

Is this going to be in v20? If it is and if there are custom code in the wild that uses $useTimezone in formatDate(), it'll produce unexpected result. But BC in v20 is tolerable. Is it worthwhile to add a note in README for BC in v20?

@kiatng
Copy link
Contributor

kiatng commented Dec 28, 2022

Test result comparing original v19 formatDate($date) and formatTimezoneDate($date):

  1. $date = '2022-12-28 13:00:00';: both return "28/12/2022 21:00", correctly converted to local time.
  2. $date = null;: both return "28/12/2022 03:04" (GMT time)
  3. $date = '';: both return "28/12/2022 11:26", local time.
  4. $date = '2022-12-28 13:00:00'; $date = new Zend_Date($date);: both return "28/12/2022 13:00", correct because timezone in Zend_Date was not set.
  5. $date = '2022-12-28 13:00:00'; $date = strtotime($date);: formatDate() -> "", formatTimezoneDate() -> "28/12/2022 21:00". I think result of formatTimezoneDate() is preferable.

@sreichel
Copy link
Contributor Author

@kiatng thanks. Would be nice to had a unit-test here.

Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

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

Should not have serious effect in v20.

@fballiano
Copy link
Contributor

so the idea is to port it to v20 right? otherwise it would be actually worse to maintain

@sreichel
Copy link
Contributor Author

sreichel commented Dec 29, 2022

Add @kiatng fix to v19 and port/replace it to v20 was my plan ... ❔

@sreichel sreichel requested a review from Flyingmana December 30, 2022 00:49
@sreichel sreichel changed the title Replacement for #1525 Fixed incorrect datetime in block, ref #1525 Jan 5, 2023
@sreichel sreichel closed this by deleting the head repository Jan 8, 2023
@addison74 addison74 reopened this Jan 9, 2023
@sreichel sreichel closed this Jan 10, 2023
@addison74
Copy link
Contributor

addison74 commented Jul 11, 2023

Closing this PR was a wrong decision. It was a better fix for that issue.

@sreichel
Copy link
Contributor Author

sreichel commented Jul 11, 2023

@addison74 there is a follow-up PR. (#2940)

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 documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants