Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] AddedControllerTrait::isFormValid#24576
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
weaverryan commentedOct 16, 2017
Hmm, I’m not sure. The form code in a controller suffers from 2 issues: itis a bit verbose, but more importantly, the flow confuses beginners (especially since we typically put both the GET and POST in one controller). By making people do ‘if $form->isSubmitted() && $form->isValid()’, it made things more clear, but uglier. In other words, I’d like to solve this issue, but I don’t think this is it. Perhaps changing the recommendation to split forms into 2 different controllers but adding some shortcuts (if needed) to help with this is a better thing to investigate. Also, see what other libs do. |
| * @return bool | ||
| * | ||
| * @final since version 4.1 | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can use a return-type as this is PHP 7.1 ❤️ 😄
nicolas-grekas commentedFeb 4, 2018
shall we close then? |
weaverryan commentedFeb 4, 2018
Unless@lyrixx has some counter argument, +1 for closing |
lyrixx commentedFeb 5, 2018
Actually, I like this PR. It makes usage of form simpler. And doing 2 controller for handling a form is wrong IMHO because it leads to code duplication. |
| * | ||
| * @final since version 4.1 | ||
| */ | ||
| protectedfunctionisFormValid(FormInterface$form,Request$request =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In order to deduce what this internally does, and more accurate with what we have currently ($form->isSubmitted() && $form->isValid()) what about naming it:isSubmittedFormValid()?
javiereguiluz commentedFeb 5, 2018
I'm always in favor of simplifying things and making code as concise as possible ... but I don't like this proposal for two reasons:
|
lyrixx commentedFeb 7, 2018
It's not an alternative way, it's a shortcut. This is really different. All method in this trait are shortcut. And they are here just to ease our life.
indeed, it's not 10x better, it's 2.6x better proof$before =strlen('$form->handleRequest($request)->isSubmitted() && $form->isValid()');$after =strlen('$this->formIsValid($form)');dump($before/$after);
Indeed. That's not the primary goal, but it's a side effect. |
weaverryan commentedFeb 7, 2018
Hmm, I just don’t like it. It’s shorter, but way less clear. From training, I already need to explain how handleRequest() only handles the request in submit. But at least it’s mostly descriptive. I can’t think of a better alternative. It really does need to be descriptive and short. Maybe: if ($this->submit($form) && $form->isValid()) ... where the submit() method would call $form->handleRequest() and then return $form->isSubmitted() ? |
gharlan commentedFeb 7, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
if ($this->isFormSubmittedAndValid($form)) { } ? |
weaverryan commentedFeb 7, 2018
I still want some line that looks like we’re doing an action (i.e. submitting the form). That looks like we’re simply checking something. That’s why I suggested $this->submit(). It’s stillin the if statement, but hopefully looks like we’redoing something, not just checking :) |
lyrixx commentedFeb 8, 2018
There are too many 👎 from the core team. I have to close this PR |
xabbuh commentedSep 23, 2018
lyrixx commentedSep 25, 2018
@xabbuh Done ✅ |
a4762fb tob4e9ffcComparefabpot commentedOct 10, 2018
Not sure why discussion on#27638 would influence the decision on this PR. I don't have any strong opinion on the matter (even if I prefer explicitness whenever possible). Let's do a vote in the core team and take a decision once and for all: 1/ Merge as is /cc @symfony/deciders |
Tobion commentedOct 10, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
3/ due to#27638 (comment) |
lyrixx commentedNov 14, 2018
I have rebased and added the check ( |
| thrownew \LogicException('You must pass a request as second argument because the request stack was empty.'); | ||
| } | ||
| if ($form->isSubmitted()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can be the first check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about:
if (!$form->isSubmitted()) {$form->handleRequest($request);}return$form->isSubmitted() &&$form->isValid();
?
Throwing the exception here does not make sense to me due to the name of the shortcut, and given that the form would already do it by itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
then we conditionally ignore $request which could make this shortcut more confusing. IMHO it's not meant to be used once the form is already handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ro0NL I updated the PR ; thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm still not happy with this. Either we should rename the method to better explicit that it submits the form somehow, or to change as I suggest. I would be in favor of a better name like:
ControllerTrait:isFormSubmitted(FormInterface$form, bool$andValid =true, Request$request =null): bool
Then it's clear that one can callFormInterface::isSubmitted() orControllerTrait::isFormSubmitted() to wrap thehandleRequest() call once.
And having a parameter for a strict valid check would allow to use the shortcut even if the logic needs two steps in the controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO it's not meant to be used once the form is already handled.
It may, think aboutforward call(s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
really? That means the form instance is passed to the sub-request, to be validated twice.. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually it can be recreated and rehandled through this short cut yes, otherwise we should not care about having this clause guard in the first place, but people can do weird things with forward call like chaining or multiple calls during the same request.
So given the name of the shortcut, and unless we can't get it more explicit that it mutates the form, we should make it silent here and just return a bool, IMHO that would avoid WTF moments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
otherwise we should not care about having this clause guard in the first place
hm, that is actually required to do so... as the "default crash" might not always happen (depending on the submitted data in the request) and so we should let it return false:
symfony/src/Symfony/Component/Form/Form.php
Lines 502 to 506 inf9414a8
| publicfunctionsubmit($submittedData,$clearMissing =true) | |
| { | |
| if ($this->submitted) { | |
| thrownewAlreadySubmittedException('A form can only be submitted once'); | |
| } |
Also i think we should use a different name indeed. What aboutsubmitForm(...): bool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
actually no :) the check is fine. But intend should be clarified by name (submitForm()).
Removing it would cause to return a possible previous state.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
ro0NL commentedDec 12, 2018
given this some more thought, i think using the base controller as a one-size-fits-all solution is a dead-end eventually. It does too much IMHO, and only enables multiple ways of doing things. While valid by design, i hope long-term we'd strive for standalone solutions, e.g.#29574 instead of approaches like this PR or#28875. my2cnt :) |
fabpot commentedJan 2, 2019
Thank you@lyrixx. |
… (lyrixx)This PR was merged into the 4.3-dev branch.Discussion----------[FrameworkBundle] Added `ControllerTrait::isFormValid`| Q | A| ------------- | ---| Branch? | master (4.1)| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |symfony/symfony-docs#8518Commits-------1a8c844 [FrameworkBundle] Added `ControllerTrait::isFormValid`
goetas commentedJan 8, 2019
I really liked the "old" way of submitting forms. It was making clear that there was involved a This feature allows to do something as: publicfunctionmyAction() {$form =// create formif ($this->isFormValid($form)) {// something }} The trait hides the Im not really against having this method in the trait, but in my opinion the Is there any chance to rever this or at lease make the |
javiereguiluz commentedJan 8, 2019
I love this new |
lyrixx commentedJan 8, 2019
I also agree we should make the request mandatory. I will open a new PR |
…) (lyrixx)This PR was squashed before being merged into the 4.3-dev branch (closes #29813).Discussion----------[FrameworkBundle] Remove ControllerTrait::isFormValid()| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#24576 (comment)| License | MIT| Doc PR |**edit**: During the first draft I made, I did not use the request stack. I finally used it to mimic other shortcut! It was a bad idea. Now there is less code, it's simpler. Love it moreCommits-------2be1987ad1 [FrameworkBundle] Remove ControllerTrait::isFormValid()
…) (lyrixx)This PR was squashed before being merged into the 4.3-dev branch (closes#29813).Discussion----------[FrameworkBundle] Remove ControllerTrait::isFormValid()| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#24576 (comment)| License | MIT| Doc PR |**edit**: During the first draft I made, I did not use the request stack. I finally used it to mimic other shortcut! It was a bad idea. Now there is less code, it's simpler. Love it moreCommits-------2be1987 [FrameworkBundle] Remove ControllerTrait::isFormValid()
Uh oh!
There was an error while loading.Please reload this page.