Skip to content
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

[Form] TransformationFailedException: Support specifying message to display #20978

Merged

Conversation

Projects
None yet
6 participants
@ogizanagi
Copy link
Member

commented Dec 18, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22501
License MIT
Doc PR N/A

Currently, a failed transformation can't display a very accurate message, as it only uses the value of the invalid_message option. I suggest to add more flexibility, somehow the same way we did for CustomUserMessageAuthenticationException.

A test case in FormTypeTest shows a use-case based on @webmozart's blog post about value/immutable objects in Symfony forms.

I named the exception properties and methods the same way as for CustomUserMessageAuthenticationException, but do you think the followings would be better:

  • setSafeMessagesetInvalidMessage
  • getMessageKeygetInvalidMessageKey
  • getMessageDatagetInvalidMessageParameters

in order to echoes invalid_message and invalid_message_parameters options?

=> Replaced to use invalidMessage & invalidMessageParameters

@ogizanagi ogizanagi force-pushed the ogizanagi:feature/form/trans_failed_ex_safe_message branch 3 times, most recently from 732ba0e to f95113d Dec 18, 2016

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 26, 2016

@ogizanagi ogizanagi force-pushed the ogizanagi:feature/form/trans_failed_ex_safe_message branch 2 times, most recently from 5115abf to 222ec1e Dec 30, 2016

@ogizanagi ogizanagi changed the base branch from master to 3.4 May 17, 2017

@ogizanagi

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2017

@HeahDude : Is it worth working again on this PR or should I wait your propositions about #20978 (comment) ?

@HeahDude

This comment has been minimized.

Copy link
Member

commented Jul 6, 2017

Yes, I suggest to wait :).

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 8, 2017

Moving to 4.1. Rebase on master needed, where PHP 7.1 features can be used btw.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2018

@HeahDude Any ways we can unblock this PR?

@nicolas-grekas nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

@xabbuh

This comment has been minimized.

Copy link
Member

commented Sep 15, 2018

What about allowing the option to be a closure that would get passed the transformation failure exception and would return the error message to use?

@ogizanagi

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2018

@xabbuh : Would you remove the safe message in favor of more specific TransformationFailedException instances then? I'm fine with both solutions, but I thing the safe message approach is more convenient than creating a dedicated exception and treat it in each form type invalid_message option when relevant.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

After thinking about it again I think having a message key in the exception isn't such a bad idea. It's also consistent with what we support in the Security component.

@ogizanagi ogizanagi closed this Oct 31, 2018

@ogizanagi ogizanagi deleted the ogizanagi:feature/form/trans_failed_ex_safe_message branch Oct 31, 2018

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

@ogizanagi ogizanagi restored the ogizanagi:feature/form/trans_failed_ex_safe_message branch Nov 5, 2018

@ogizanagi ogizanagi reopened this Nov 5, 2018

@ogizanagi

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2018

Woops. I've closed this one by mistake.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

rebase needed. for which branch is this PR?

@ogizanagi ogizanagi modified the milestones: 4.2, next Nov 9, 2018

@ogizanagi ogizanagi changed the base branch from 3.4 to master Nov 9, 2018

@ogizanagi ogizanagi force-pushed the ogizanagi:feature/form/trans_failed_ex_safe_message branch 2 times, most recently from 0adf762 to 4ede271 Nov 9, 2018

@ogizanagi

This comment has been minimized.

Copy link
Member Author

commented Nov 9, 2018

Rebased on master

Show resolved Hide resolved src/Symfony/Component/Form/Exception/TransformationFailedException.php Outdated
@@ -1050,7 +1050,13 @@ private function modelToNorm($value)
$value = $transformer->transform($value);
}
} catch (TransformationFailedException $exception) {
throw new TransformationFailedException('Unable to transform value for property path "'.$this->getPropertyPath().'": '.$exception->getMessage(), $exception->getCode(), $exception);
throw new TransformationFailedException(

This comment has been minimized.

Copy link
@xabbuh

xabbuh Nov 10, 2018

Member

should be kept on one line

This comment has been minimized.

Copy link
@xabbuh

xabbuh Nov 10, 2018

Member

also, not sure if we really need this change

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Mar 24, 2019

Author Member

not sure if we really need this change

Well, we need to forward the invalid message and parameters to the new exception, otherwise it's just vanished. What did you mean?

Show resolved Hide resolved src/Symfony/Component/Form/Form.php Outdated
Show resolved Hide resolved src/Symfony/Component/Form/Form.php Outdated
Show resolved Hide resolved src/Symfony/Component/Form/Form.php Outdated
@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

@ogizanagi @HeahDude I think we need to take a decision here. Finish/merge or close?

@ogizanagi
Copy link
Member Author

left a comment

I still think this is valuable and doesn't bring much complexity.

@@ -1050,7 +1050,13 @@ private function modelToNorm($value)
$value = $transformer->transform($value);
}
} catch (TransformationFailedException $exception) {
throw new TransformationFailedException('Unable to transform value for property path "'.$this->getPropertyPath().'": '.$exception->getMessage(), $exception->getCode(), $exception);
throw new TransformationFailedException(

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Mar 24, 2019

Author Member

not sure if we really need this change

Well, we need to forward the invalid message and parameters to the new exception, otherwise it's just vanished. What did you mean?

@ogizanagi ogizanagi force-pushed the ogizanagi:feature/form/trans_failed_ex_safe_message branch 2 times, most recently from a0a2e45 to 3146257 Mar 26, 2019

@ogizanagi ogizanagi force-pushed the ogizanagi:feature/form/trans_failed_ex_safe_message branch from 3146257 to d11055c Mar 26, 2019

@ogizanagi

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

(PR rebased)

@fabpot

fabpot approved these changes Mar 27, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Thank you @ogizanagi.

@fabpot fabpot merged commit d11055c into symfony:master Mar 27, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 27, 2019

feature #20978 [Form] TransformationFailedException: Support specifyi…
…ng message to display (ogizanagi)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Form] TransformationFailedException: Support specifying message to display

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22501
| License       | MIT
| Doc PR        | N/A

Currently, a failed transformation can't display a very accurate message, as it only uses the value of the `invalid_message` option. I suggest to add more flexibility, somehow the same way we did for `CustomUserMessageAuthenticationException`.

A test case in `FormTypeTest` shows a use-case based on @webmozart's [blog post about value/immutable objects in Symfony forms](https://webmozart.io/blog/2015/09/09/value-objects-in-symfony-forms/).

~I named the exception properties and methods the same way as for `CustomUserMessageAuthenticationException`, but do you think the followings would be better:~

- ~`setSafeMessage` ➜ `setInvalidMessage`~
- ~`getMessageKey` ➜ `getInvalidMessageKey`~
- ~`getMessageData` ➜ `getInvalidMessageParameters`~

~in order to echoes `invalid_message` and `invalid_message_parameters` options?~

=> Replaced to use `invalidMessage` & `invalidMessageParameters`

Commits
-------

d11055c [Form] TransformationFailedException: Support specifying message to display

@ogizanagi ogizanagi deleted the ogizanagi:feature/form/trans_failed_ex_safe_message branch Mar 27, 2019

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.