Skip to content

[WebProfilerBundle][Form] Add information about form synchronization err... #11487

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 1 commit into from

Conversation

psliwa
Copy link

@psliwa psliwa commented Jul 26, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #9891, #9989, #5607
License MIT
Doc PR

When form data transformation fails, the origin error message is lost. This error shouldn't be shown to user, because this is technical error, so maybe a good idea is to show transformation failures in web profiler in form panel below errors section?

There are no BC-breaks in this PR, I have added public internal method to Symfony\Component\Form\Form class, that returns TransformationFailedException, it is used by FormDataExtractor. Symfony\Component\Form\FormInterface has not been touched by me, because that would be a BC-break.

@sstok
Copy link
Contributor

sstok commented Jul 26, 2014

Epic 👍 sometimes you get a failed transformation because your not using some internal stuff correctly. And its really hard to debug such cases.

@@ -134,6 +135,10 @@ public function extractSubmittedData(FormInterface $form)

$data['synchronized'] = $this->valueExporter->exportValue($form->isSynchronized());

if ($form instanceof Form && $form->getSynchronizationFailureCause() !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

Small detail: we are using null !== xxx in Symfony.

@fabpot
Copy link
Member

fabpot commented Jul 27, 2014

Looks like a great idea. @webmozart Can you check the implementation?

@fabpot
Copy link
Member

fabpot commented Jul 27, 2014

To make it even more obvious, what about changing the color to red in the toolbar itself (and probably increment the error count to 1 in this case)?

@psliwa
Copy link
Author

psliwa commented Jul 27, 2014

ok, error count in toolbar and in form panel is incremented by 1 for every transformation failure.

}

/**
* @return TransformationFailedException
Copy link
Member

Choose a reason for hiding this comment

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

TransformationFailedException|null

@webmozart
Copy link
Contributor

Thanks for your PR @psliwa! I had a slightly different approach in mind which I implemented in #12052 and #12054. Let me hear what you think about it.

Cheers :)

@webmozart webmozart closed this Sep 26, 2014
@psliwa
Copy link
Author

psliwa commented Sep 28, 2014

I thought about do that in simillar way as you, but I realized there would be BC-break so I decided to do that in less invasively way. If BC-break is acceptable in that situation, your solution is more elegant and flexible ;)

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.

5 participants