[Form] FormErrorIterator do not filter only with Constraint causes #28649

gmponos · 2018-09-30T11:39:08Z

Q	A
Branch?	master for features
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT
Doc PR	symfony/symfony-docs

I made this PR based on this

In general I believe that the FormError object should be strict on the type of the Cause. It should either be a ConstraintViolation object an object that implements toString function or a simple string and this should be checked during the construction of the FormError. For me it's too vague what a cause argument is and what findByCodes does.. Althought this PR does not check anything on the construction of the FormError object this can be the first step.

I will add tests later if you agree with this PR. Let me know. 😄

ro0NL · 2018-09-30T11:57:28Z

i dont think FormError object should be strict on the type of the Cause.. but this change seems reasonable yes 👍

nicolas-grekas · 2018-10-01T20:07:32Z

src/Symfony/Component/Form/FormErrorIterator.php

+            }
+
+            $cause = (string)$cause;
+            if(\in_array($cause, $codes, true)){


don't miss fabbot patch around this line

gmponos · 2018-10-02T17:01:06Z

Do you guys believe that this change is a BC?

An edge case:
Someone was passing as a cause an object and he was filtering using findByCodes(). Object did have a __toString function. Before he would get 2 FormErrors that all contained ConstraintViolations now he gets 3 FormErrors and some contain various causes.

nicolas-grekas · 2018-10-20T19:46:44Z

(rebase needed)
@xabbuh @HeahDude maybe you'd like to review this PR?

… is string or object that has a __toString method

xabbuh · 2018-10-21T09:11:24Z

I think a better solution would be to introduce the concept of error codes in the FormError class too and filter based on that code instead.

xabbuh · 2018-10-21T09:12:45Z

We could also think about having a filter() method that accepts a callback to filter the list of errors.

gmponos · 2018-10-21T11:23:53Z

We could also think about having a filter() method that accepts a callback to filter the list of errors.

I do like this idea even better.

I think a better solution would be to introduce the concept of error codes in the FormError class too and filter based on that code instead.

Can you elaborate a little bit here? Do you mean that FormError can have a Cause and a Code also?

ro0NL · 2018-10-21T11:54:43Z

We could also think about having a filter()

also see #19496 (comment). Im curious about the proposed API, as we avoid the callback to have convenient API today. This PR just provides more meaning to "code" in the form component (e.g. if the cause is scalar-ish; it's a code). That's valid to me.

It's effectively the same as

I think a better solution would be to introduce the concept of error codes in the FormError class too and filter based on that code instead.

Other filters fit foreach IMHO.

ro0NL · 2018-10-21T11:59:54Z

src/Symfony/Component/Form/Tests/FormErrorIteratorTest.php

+     */
+    public function testFindByCodesWhenAllCausesAreVariousObjectsOrSimpleStrings($code, $violationsCount)
+    {
+        if (!class_exists(ConstraintViolation::class)) {


we should not require validator component to test the new feature (we aim for compatibility without it)

FYI this test class on master contains one test case. I have duplicated this test case on the current PR. These lines are the copied code from the previous test case.

Your proposal is to remove them from both test cases?

FYI this test class on master contains one test case

then i suggest to refactor it a bit, and keep a single testFindByCodes() where the expectations are built dynamically (if validator exists yes/no)

ro0NL · 2018-10-21T12:34:09Z

src/Symfony/Component/Form/FormErrorIterator.php

+            }
+
+            $cause = (string) $cause;
+            if (\in_array($cause, $codes, true)) {


id say in_array((string) $cause, ..

fabpot · 2019-07-08T10:26:48Z

What's the status of this pull request? /cc @xabbuh

HeahDude

LGTM

gmponos · 2019-11-23T13:18:03Z

I dont have the time to work on this.. whoever wants can take it...

fabpot · 2020-08-11T10:22:57Z

Closing as nobody stepped up to finish this PR.

carsonbot added Status: Needs Review Form Feature labels Sep 30, 2018

nicolas-grekas reviewed Oct 1, 2018

View reviewed changes

nicolas-grekas added this to the 4.2 milestone Oct 1, 2018

gmponos force-pushed the filter_validation_errors_on_form branch from 3ac3ab1 to 545dc12 Compare October 2, 2018 18:02

nicolas-grekas modified the milestones: 4.2, next Oct 20, 2018

Make the findByCodes able to filter FormErrors that have a cause that…

e5275c6

… is string or object that has a __toString method

gmponos force-pushed the filter_validation_errors_on_form branch from 545dc12 to e5275c6 Compare October 20, 2018 20:21

ro0NL reviewed Oct 21, 2018

View reviewed changes

HeahDude reviewed Nov 23, 2019

View reviewed changes

fabpot closed this Aug 11, 2020

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] FormErrorIterator do not filter only with Constraint causes #28649

[Form] FormErrorIterator do not filter only with Constraint causes #28649

gmponos commented Sep 30, 2018 •

edited

Loading

ro0NL commented Sep 30, 2018 •

edited

Loading

nicolas-grekas Oct 1, 2018

gmponos Oct 2, 2018

gmponos commented Oct 2, 2018 •

edited

Loading

nicolas-grekas commented Oct 20, 2018

xabbuh commented Oct 21, 2018

xabbuh commented Oct 21, 2018

gmponos commented Oct 21, 2018

ro0NL commented Oct 21, 2018 •

edited

Loading

ro0NL Oct 21, 2018

gmponos Oct 21, 2018

ro0NL Oct 21, 2018

ro0NL Oct 21, 2018

fabpot commented Jul 8, 2019

HeahDude left a comment

gmponos commented Nov 23, 2019

fabpot commented Aug 11, 2020

[Form] FormErrorIterator do not filter only with Constraint causes #28649

[Form] FormErrorIterator do not filter only with Constraint causes #28649

Conversation

gmponos commented Sep 30, 2018 • edited Loading

ro0NL commented Sep 30, 2018 • edited Loading

nicolas-grekas Oct 1, 2018

Choose a reason for hiding this comment

gmponos Oct 2, 2018

Choose a reason for hiding this comment

gmponos commented Oct 2, 2018 • edited Loading

nicolas-grekas commented Oct 20, 2018

xabbuh commented Oct 21, 2018

xabbuh commented Oct 21, 2018

gmponos commented Oct 21, 2018

ro0NL commented Oct 21, 2018 • edited Loading

ro0NL Oct 21, 2018

Choose a reason for hiding this comment

gmponos Oct 21, 2018

Choose a reason for hiding this comment

ro0NL Oct 21, 2018

Choose a reason for hiding this comment

ro0NL Oct 21, 2018

Choose a reason for hiding this comment

fabpot commented Jul 8, 2019

HeahDude left a comment

Choose a reason for hiding this comment

gmponos commented Nov 23, 2019

fabpot commented Aug 11, 2020

gmponos commented Sep 30, 2018 •

edited

Loading

ro0NL commented Sep 30, 2018 •

edited

Loading

gmponos commented Oct 2, 2018 •

edited

Loading

ro0NL commented Oct 21, 2018 •

edited

Loading