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

Replace php date functions, Varien_Date with Carbon date library #4463

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Jan 8, 2025

Description (*)

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

#3594 (comment)

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 #4454, that should be merge first.

@sreichel sreichel changed the title Replace php date functions, Varien_Date with Carbon library Replace php date functions, Varien_Date with Carbon date library Jan 8, 2025
@fballiano
Copy link
Contributor

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)

@ajbonner
Copy link

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
Copy link
Contributor

Hanmac commented Jan 10, 2025

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
Copy link
Contributor Author

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants