Skip to content

[Validator] Pre-check constraint validator dependencies #28644

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 3, 2018
Merged

[Validator] Pre-check constraint validator dependencies #28644

merged 1 commit into from
Oct 3, 2018

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Sep 30, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #25865
License MIT
Doc PR symfony/symfony-docs#...

chalasr
chalasr reviewed Sep 30, 2018
View reviewed changes
src/Symfony/Component/Validator/CHANGELOG.md Outdated
* deprecated using the `Email` constraint without `egulias/email-validator`
* deprecated using the `Expression` constraint without `symfony/expression-language`
* deprecated using the `Language` constraint without `symfony/intl`
* deprecated using the `Locale` constraint without `symfony/intl`
Copy link
Member

Choose a reason for hiding this comment

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

could we gather them together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

xabbuh
xabbuh reviewed Sep 30, 2018
View reviewed changes
UPGRADE-4.2.md
@@ -216,4 +216,6 @@ Validator
* The component is now decoupled from `symfony/translation` and uses `Symfony\Contracts\Translation\TranslatorInterface` instead
* The `ValidatorBuilderInterface` has been deprecated and `ValidatorBuilder` made final
* Deprecated validating instances of `\DateTimeInterface` in `DateTimeValidator`, `DateValidator` and `TimeValidator`. Use `Type` instead or remove the constraint if the underlying model is type hinted to `\DateTimeInterface` already.
* Using the `Bic` constraint without `symfony/intl` is deprecated
* Using the `Bic`, `Country`, `Currency`, `Language` and `Locale` constraints without `symfony/intl` is deprecated
Copy link
Member

Choose a reason for hiding this comment

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

missing final dots here and below (same below for other upgrade files)

Copy link
Contributor Author

@ro0NL ro0NL Sep 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

i think we only do that for multiple sequences...

#28644 (diff)

Copy link
Member

Choose a reason for hiding this comment

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

I think our markdowns are just inconsistent regarding final dots.
Sometimes we add it, sometimes not, mostly aligning with previous entries...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in general i think fabbot.io should take care of this yes. I.e. same for indenting, upper-/lowercase first..

xabbuh
xabbuh reviewed Sep 30, 2018
View reviewed changes
src/Symfony/Component/Validator/Constraints/BicValidator.php
if (class_exists(Intl::class)) {
$validCountryCode = isset(Intl::getRegionBundle()->getCountryNames()[substr($canonicalize, 4, 2)]);
} else {
$validCountryCode = ctype_alpha(substr($canonicalize, 4, 2));
// throw new LogicException('The "symfony/intl" component is required to use the Bic constraint.');
Copy link
Member

Choose a reason for hiding this comment

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

We should trigger a deprecation here for now (at least when the passed constraint is a subclass of the Bic constraint).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldnt we assume they call the parent constructor?

Copy link
Member

Choose a reason for hiding this comment

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

agreed, sounds like a very edge case afterall

Copy link
Member

Choose a reason for hiding this comment

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

fair enough, but I think we should still throw the exception in 5.0

@ro0NL ro0NL mentioned this pull request Sep 30, 2018
Merged
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Oct 1, 2018
@nicolas-grekas
Copy link
Member

Thank you @ro0NL.

@nicolas-grekas nicolas-grekas merged commit 0544985 into symfony:master Oct 3, 2018
nicolas-grekas added a commit that referenced this pull request Oct 3, 2018
@nicolas-grekas
feature #28644 [Validator] Pre-check constraint validator dependencie…
3b604ff 
…s (ro0NL)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Validator] Pre-check constraint validator dependencies

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #25865
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Commits
-------

0544985 [Validator] Pre-check constraint validator dependencies
@ro0NL ro0NL deleted the constraints branch October 3, 2018 06:41
This was referenced Nov 3, 2018
Closed
Merged
@ro0NL ro0NL mentioned this pull request May 29, 2019
Merged
nicolas-grekas added a commit that referenced this pull request May 29, 2019
@nicolas-grekas
minor #31680 [Validator] Remove fallback dependency checks (ro0NL)
03797a1 
This PR was merged into the 5.0-dev branch.

Discussion
----------

[Validator] Remove fallback dependency checks

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

See #28644

Commits
-------

aa84cfd     [Validator] Remove fallback dependency checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@xabbuh xabbuh xabbuh approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Deprecation Feature Status: Reviewed Validator
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
5 participants
@ro0NL @nicolas-grekas @xabbuh @chalasr @carsonbot