Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromogizanagi:feature/form/trans_failed_ex_safe_message
Mar 27, 2019
Merged

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

fabpot merged 1 commit intosymfony:masterfromogizanagi:feature/form/trans_failed_ex_safe_message
Mar 27, 2019

Conversation

@ogizanagi
Copy link
Contributor

@ogizanagiogizanagi commentedDec 18, 2016•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#22501
LicenseMIT
Doc PRN/A

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

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

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

  • setSafeMessage ➜setInvalidMessage
  • getMessageKey ➜getInvalidMessageKey
  • getMessageData ➜getInvalidMessageParameters

in order to echoesinvalid_message andinvalid_message_parameters options?

=> Replaced to useinvalidMessage &invalidMessageParameters

vsoroka, anyt, sstok, and yceruto reacted with thumbs up emoji
@nicolas-grekasnicolas-grekas added this to the3.x milestoneDec 26, 2016
@ogizanagi
Copy link
ContributorAuthor

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

@HeahDude
Copy link
Contributor

Yes, I suggest to wait :).

@nicolas-grekas
Copy link
Member

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

@nicolas-grekasnicolas-grekas modified the milestones:3.4,4.1Oct 8, 2017
@fabpot
Copy link
Member

@HeahDude Any ways we can unblock this PR?

@nicolas-grekasnicolas-grekas modified the milestones:4.1,nextApr 20, 2018
@xabbuh
Copy link
Member

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
Copy link
ContributorAuthor

@xabbuh : Would you remove the safe message in favor of more specificTransformationFailedException 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 typeinvalid_message option when relevant.

@xabbuh
Copy link
Member

xabbuh commentedOct 9, 2018•
edited
Loading

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.

@ogizanagiogizanagi deleted the feature/form/trans_failed_ex_safe_message branchOctober 31, 2018 17:41
@nicolas-grekasnicolas-grekas modified the milestones:next,4.2Nov 1, 2018
@ogizanagiogizanagi restored the feature/form/trans_failed_ex_safe_message branchNovember 5, 2018 23:23
@ogizanagiogizanagi reopened thisNov 5, 2018
@ogizanagi
Copy link
ContributorAuthor

Woops. I've closed this one by mistake.

@nicolas-grekas
Copy link
Member

rebase needed. for which branch is this PR?

@ogizanagiogizanagi removed this from the4.2 milestoneNov 9, 2018
@ogizanagiogizanagi added this to thenext milestoneNov 9, 2018
@ogizanagiogizanagi changed the base branch from3.4 tomasterNovember 9, 2018 07:30
@ogizanagi
Copy link
ContributorAuthor

Rebased on master

}
}catch (TransformationFailedException$exception) {
thrownewTransformationFailedException('Unable to transform value for property path "'.$this->getPropertyPath().'":'.$exception->getMessage(),$exception->getCode(),$exception);
thrownewTransformationFailedException(
Copy link
Member

Choose a reason for hiding this comment

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

should be kept on one line

ogizanagi reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

also, not sure if we really need this change

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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?

@fabpot
Copy link
Member

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

Copy link
ContributorAuthor

@ogizanagiogizanagi left a comment

Choose a reason for hiding this comment

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

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

}
}catch (TransformationFailedException$exception) {
thrownewTransformationFailedException('Unable to transform value for property path "'.$this->getPropertyPath().'":'.$exception->getMessage(),$exception->getCode(),$exception);
thrownewTransformationFailedException(
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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
Copy link
ContributorAuthor

(PR rebased)

@fabpot
Copy link
Member

Thank you@ogizanagi.

@fabpotfabpot merged commitd11055c intosymfony:masterMar 27, 2019
fabpot added a commit that referenced this pull requestMar 27, 2019
…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/ACurrently, 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
@ogizanagiogizanagi deleted the feature/form/trans_failed_ex_safe_message branchMarch 27, 2019 06:53
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@xabbuhxabbuhxabbuh left review comments

@fabpotfabpotfabpot approved these changes

+1 more reviewer

@HeahDudeHeahDudeHeahDude left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

6 participants

@ogizanagi@HeahDude@nicolas-grekas@fabpot@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp