Skip to content
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

Open
wants to merge 2 commits into
base: 5.4
Choose a base branch
from

Conversation

macintoshplus
Copy link

@macintoshplus macintoshplus commented Dec 31, 2022

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #46544
License MIT

The command debug:validator doesn't dump the Valid constraints with the default group.

The Valid constraints with default group change the property CascadingStrategy and the TraversalStrategy on property metadata.

This patch adds the Valid constraint on debug output if the CascadingStrategy is the same as CASCADE.

@carsonbot
Copy link

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:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.3 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot carsonbot changed the title Dump Valid constaints on debug command #46544 [Validator] Dump Valid constaints on debug command #46544 Jan 1, 2023
@OskarStark OskarStark changed the title [Validator] Dump Valid constaints on debug command #46544 [Validator] Dump Valid constaints on debug command Jan 1, 2023
@carsonbot
Copy link

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),
];
Copy link
Contributor

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.

Copy link
Author

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).

Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@loic425 Yes, I can.

During the investigation (thank @HeahDude), I found some other constraints with a particular behavior.

Without trying to retrieve the original constraint to display, it's possible to display class or property metadata properties differently? How?

@alexislefebvre
Copy link
Contributor

Nitpicking: the title of this PR mentions “constaints” instead of “constraints”.

@macintoshplus macintoshplus changed the title [Validator] Dump Valid constaints on debug command [Validator] Dump Valid constraints on debug command Jan 3, 2023
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.

None yet

7 participants