[BC-Backport] Fixed incorrect datetime in block, ref #1525

[sreichel]

Description (*)

New PR for #2854

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]

Unit Test Results

7 tests   5 ✔️  0s ⏱️
1 suites  2 💤
1 files    0 ❌

Results for commit 43a91fb.

[Flyingmana]

Replace #1525 #2940

Just as a hint, the Name of the PR is used for the merge commit and the changelog generation

[fballiano]

@Flyingmana true, when I merge I always fix the commit description 😊

[fballiano]

let's please decide what to do with this, the readme has a conflict and it needs to be rebased.
if this is BC, since it's 4 months old, do we still need it? should it be on all branches since it's probably "broken" on 19.5.0-rcX?

[sreichel]

Y, it should be on all branches,

[fballiano]

then can you rebase it anyway? 1.9.4.x is a dead branch.

[sreichel]

4 month ago 1.9.4.x was not EOL

You changed the rules. (please) deal with it,

[fballiano]

we went thru a very long RFC process which was participated by many people and you've the courage to say I changed the rules. wow.

[fballiano]

I was trying to rebase but I can't wrap my head around fixing the conflicts with the main branch how it is today

so if nobody wants to pick it up I guess the only option is to close it

[sreichel]

by many people

So someome who carried this RFC should take care of old PRs to get merged.

[fballiano]

maybe you didn't notice that my RFC got closed and the approved one is a totally different one. weird, if only you participated.

[fballiano]

and btw I've rebased all of the PR myself alone, but for the few ones that I felt I couldn't fix, I stopped or closed the PR if it was abandoned. I mean, literally what else can I do?
instead, you're active, it would maybe take 10 minutes for you because you are very skilled, but you prefer to argue forever over nothing.

ah, maybe you don't remember that for something like a year I worked with the "v19 is the main branch" fixing all of the conflicts and cherry picking everything myself alone because that was the rule and I got told "if you want change, create an RFC". what, the, actual, heck, the courage you have to talk this way.

going to bed, good night to everybody reading this pointless discussion.

[sreichel]

it would maybe take 10 minutes for you because you are very skilled

It takes you the same 10 minutes, because you are very skilled too.

I spent time to review PR #1525. I rewrote it to make it backward-compatible, I tested it, What else should i do?

With propper reviews this was no b/c-breaking change,

[fballiano]

It takes you the same 10 minutes, because you are very skilled too.

I spent more but I wasn't sure of what to do. I'm far less skilled than many of the contributors here, but what I have for sure is that I work like a jackass

anyway, I'll try to do that, hoping not to break it

[fballiano]

In the meanwhile I'm adding the original patch file, as a backup
2940.diff.txt

[fballiano]

this is for sure a mistake because both $time and $date are not used since it returns ''

[sreichel]

Right. It should be

} else {
    return '';
}

[fballiano]

that's more or less what I committed

[sreichel]

You removed that part ... $date is used in return statement.

            $time = strtotime($date);
            if ($time) {
                $date = $locale->date($time, null, null, $useTimezone);
            }

if ($showTime) { changed ...

This PR was NOT for main branch. #1525 was applied to main only cause of changing method signature. It should have been reverted first and replaced by this one. ATM this PRs looks not correct anymore.

[fballiano]

I understood that this PR was going to unify the same code on all branches, so main then ported to v19 and next, no?

[fballiano]

You removed that part ... $date is used in return statement.

            $time = strtotime($date);
            if ($time) {
                $date = $locale->date($time, null, null, $useTimezone);
            }

I didn't understand, from the screenshot both $time and $date are not returned

[sreichel]

Its not on your screenshot, but $date is used in last line of that method. (return $date->....)

[fballiano]

I sencerely can't find that in the original diff https://github.com/OpenMage/magento-lts/files/11548215/2940.diff.txt it doesn't seem to me that that variable is needed

[sreichel]

Please check https://github.com/OpenMage/magento-lts/pull/1525/files and https://github.com/OpenMage/magento-lts/pull/2854/files.

I just missed that else statement.

[fballiano]

If I'm not mistaken, the purpose of this PR was to reimplement #1525 in order to backport it to v19.

But since nowadays v19 is frozen, it seems to me this PR won't be backported to v19 anyway.

Also, because of the 1.9.4.x->main rebase we had a few months ago, I think I broke (my fault) this PR (because of the conflict) and I cannot bring it back to full functioning.

Also, this PR has a branch in our main repo, which shouldn't be there.

So, all I all I think we should close it.
@kiatng @elidrissidev @Flyingmana @ADDISON74 what do you think?

[github-actions]

Test Results

7 tests   7 ✔️  0s ⏱️
3 suites  0 💤
1 files    0 ❌

Results for commit cac07b5.

[kiatng]

When I tested it some months back, I actually prefer this PR over my old one. However, I need to retest it again to be sure.

[fballiano]

@kiatng the author said this: #2940 (comment)

[kiatng]

But since nowadays v19 is frozen, it seems to me this PR won't be backported to v19 anyway.

This PR add a new method formatTimezoneDate() to avoid BC, but v19 is frozen, so I agree that this PR can be closed.

[sreichel]

and I cannot bring it back to full functioning.

Pls see #2854

[sreichel]

Closed as suggested. Thanks f...