8000 [FrameworkBundle][Validator] Remove deprecated ConstraintValidatorFactory by ogizanagi · Pull Request #22887 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle][Validator] Remove deprecated ConstraintValidatorFactory #22887

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
Closed
wants to merge 1 commit into from
Closed

[FrameworkBundle][Validator] Remove deprecated ConstraintValidatorFactory #22887

wants to merge 1 commit into from

Conversation

ogizanagi
Copy link
Contributor
@ogizanagi ogizanagi commented May 24, 2017 •
edited
Loading
Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #22905
License MIT
Doc PR N/A

Follows #22905

@chalasr
Copy link
Member
chalasr commented May 24, 2017

I thought we would fully avoid storing instances from the container (i.e. remove the $validators property). Is there much interest to keep it? We did remove a similar one from the ServiceLocator class in #22029

@ogizanagi
Copy link
Contributor Author
ogizanagi commented May 24, 2017 •
edited
Loading

I think keeping this internal cache is cheap and still worth it, but no strong opinion on this.

#22029 was motivated by the fact the memoization was useless when used with the symfony DI container and prevents from using properly non-shared services.
This one can be used with any PSR-11 implementation, which may or may not cache instances (unless it's specified as a PSR-11 requirement?) and we also create instances on the fly.

BTW, this makes me think this implementation can be moved to the Validator component instead of keeping it in the FrameworkBundle, as anyone should be able to use this ContainerConstraintValidatorFactory without using the framework bundle.

EDIT: The PSR-11 specifies:

Two successive calls to get with the same identifier SHOULD return the same value. However, depending on the implementor design and/or user configuration, different values might be returned, so user SHOULD NOT rely on getting the same value on 2 successive calls.

WDYT?

@chalasr
Copy link
Member
chalasr commented May 24, 2017

and we also create instances on the fly.

8000 fair enough to me!

@xabbuh xabbuh added this to the 4.0 milestone May 24, 2017
@ogizanagi ogizanagi mentioned this pull request May 25, 2017
Merged
@ogizanagi
Copy link
Contributor Author
ogizanagi commented May 25, 2017 •
edited
Loading

See #22905 about moving the class in the component (and keeping only one deprecation rather than the current three in 3.3)

stof added a commit that referenced this pull request May 26, 2017
@stof
feature #22905 [FrameworkBundle][Validator] Move the PSR-11 factory t…
cf60f6d 
…o the component (ogizanagi)

This PR was merged into the 3.3 branch.

Discussion
----------

[FrameworkBundle][Validator] Move the PSR-11 factory to the component

| Q             | A
| ------------- | ---
| Branch?       | 3.3 <!-- see comment below -->
| Bug fix?      | no
| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | yes <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #22887 (comment) <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

Instead of the 3 following deprecations:
 * The `ConstraintValidatorFactory::$validators` and `$container` properties
   have been deprecated and will be removed in 4.0.
 * Extending `ConstraintValidatorFactory` is deprecated and won't be supported in 4.0.
* Passing an array of validators or validator aliases as the second argument of
   `ConstraintValidatorFactory::__construct()` is deprecated since 3.3 and will
   be removed in 4.0. Use the service locator instead.

I'd suggest simply deprecating the FrameworkBundle's class in favor of using a new `ContainerConstraintValidatorFactory`. To me, there is no reason anyone using the validator component without the framework bundle cannot use this PSR-11 compliant implementation, nor I see a reason to make it final.

Commits
-------

68c1917 [FrameworkBundle][Validator] Move the PSR-11 factory to the component
@ogizanagi ogizanagi changed the title [FrameworkBundle][Validator] Remove deprecated ConstraintValidatorFactory $validators argument [FrameworkBundle][Validator] Remove deprecated ConstraintValidatorFactory May 26, 2017
@ogizanagi
Copy link
Contributor Author

On hold (waiting for 3.3 branch to be merged in upper branches to get #22905).

@chalasr
Copy link
Member
chalasr commented May 27, 2017

This can be closed

@ogizanagi ogizanagi deleted the rm_deprec/fwb_validator branch May 27, 2017 14:21
@ogizanagi
Copy link
Contributor Author

@chalasr : Not exactly, as the changelog is still missing an update. That's why I kept this opened in case it was not entirely done during the merge. Anyway, see #22926 then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
BC Break FrameworkBundle Status: Needs Review Validator
Projects
None yet
Milestone
4.0
Development

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