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

Added php-extension "intl" as requirement, updated composer #2437

Merged
merged 5 commits into from
Sep 8, 2022

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Aug 16, 2022

Description (*)

  • Magento installer checks for various php exentsion ... added ext-intl (and ext-libxml)
  • added missing extensions from install.xml to composer.json

Related Pull Requests

  1. See Moved phpseclib, mcrypt_compat, Cm_RedisSession, Cm_Cache_Backend_Redis and Pelago_Emogrifier to composer #2411
  2. See Used transliterator_transliterate to generate url_key #1631

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: Core Relates to Mage_Core Component: Install Relates to Mage_Install Component: lib/* Relates to lib/* composer Relates to composer.json labels Aug 16, 2022
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@sreichel sreichel force-pushed the required-extensions branch from b0bbcc4 to e1e807d Compare August 16, 2022 23:05
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@justinbeaty
Copy link
Contributor

justinbeaty commented Aug 16, 2022

I like the simplification in the code and the removal of Net_IDNA, but I am not sure I agree with ext-intl being a required module. I think we should have symfony/polyfill-intl-idn required and put ext-intl as a suggested module.

The reason is that if I check on my server, I don't have php-intl installed by default on Ubuntu 20.04. It's no problem to add for me, but it might catch people off guard.

@fballiano
Copy link
Contributor

I tend to agree with @justinbeaty, seems something for a major release, I'll think about it

@sreichel
Copy link
Contributor Author

sreichel commented Aug 16, 2022

The reason is that if I check on my server, I don't have php-intl installed by default on Ubuntu 20.04. It's no problem to add for me, but it might catch people off guard.

I understand your concern, but then we should not even consider to raise minimum php version to 7.3 ... "because it could break something" (does not fit server setup anymore). Even on shared hosts (with Plesk/cPanel/...) you can enable/disable extensions with some clicks.

@fballiano
Copy link
Contributor

Yes but it shouldnt just be a patch level release. And I’m confused now, for some things we’re extremely conservative and for some other we’re the opposite.

@justinbeaty
Copy link
Contributor

Well I agree we need to move forward and drop old PHP versions, but two things:

  1. ext-intl was never required before
  2. this is different than a PHP version that’s EOL.

But either way, there’s a simple fix which is to include the polyfill and is why I think it’s better to just include it for now.

@sreichel
Copy link
Contributor Author

  1. ext-intl was never required before

PHP7 was never required before. ;)

And I’m confused now, for some things we’re extremely conservative and for some other we’re the opposite.

It seems to be opinion based. I'd like to avoid BC breaking changes in code, but require a higher php-version or in this case another php-extionsion should not be a problem. I'd just (contact my hoster to) update requirements.

@sreichel
Copy link
Contributor Author

sreichel commented Aug 17, 2022

Yes but it shouldnt just be a patch level release.

We have no semantic versioning. PHP5-support has been dropped with v19.4.4 (if i'm right).

@justinbeaty
Copy link
Contributor

  1. ext-intl was never required before

PHP7 was never required before. ;)

I still don’t think it’s a good comparison because when we supported PHP 5, PHP 7 was not required, it was optional. Only when PHP 5 was EOL did we require PHP 7.

It would be more like this situation if there was a php-intl2 extension that we now require because php-intl1 is deprecated.

In the end it doesn’t bother me either way as I follow this GitHub and am a composer user. I just think it’s a cheap fix to not make this breaking change by including the polyfill.

@fballiano
Copy link
Contributor

We have no semantic versioning. PHP5-support has been dropped with v19.4.4 (if i'm right).

in issue 14 of the internal organization repository it seems to me that we're using semver, also

* @link https://semver.org/
links to semver.org website.

I didn't decide that, to me semver is too strict and everybody here knows i'm more for the disruptive changes, but since i've been blocked more than once because of BC, now I really am getting confused and I don't know what I should suggest anymore.

@sreichel
Copy link
Contributor Author

I got your point, but again ... if you want to use latest OM release, you should update your environment. Another example ... Magento2 has dropped MySQL-based search with 2.4.3 and everyone had to upgrade their servers (to install elastic-search) OR stay on previous release-line, that get only security fixes for another year(?).

We have no different release-lines, so we have to make a cut somewhere ....

(Just my point of view)

@sreichel
Copy link
Contributor Author

sreichel commented Aug 17, 2022

seems to me that we're using semver

No. We never had major/minor/hotfix-releases ... even README says, we do NOT follow semver.

now I really am getting confused and I don't know what I should suggest anymore.

Why? You`re doing great. :) We`re just about dicussing details :)

@justinbeaty
Copy link
Contributor

Agreed that we have no semblance of semver. I wish we would at least bump the minor version on larger releases (like magento team did) instead of BC breaks in patch versions. But anyway that’s another topic I guess.

I also get your point @sreichel, we’ve done plenty of breaking changes much larger than this. If you feel strongly about it, then I can agree with your changes. One less package in vendor anyway.

@sreichel
Copy link
Contributor Author

sreichel commented Aug 17, 2022

I wish we would at least bump the minor version on larger releases

Agree. It should maybe combined with #2413 and release version could change to 19.5.x / 20.1.x ...

@Flyingmana?

@fballiano
Copy link
Contributor

Agree. It should maybe combined with #2413 and release version could change to 19.5.x / 20.1.x ...

that would be great, I'd just do a release first since the last one had a couple of issues, then go on with this one :-)

tobihille
tobihille previously approved these changes Aug 19, 2022
@elidrissidev
Copy link
Member

elidrissidev commented Sep 2, 2022

Since not everyone uses composer, I think this PRs should be left for a new major version, or at least v20, until the RFC for versioning is set in stone. Not against the change, just my personal take.

@Flyingmana
Copy link
Contributor

I wish we would at least bump the minor version on larger releases

Agree. It should maybe combined with #2413 and release version could change to 19.5.x / 20.1.x ...

@Flyingmana?

We dont have a process or rule for minor releases is all I can say to this

I got your point, but again ... if you want to use latest OM release, you should update your environment. Another example ... Magento2 has dropped MySQL-based search with 2.4.3 and everyone had to upgrade their servers (to install elastic-search) OR stay on previous release-line, that get only security fixes for another year(?).

We have no different release-lines, so we have to make a cut somewhere ....

(Just my point of view)

we do have different release lines at the moment, v19 and v20 with v19 which should primarily only get bugfixes. and v20 for most of the other changes at the moment (although a v22 is justified by now)
Or did I misunderstood what you mean here?

@sreichel
Copy link
Contributor Author

sreichel commented Sep 4, 2022

@Flyingmana question was to raise minor version or not, when we make php7.3/intl required.

fballiano
fballiano previously approved these changes Sep 5, 2022
addison74
addison74 previously approved these changes Sep 5, 2022
@fballiano
Copy link
Contributor

Let’s fix conflicts and merge

@sreichel
Copy link
Contributor Author

sreichel commented Sep 5, 2022

I'll wait for #2413 (b/c conflicts with composer.lock)

@sreichel sreichel dismissed stale reviews from addison74, fballiano, and tobihille via 17f67f9 September 7, 2022 22:14
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

addison74
addison74 previously approved these changes Sep 7, 2022
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@fballiano fballiano merged commit 5974333 into 1.9.4.x Sep 8, 2022
@fballiano fballiano deleted the required-extensions branch September 8, 2022 09:25
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

Unit Test Results

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

Results for commit 5974333. ± Comparison against base commit a5541ff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Relates to Mage_Core Component: Install Relates to Mage_Install Component: lib/* Relates to lib/* composer Relates to composer.json documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants