NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Validator] Dump Valid constraints on debug command#48840

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:5.4frommacintoshplus:patch-1

Aug 14, 2023

Merged

[Validator] Dump Valid constraints on debug command#48840

nicolas-grekas merged 1 commit intosymfony:5.4frommacintoshplus:patch-1

Aug 14, 2023

Conversation

Copy link

Contributor

macintoshplus commentedDec 31, 2022•
edited by OskarStark
Loading

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

The commanddebug:validator doesn't dump theValid constraints with the default group.

TheValid constraints with default group change the propertyCascadingStrategy and theTraversalStrategy on property metadata.

This patch adds theValid constraint on debug output if theCascadingStrategy is the same asCASCADE.

carsonbot added Status: Needs Review Bug labels

Dec 31, 2022

carsonbot added this to the5.4 milestone

Dec 31, 2022

Copy link

carsonbot commentedDec 31, 2022

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://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

macintoshplus mentioned this pull request

Dec 31, 2022

incomplete result of debug:validator command#46544

Closed

OskarStark added the Validator label

Jan 1, 2023

carsonbot changed the title~~Dump Valid constaints on debug command #46544~~[Validator] Dump Valid constaints on debug command #46544

Jan 1, 2023

OskarStark changed the title~~[Validator] Dump Valid constaints on debug command #46544~~[Validator] Dump Valid constaints on debug command

Jan 1, 2023

Copy link

carsonbot commentedJan 1, 2023

Hey!

I think@loic425 has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

WebMamba reviewed

Jan 1, 2023

View reviewed changes

src/Symfony/Component/Validator/Command/DebugCommand.php Outdated

Comment on lines 174 to 180

		if (CascadingStrategy::CASCADE ===$metadata->getCascadingStrategy()) {
		$constraint =newValid([], [],null, TraversalStrategy::IMPLICIT ===$metadata->getTraversalStrategy());
		$data[] = [
		'class' => Valid::class,
		'groups' => [$constraint::DEFAULT_GROUP],
		'options' =>$this->getConstraintOptions($constraint),
		];

Copy link

Contributor

WebMambaJan 1, 2023

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

ContributorAuthor

macintoshplusJan 2, 2023•
edited
Loading

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 extendValid annotation without changing anything, the behavior is the same as using theValid constraint. And, your custom constraint is changed toValid on debug command. The debug command cannot know the original constraint class name because Symfony has removed it.

See the codehttps://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 withEnableAutoMapping andDisableAutoMapping (I cannot add the constraint in debug command output because thegetAutoMappingStrategy function is not defined inMetadataInterface).

Copy link

Contributor

loic425Jan 3, 2023

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

HeahDudeJan 3, 2023

Choose a reason for hiding this comment

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

It is the same forCascade andClassMetadatahttps://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/Validator/Mapping/ClassMetadata.php#L210

Copy link

ContributorAuthor

macintoshplusJan 3, 2023

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?

Copy link

Contributor

alexislefebvre commentedJan 3, 2023

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

macintoshplus changed the title~~[Validator] Dump Valid constaints on debug command~~[Validator] Dump Valid constraints on debug command

Jan 3, 2023

Copy link

Member

nicolas-grekas commentedApr 18, 2023

What can we do here to unlock the PR?

Copy link

ContributorAuthor

macintoshplus commentedApr 18, 2023

What can we do here to unlock the PR?

Hi Nicolas,

Thank you for your interest in this PR.

What way to display the validation constraint that changes parameters and has been removed by Symfony?

I have tried to displayValid assert in the dump command but, some others constraint is in the same situation.

IMHO two choices:

display parameters on top
display the Symfony native constraints used to change the parameters (my first choice in this PR)

I'm available on Symfony Slack (Jean-Baptiste N.) or Twitter (@jbnahan69) to discuss this in French if you want.

Copy link

ContributorAuthor

macintoshplus commentedJul 11, 2023

Hi@nicolas-grekas,@loic425,

I have changed the property options displayed on debug output.

The 3 options (cascadeStrategy, autoMappingStrategy, traversalStrategy) it's now visible before property validators.

Symfony\Component\Validator\Tests\Dummy\DummyClassOne-----------------------------------------------------+---------------+----------------------------------------------------+---------+------------------------------------------------------------+| Property      | Name                                               | Groups  | Options                                                    |+---------------+----------------------------------------------------+---------+------------------------------------------------------------+| -             | Symfony\Component\Validator\Constraints\Expression | Default | [                                                          ||               |                                                    |         |   "expression" => "1 + 1 = 2",                             ||               |                                                    |         |   "message" => "This value is not valid.",                 ||               |                                                    |         |   "payload" => null,                                       ||               |                                                    |         |   "values" => []                                           ||               |                                                    |         | ]                                                          || firstArgument | property options                                   |         | [                                                          ||               |                                                    |         |   "cascadeStrategy" => "Cascade",                          ||               |                                                    |         |   "autoMappingStrategy" => "Not supported",                ||               |                                                    |         |   "traversalStrategy" => "Implicit"                        ||               |                                                    |         | ]                                                          || firstArgument | Symfony\Component\Validator\Constraints\NotBlank   | Default | [                                                          ||               |                                                    |         |   "allowNull" => false,                                    ||               |                                                    |         |   "message" => "This value should not be blank.",          ||               |                                                    |         |   "normalizer" => null,                                    ||               |                                                    |         |   "payload" => null                                        ||               |                                                    |         | ]                                                          || firstArgument | Symfony\Component\Validator\Constraints\Email      | Default | [                                                          ||               |                                                    |         |   "message" => "This value is not a valid email address.", ||               |                                                    |         |   "mode" => null,                                          ||               |                                                    |         |   "normalizer" => null,                                    ||               |                                                    |         |   "payload" => null                                        ||               |                                                    |         | ]                                                          |+---------------+----------------------------------------------------+---------+------------------------------------------------------------+

macintoshplus requested review fromOskarStark,chalasr,dunglas,lyrixx,wouterj,xabbuh andyceruto ascode owners

July 11, 2023 08:16

loic425 mentioned this pull request

Jul 11, 2023

[Validator] Do not mock metadata factory on debug command tests#50935

Merged

Copy link

Contributor

loic425 commentedJul 11, 2023•
edited
Loading

@nicolas-grekas I think we needthis PR first to be sure of the behaviour.

nicolas-grekas added a commit that referenced this pull request

Jul 13, 2023

minor#50935[Validator] Do not mock metadata factory on debug comman…

67acf9e

…d tests (loic425)This PR was squashed before being merged into the 5.4 branch.Discussion----------[Validator] Do not mock metadata factory on debug command tests| Q             | A| ------------- | ---| Branch?       |  5.4| Bug fix?      | not really, but it will be usefull to fix that command| New feature?  | no| Deprecations? | no| Tickets       || License       | MIT| Doc PR        |<!--Replace this notice by a short README for your feature/bugfix.This will help reviewers and should be a good start for the documentation.Additionally (seehttps://symfony.com/releases): - Always add tests and ensure they pass. - Bug fixes must be submitted against the lowest maintained 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 latest branch. - For new features, provide some code snippets to help understand usage. - Changelog entry should followhttps://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (seehttps://symfony.com/bc).-->Before#48840 I think this one could improve the tests.Commits-------de6d15f [Validator] Do not mock metadata factory on debug command tests