Skip to content

[Form] Add getErrorsAsArray method #7512

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

Closed
wants to merge 2 commits into from

Conversation

raziel057
Copy link
Contributor

Usefull to return form errors through JsonResponse.

See #7205

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets 7205
License MIT
Doc PR Is it needed for this kind of addition?
modified:   src/Symfony/Component/Form/Form.php
modified:   src/Symfony/Component/Form/Tests/CompoundFormTest.php
modified:   src/Symfony/Component/Form/Tests/SimpleFormTest.php

Usefull to return form errors through JsonResponse
	modified:   src/Symfony/Component/Form/Form.php
	modified:   src/Symfony/Component/Form/Tests/CompoundFormTest.php
	modified:   src/Symfony/Component/Form/Tests/SimpleFormTest.php
	modified:   src/Symfony/Component/Form/Tests/CompoundFormTest.php
@raziel057
Copy link
Contributor Author

For information, Scrutinizer errors are not due to my commits.

I can't see what is wrong with Travis check on PHP 5.4. Is it possible to have the detail of the failure?

When I click here: https://travis-ci.org/symfony/symfony/jobs/5879397, I simply have an endless "loading" message.

@stof
Copy link
Member

stof commented Apr 2, 2013

the travis interface seems broken currently (I see a JS error in the console). You should report it in their issue tracker. And waiting for a fix, you could try to get the logs through their CLI or their API.

@raziel057
Copy link
Contributor Author

@stof Thanks. In fact I get an error with Firefox 19.0.2 but not in Chrome. I opened a ticket on travis-web tracker: https://github.com/travis-ci/travis-web/issues/171

The failure concern this test, but I didn't touch it.
There was 1 failure:

  1. Symfony\Component\Process\Tests\SigchildDisabledProcessTest::testRestart
    Failed asserting that true is false.

$template = $error->getMessageTemplate();
if (null !== $template) {
$parameters = $error->getMessageParameters();
$errors[$key] = strtr($template, $parameters);
Copy link
Contributor

Choose a reason for hiding this comment

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

How it pissible translate this messages?

Copy link
Member

Choose a reason for hiding this comment

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

It is not possible. :(
may be, we can introduce a little optional coupling by allow the end user to give a translatorInterface to getErrorsAsArray

so from the controller $form->getErrorsAsArray($translator);

Copy link
Contributor

Choose a reason for hiding this comment

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

@raziel057 Does one $key can contain more than 1 error? If yes - you should append they but not override

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Koc As you can see it on the tests I added, a field can contain multiple errors, but in this case there is only one error per $key. Do you have use cases with multiples error per $key?

@lyrixx Yes, I also thought about this solution. I think it's not so bad. I can add this option if @fabpot is ok for that.

@fabpot
Copy link
Member

fabpot commented Apr 20, 2013

@bschussek What do you think about this addition?

@webmozart
Copy link
Contributor

The current implementation here is a bit flawed. It combines both local errors (indexed by integers) and nested errors (indexed by field names, which can be integers too) in one single data structure, which cannot work.

I think this functionality should be merged into getErrors() by introducing a new parameter $deep that is false by default. getErrors() should then return some sort of array-compatible (for BC) object (FormErrorList, FormErrorIterator, not sure yet) that supports recursive iteration (\RecursiveIterator). Also, this object should implement __toString(), which should return the same output as getErrorsAsString() (which then could be deprecated).

I can't really give any advice yet on further details. This needs to be thought through and it is definitely not trivial.

@tijuan
Copy link

tijuan commented May 6, 2013

If I can give my opinion on the expected format, this should look like
$errors[field_name] = ['label' => 'field_label', 'errors' => ['error_msg_1', 'error_msg_2'] ];

@TomiS
Copy link

TomiS commented May 9, 2013

@bschussek That seems like a nice idea to me. However, it seems to me FormError class currently does not contain any information about the form field name the error is related to. So some additional data about the field name/key should be added to FormError too, I think.

About the array compatibility, would implementing ArrayAccess interface do the trick?

@stof
Copy link
Member

stof commented May 9, 2013

@TomiS FormError instances are attached to a Form instance. So getting the field name is possible as you need to Form to get the errors.

@TomiS
Copy link

TomiS commented May 10, 2013

Ok, I spent yesterday playing with this thing and first tried to implement something similar to what @bschussek had in mind. Here's a diff of what I ended up with. There are a couple of problems with this approach, though. 1) The code becomes quite complex if we really want array compatibility and if it should be fully array serializable (to get json too). 2) I ended up repeating the tree structure already introduced by Form class and I feel this adds unnecessary complexity too. 3) Getting a nice looking json output out of the very complex class structure is tedious (at least before PHP 5.4 and its JsonSerializable inteface)

So... I also ended up trying something much more simple. Basically what I did was repeating the same implementation introduced in this PR but just in the getErrors() method. You can see that implementation in here. I tried to serialize the output of this with JMSSerializer to json and the result was "just what the api developer needs", imo :) The benefit of this approach is that it's very simple as it utilizes the tree structure provided by Form class itself. What comes to indexes of local and nested errors, I really can't see the problem. Is there really a risk for index collision because I truly can't see it. (but on the other hand, I didn't write the component like some of us, so what do I know ;)

ps. Good point @stof. And it's also kind of related to what I say here about duplicating tree structure.

@webmozart
Copy link
Contributor

replaced by #9099

@webmozart webmozart closed this Oct 4, 2013
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.

8 participants