Skip to content

[Form] Remove hard dependency on symfony/intl #40298

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

Merged
merged 1 commit into from
Feb 25, 2021
Merged

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Feb 24, 2021

Q A
Branch? 5.x (or 6.0)
Bug fix?
New feature? no
Deprecations? yes
Tickets Fix #39596
License MIT
Doc PR symfony/symfony-docs#15026

This was voted down in 2018 (#29229) and will revert #29720 by @chalasr. I reopen it because the Form component is way less dependent on Intl component now.

Im hesitant if we should do this in 5.x or 6.0. If a user don't have symfony/intl installed, they will get an error in runtime. That is something that speaks for doing it in 6.0.

Could I get some opinions?

TODO

  • Update UPGRADE-x.x.md

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I'm fine doing it in 5.x personally.
Of course, this should be documented in CHANGELOG+UPGRADE files, in the doc, and the website-skeleton also should be updated (it has explicit "intl" as a dep since 2018).

@Nyholm
Copy link
Member Author

Nyholm commented Feb 24, 2021

The PR is updated and documentation is added.

@@ -47,6 +48,10 @@ public function configureOptions(OptionsResolver $resolver)
$input = $options['input'];

if ($options['intl']) {
if (!class_exists(Intl::class)) {
throw new LogicException(sprintf('The "symfony/intl" component is required to use "%s" with option "intl=true". Try running "composer require symfony/intl".', static::class));
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this exception message is different.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

symfony/intl is listed in the website-skeleton since 2018

@fabpot
Copy link
Member

fabpot commented Feb 25, 2021

Thank you @Nyholm.

@fabpot fabpot merged commit 304980e into symfony:5.x Feb 25, 2021
@Nyholm
Copy link
Member Author

Nyholm commented Feb 25, 2021

Thank you for merging

@Nyholm Nyholm deleted the form-intl branch February 25, 2021 07:36
Nyholm added a commit that referenced this pull request Feb 25, 2021
… (wouterj)

This PR was merged into the 4.4 branch.

Discussion
----------

[TwigBridge] Install symfony/intl to run tests on Travis

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | bi
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

The hard dependency on `symfony/intl` was removed from the Form component in 5.3-dev (#40298). I suggest to add the explicit dev dependency on TwigBridge on 4.4 already.

Commits
-------

b297045 [TwigBridge] Install symfony/intl to run tests on Travis
@fabpot fabpot mentioned this pull request Apr 18, 2021
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.

[Form] Remove dependency on symfony/intl
4 participants