[Validator] added improve support for collection validation #31196

Simperfit · 2019-04-22T08:18:55Z

Q	A
Branch?	4.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	to set
License	MIT
Doc PR	to do

I've taken #9988, what's missing here @webmozart ?

nicolas-grekas · 2019-04-23T08:39:29Z

Can you please squash + review the CS?

Simperfit · 2019-04-26T06:56:30Z

done @nicolas-grekas

xabbuh · 2019-04-27T08:34:34Z

Can you add an entry to the Validator component CHANGELOG.md file?

Simperfit · 2019-04-27T14:38:57Z

changelog added, PR rebased with master.

Simperfit · 2019-04-27T17:00:42Z

Validators and test rewrited.

src/Symfony/Component/Validator/Constraints/AbstractComposite.php

src/Symfony/Component/Validator/Tests/Constraints/AbstractCompositeTest.php

elementaire

Some typos.

src/Symfony/Component/Validator/Constraints/Some.php

src/Symfony/Component/Validator/Tests/Validator/RecursiveValidatorTest.php

src/Symfony/Component/Validator/Constraints/AbstractComposite.php

src/Symfony/Component/Validator/CHANGELOG.md

Simperfit · 2019-05-01T08:20:43Z

Thanks @elementaire for the review.

src/Symfony/Component/Validator/Constraints/Some.php

src/Symfony/Component/Validator/Constraints/AbstractComposite.php

src/Symfony/Component/Validator/Constraints/Each.php

src/Symfony/Component/Validator/Tests/Constraints/AbstractCompositeTest.php

Simperfit · 2019-06-08T16:26:03Z

Thanks for the review @xabbuh, all comments have been resolved ;).

Status: Needs review

Simperfit · 2019-06-08T20:19:32Z

Travis failure is not related

Simperfit · 2019-06-20T20:22:24Z

PR Rebased

Simperfit · 2019-07-09T06:30:55Z

@fabpot @xabbuh can we move forward with this ? conflict has been resolved.

xabbuh · 2019-07-13T10:29:43Z

src/Symfony/Component/Validator/Constraints/Unique.php

@@ -18,6 +18,8 @@
 * @Target({"PROPERTY", "METHOD", "ANNOTATION"})
 *
 * @author Yevgeniy Zholkevskiy <zhenya.zholkevskiy@gmail.com>
+ * @author Marc Morera Merino <yuhu@mmoreram.com>
+ * @author Marc Morales Valldepérez <marcmorales83@gmail.com>


Why these changes? And if, shouldn't they be done on the 4.3 branch?

was from the original PR

I think it needs to be revert because it was the contraints has been modfied in the original PR WDYT ? @xabbuh

src/Symfony/Component/Validator/CHANGELOG.md

xabbuh · 2019-07-13T10:39:57Z

src/Symfony/Component/Validator/Constraints/AbstractComposite.php

+    /**
+     * {@inheritdoc}
+     */
+    protected function getCompositeOption()


I do not really see why we need this method.

to say if it's a contraints or a field, if we remove it, we need to remove it from the abtracts composite too and check if it's a contraints or a field

src/Symfony/Component/Validator/Constraints/AbstractComposite.php

xabbuh · 2019-07-13T10:47:53Z

src/Symfony/Component/Validator/Constraints/ExactlyValidator.php

+/**
+ * @author Hamza Amrouche <hamza.simperfit@gmail.com>
+ */
+class ExactlyValidator extends ConstraintValidator


I am actually not convinced that we need this validator. What would be a use case for it?

because of #31196 (comment)

src/Symfony/Component/Validator/Constraints/None.php

src/Symfony/Component/Validator/Constraints/Some.php

xabbuh · 2019-07-13T10:52:12Z

src/Symfony/Component/Validator/Constraints/SomeValidator.php

+ * @author Marc Morera Merino <yuhu@mmoreram.com>
+ * @author Marc Morales Valldepérez <marcmorales83@gmail.com>
+ */
+class SomeValidator extends ConstraintValidator


What would be an actual use case for this constraint?

I guess if some of the required contraints passesa and some other not it should be working properly.

Simperfit · 2019-07-16T06:48:44Z

PR Rebased and review answered.

Status: Needs Review

Simperfit · 2019-07-31T17:25:58Z

cc @xabbuh another round ?

xabbuh · 2019-08-26T09:55:16Z

@Simperfit I would really be interested in seeing a real concrete use case where one would like to ensure that a particular number of constraints passes (or fails). I cannot come up with an example where this seems to make sense.

stof

Almost all validators need to be re-implemented in that PR.

stof · 2019-08-26T10:13:06Z

src/Symfony/Component/Validator/CHANGELOG.md

+ * added `Each` constraint
+ * added `EachValidator`
+ * added `None` constraint
+ * added `NoneValidator`


I suggest merging this line with the previous one. Adding the new constraint and its validator are a single feature addition (same for other constraints).
Older versions don't mention the validator at all btw, only the new constraint (as that's the relevant user-facing thing)

stof · 2019-08-26T10:13:28Z

src/Symfony/Component/Validator/Constraints/AbstractComposite.php

+use Symfony\Component\Validator\Exception\ConstraintDefinitionException;
+
+/**
+ * @Annotation


should be removed. This abstract class is not an annotation by itself.

stof · 2019-08-26T10:14:35Z

src/Symfony/Component/Validator/Constraints/All.php

@@ -16,23 +16,12 @@
 * @Target({"PROPERTY", "METHOD", "ANNOTATION"})
 *
 * @author Bernhard Schussek <bschussek@gmail.com>
+ *
+ * @api


should be removed. We dropped such tag years ago in favor of @internal and @experimental

stof · 2019-08-26T10:14:42Z

src/Symfony/Component/Validator/Constraints/All.php

+ *
+ * @api
+ *
+ * @deprecated Deprecated in 4.3, to be removed in 5.0. Use


and this also needs to trigger a deprecation warning.

stof · 2019-08-26T10:15:14Z

src/Symfony/Component/Validator/Constraints/AllValidator.php

+ *
+ * @api
+ *
+ * @deprecated Deprecated in 4.3, to be removed in 5.0. Use


same issues here

stof · 2019-08-26T10:19:34Z

src/Symfony/Component/Validator/Constraints/NoneValidator.php

+            throw new UnexpectedValueException($value, 'array or Traversable');
+        }
+
+        $validator = $this->context->getValidator()->inContext($this->context);


same here about not using the current context for violations you don't want to show.

stof · 2019-08-26T10:20:20Z

src/Symfony/Component/Validator/Constraints/Some.php

+     *
+     * Message for notice Max Violation
+     */
+    public $maxMessage = '{{ limit }} or less element of this collection should pass validation.|{{ limit }} or less elements of this collection should pass validation.';


no message for the exactly case ?

stof · 2019-08-26T10:20:47Z

src/Symfony/Component/Validator/Constraints/Some.php

+     *
+     * Exactly number of Success expected
+     */
+    public $exactly;


Do we need this property ? Isn't it the same than min = max ?

stof · 2019-08-26T10:21:16Z

src/Symfony/Component/Validator/Constraints/SomeValidator.php

+
+        $totalIterations = \count($value) * \count($constraint->constraints);
+
+        $validator = $this->context->getValidator()->inContext($this->context);


same issue here about not reusing the context

stof · 2019-08-26T10:22:44Z

src/Symfony/Component/Validator/Constraints/SomeValidator.php

+            $validator->atPath('['.$key.']')->validate($element, $constraint->constraints);
+        }
+
+        $constraintsSuccess = $totalIterations - (int) $this->context->getViolations()->count();


this is wrong IMO. You should not count how many constraints were successful, but how many elements were successful. Otherwise, this is a pain to get right.

fabpot · 2020-02-04T13:35:27Z

Closing based on the comments from @xabbuh and @stof

carsonbot added Status: Needs Review Validator Bug Feature Deprecation labels Apr 22, 2019

nicolas-grekas removed Bug Deprecation labels Apr 23, 2019

nicolas-grekas added this to the next milestone Apr 23, 2019

Simperfit force-pushed the feature/improve-support-for-collection branch from fcfb4af to 36f0176 Compare April 26, 2019 06:56

Simperfit force-pushed the feature/improve-support-for-collection branch from 654a846 to 1f7be1f Compare April 26, 2019 06:58

Simperfit force-pushed the feature/improve-support-for-collection branch from 1f7be1f to fb26d1f Compare April 27, 2019 14:39

Simperfit changed the title ~~[Validator] added improve support for collection validation~~ [WIP][Validator] added improve support for collection validation Apr 27, 2019

Simperfit changed the title ~~[WIP][Validator] added improve support for collection validation~~ [Validator] added improve support for collection validation Apr 27, 2019

Simperfit force-pushed the feature/improve-support-for-collection branch 2 times, most recently from d16e041 to 9206395 Compare April 27, 2019 17:02

OskarStark reviewed Apr 29, 2019

View reviewed changes

src/Symfony/Component/Validator/Constraints/AbstractComposite.php Outdated Show resolved Hide resolved

src/Symfony/Component/Validator/Tests/Constraints/AbstractCompositeTest.php Outdated Show resolved Hide resolved

Simperfit force-pushed the feature/improve-support-for-collection branch from 9206395 to 20f9edd Compare April 30, 2019 07:35

elementaire reviewed Apr 30, 2019

View reviewed changes

src/Symfony/Component/Validator/CHANGELOG.md Outdated Show resolved Hide resolved

src/Symfony/Component/Validator/CHANGELOG.md Outdated Show resolved Hide resolved

Simperfit force-pushed the feature/improve-support-for-collection branch from 20f9edd to 59a8f1d Compare May 1, 2019 08:18

xabbuh requested changes May 4, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels May 4, 2019

Simperfit force-pushed the feature/improve-support-for-collection branch from 59a8f1d to 4d05d1d Compare May 8, 2019 10:56

Simperfit force-pushed the feature/improve-support-for-collection branch 2 times, most recently from d6efa82 to 6501f58 Compare June 8, 2019 16:25

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jun 8, 2019

Simperfit force-pushed the feature/improve-support-for-collection branch from 6501f58 to 04ab76c Compare June 20, 2019 20:22

Simperfit force-pushed the feature/improve-support-for-collection branch from 04ab76c to a9aa221 Compare July 9, 2019 06:29

xabbuh requested changes Jul 13, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 13, 2019

Simperfit force-pushed the feature/improve-support-for-collection branch from a9aa221 to 98cd0d4 Compare July 16, 2019 06:45

mrcmorales and others added 2 commits July 16, 2019 08:46

[Validator] added improve support for collection validation

1079126

[Validator] rewrite the feature to fit with the new way

10948fc

Simperfit force-pushed the feature/improve-support-for-collection branch from 98cd0d4 to 10948fc Compare July 16, 2019 06:47

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jul 16, 2019

stof requested changes Aug 26, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Aug 26, 2019

xabbuh mentioned this pull request Jan 6, 2020

Compose validation #35226

Closed

fabpot closed this Feb 4, 2020

nicolas-grekas modified the milestones: next, 5.1 May 4, 2020


		$totalIterations = \count($value) * \count($constraint->constraints);

		$validator = $this->context->getValidator()->inContext($this->context);

Uh oh!

[Validator] added improve support for collection validation #31196

[Validator] added improve support for collection validation #31196

Uh oh!

Conversation

Simperfit commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Apr 23, 2019

Uh oh!

Simperfit commented Apr 26, 2019

Uh oh!

xabbuh commented Apr 27, 2019

Uh oh!

Simperfit commented Apr 27, 2019

Uh oh!

Simperfit commented Apr 27, 2019

Uh oh!

Uh oh!

Uh oh!

elementaire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Simperfit commented May 1, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Simperfit commented Jun 8, 2019

Uh oh!

Simperfit commented Jun 8, 2019

Uh oh!

Simperfit commented Jun 20, 2019

Uh oh!

Simperfit commented Jul 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit commented Jul 16, 2019

Uh oh!

Simperfit commented Jul 31, 2019

Uh oh!

Simperfit commented Apr 22, 2019 •

edited

Loading

Simperfit Jul 16, 2019 •

edited

Loading