Skip to content

[Validator] Fix auto-mapping constraints should not be validated #34694

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
Dec 10, 2019
Merged

[Validator] Fix auto-mapping constraints should not be validated #34694

merged 1 commit into from
Dec 10, 2019

Conversation

ogizanagi
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets #34672
License MIT
Doc PR N/A

As for Traverse, I don't think we should add these "constraints" to the list.

I'm also wondering if it really makes sense to have these annotations as constraints. I think it should rather behave like the GroupSequence annotation to add the info the generic metadata at loading time, but we don't need to rely on the constraints behavior at all.

@ogizanagi ogizanagi mentioned this pull request Nov 28, 2019
Closed
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Nov 28, 2019
ogizanagi
ogizanagi commented Nov 28, 2019
View reviewed changes
src/Symfony/Component/Validator/Mapping/GenericMetadata.php
/**
* @see AutoMappingStrategy
*/
public function getAutoMappingStrategy(): int
Copy link
Contributor Author

@ogizanagi ogizanagi Nov 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Worth adding to the interface (@method to get the deprec), or adding a new AutoMappingMetadataInterface?
AFAIK, implementations all rely on GenericMetadata classes, so might only be an implementation detail.

@ogizanagi
Copy link
Contributor Author

Tested on my side, it solves the issue, no-side effects noticed.

@xabbuh
Copy link
Member

xabbuh commented Nov 29, 2019

Would it not make sense to not let both classes extend Constraint instead?

@ogizanagi
Copy link
Contributor Author

ogizanagi commented Nov 29, 2019 •
edited
Loading

If I understand your suggestion, extending Constraint is required for now to leverage the existing infrastructure to load this annotations & call \Symfony\Component\Validator\Mapping\ClassMetadata::addConstraint.

That's why I mentioned the GroupSequence annotation in the description, which has its own parsing layer in \Symfony\Component\Validator\Mapping\Loader\LoaderInterface implems. We could do the same for these annotations, but we're late to find and implement another syntax to parse for this, right?

@ogizanagi
Copy link
Contributor Author

ping @mpiot & @dunglas

@weaverryan weaverryan mentioned this pull request Dec 6, 2019
Closed
dunglas
dunglas approved these changes Dec 8, 2019
View reviewed changes
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

lgtm

@nicolas-grekas
Copy link
Member

Thank you @ogizanagi.

nicolas-grekas added a commit that referenced this pull request Dec 10, 2019
@nicolas-grekas
bug #34694 [Validator] Fix auto-mapping constraints should not be val…
7049b94 
…idated (ogizanagi)

This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

[Validator] Fix auto-mapping constraints should not be validated

| Q             | A
| ------------- | ---
| Branch?       | 4.4 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | #34672 <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | N/A

As for `Traverse`, I don't think we should add these "constraints" to the list.

I'm also wondering if it really makes sense to have these annotations as constraints. I think it should rather behave like the `GroupSequence` annotation to add the info the generic metadata at loading time, but we don't need to rely on the constraints behavior at all.

Commits
-------

bc53e4b [Validator] Fix auto-mapping constraints should not be validated
@nicolas-grekas nicolas-grekas merged commit bc53e4b into symfony:4.4 Dec 10, 2019
@ogizanagi ogizanagi deleted the fix_validator_automap_annots branch December 10, 2019 11:10
This was referenced Dec 19, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas dunglas approved these changes

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

Assignees
No one assigned
Labels
Bug Status: Reviewed Validator
Projects
None yet
Milestone
4.4
Development

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