rector: Replace php date functions, `Varien_Date` with Carbon date library

[sreichel]

Description (*)

Use https://carbon.nesbot.com/docs/.

https://github.com/OpenMage/magento-lts/pull/3594#issuecomment-2577704973

I think its a bit more then "YetAnotherDateTime".

Currently we have ...

• basic date funtions (mktime(), date(), ...)
• Varien_Date
• Zend_Date
• Mage_Locale

This could be unified.

Carbon provides some usefull date operation (compare dates, add or subsract seconds/hours/days) that make writing code easier. Zend_Date, DateTime etc have similar, but its all mixed up.

(Plus. Rectors supports t. :) )

@Hanmac finally it should replace Zend_Date at all, but i'd start with Varien_Date etc first.

Edit: PR is based on https://github.com/OpenMage/magento-lts/pull/4454, that should be merge first.

[fballiano]

carbon requires all these components

"carbonphp/carbon-doctrine-types": "<100.0",
"psr/clock": "^1.0",
"symfony/clock": "^6.3 || ^7.0",
"symfony/polyfill-mbstring": "^1.0",
"symfony/translation": "^4.4.18 || ^5.2.1|| ^6.0 || ^7.0"

dependencies growth should be taken in deep consideration.

I agree with @Hanmac https://github.com/OpenMage/magento-lts/pull/3594#issuecomment-2577704973

[ajbonner]

Aside from dependencies performance should also be considered. Zend_Date / Locale handling is extremely slow. There ought to be significant benefits getting away from those, but would be interesting to quantify effects.

[Hanmac]

Aside from dependencies performance should also be considered. Zend_Date / Locale handling is extremely slow. There ought to be significant benefits getting away from those, but would be interesting to quantify effects.

The only interesting part of Zend is the "Additional Data", like postalCodeData and telephoneCodeData

[sreichel]

carbon requires all these components

"carbonphp/carbon-doctrine-types": "<100.0",
"psr/clock": "^1.0",
"symfony/clock": "^6.3 || ^7.0",
"symfony/polyfill-mbstring": "^1.0",
"symfony/translation": "^4.4.18 || ^5.2.1|| ^6.0 || ^7.0"

dependencies growth should be taken in deep consideration.

I agree with @Hanmac #3594 (comment)

We have added https://packagist.org/packages/pelago/emogrifier for single usage. That requires +2

• sabberworm/php-css-parser
• symfony/css-selector

Now we are talking about a complete replacement of Varien_Date (later Zend_Date) adding +2

• symfony/clock
• symfony/translation
• ... psr/clock is a sub-dependency and symfony/polyfill-mbstring is already installed

Just to mention ...

Zend_Date / Locale handling is extremely slow

At the moment it is about ro replace Varien_Date. When it comes to Zend_Date / Locale handling i'll provide some tests. (btw. DDEV+XHGUI is easy to use. Feel free.)

The only interesting part of Zend is the "Additional Data", like postalCodeData and telephoneCodeData

Both are part of Zend_Locale. "telephoneCodeData" was updated last in 2013 and isnt up to date. There are better libraries if needed. E.g. https://github.com/giggsey/libphonenumber-for-php

I'd be happy if we can get away from outdouted code. ZF1Future does its best to keep it alive, but it is outdated. There are no successors for Zend_Date/Zend_Locale in Zend2/lamininas.

[fballiano]

We have added https://packagist.org/packages/pelago/emogrifier for single usage. That requires +2

we didn't add emogrifier, it was a part of magento since way before, when I worked on it we just updated it.

[sreichel]

Nitpicking?

We replaced bundled Emogrifier with updated composer-version - accecpting install additional dependencies w/o worriers.

Now you complain about 2 other dependencies - to make a much bigger step?

[sreichel]

@fballiano

As long as I am blocked by you, i'd like you to not comment on any of my PRs/issues/whatever here! Thank you

[fballiano]

Nitpicking?

We replaced bundled Emogrifier with updated composer-version - accecpting install additional dependencies w/o worriers.

it's not nitpicking, we couldn't remove that because it would be BC and we chose to upgrade it, correctly.

that doesn't mean that you should add dependencies for everything.

this is a constructive criticism, growth in dependencies should be considered, that's the only thing I said, and you got defensive.

[sonarqubecloud[bot]]

Quality Gate failed

Failed conditions
8 Security Hotspots
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[sonarqubecloud[bot]]

Quality Gate failed

Failed conditions
2 Security Hotspots
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[sonarqubecloud[bot]]

Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarQube Cloud

[joshua-bn]

I am confused by the description regarding "Rector". Was this (or is this) being done with a Rector? It should be doable and not that difficult to do with Rector or nikic/php-parser

[sreichel]

Most changes where made with rector.

[joshua-bn]

Most changes where made with rector.

If you're making changes with Rector or some other tool, it's helpful to see that in the PR. I review that first to make sure the logic is correct. If you have 1000 files that are changed with Rector, nobody is going to review all of them.