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
[Validator] Dump Valid constraints on debug command #48840
base: 5.4
Are you sure you want to change the base?
Conversation
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
ea6797c
to
2dc3a38
Compare
Hey! I think @loic425 has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
if (CascadingStrategy::CASCADE === $metadata->getCascadingStrategy()) { | ||
$constraint = new Valid([], [], null, TraversalStrategy::IMPLICIT === $metadata->getTraversalStrategy()); | ||
$data[] = [ | ||
'class' => Valid::class, | ||
'groups' => [$constraint::DEFAULT_GROUP], | ||
'options' => $this->getConstraintOptions($constraint), | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, @macintoshplus thanks for your this PR! It's working great! But are we sure only Valid constraints have the CascadingStrategy::CASCADE? For example, if you create custom constraints that extend the Valid constraint you gonna have this CascadingStrategy::CASCADE, but you don't want to add here the Valid constraint but the custom one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true, if you extend Valid
annotation without changing anything, the behavior is the same as using the Valid
constraint. And, your custom constraint is changed to Valid
on debug command. The debug command cannot know the original constraint class name because Symfony has removed it.
See the code https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/Validator/Mapping/GenericMetadata.php#L145-L155
But if you add some options or groups, the original constraint is kept by Symfony and can be displayed on the dump command output.
Symfony changes property mapping configuration with EnableAutoMapping
and DisableAutoMapping
(I cannot add the constraint in debug command output because the getAutoMappingStrategy
function is not defined in MetadataInterface
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@macintoshplus Maybe you can check if the metadata is an instance of ClassMetadata and then getAutoMappingStragegy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the same for Cascade
and ClassMetadata
https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/Validator/Mapping/ClassMetadata.php#L210
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking: the title of this PR mentions “constaints” instead of “constraints”. |
The command
debug:validator
doesn't dump theValid
constraints with the default group.The
Valid
constraints with default group change the propertyCascadingStrategy
and theTraversalStrategy
on property metadata.This patch adds the
Valid
constraint on debug output if theCascadingStrategy
is the same asCASCADE
.