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
[FrameworkBundle] add isSubmittedFormValid
method to AbstractController
#54743
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
3a6e34d
to
33a79e8
Compare
src/Symfony/Bundle/FrameworkBundle/Tests/Controller/AbstractControllerTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/Tests/Controller/AbstractControllerTest.php
Outdated
Show resolved
Hide resolved
I find a bit strange to not differenciate isSubmitted() & isValid() 😐 And when a form is not submitted, any later call to "isValid()" will throw an exception afterwards. Meaning when this method will return false you will need aditional checks to define next steps / actions :| |
You made a good point for the prefix 'is' maybe another name like 'checkSubmitFormValid'. I think this function can be increase productivity by less writing code, it's optionnal and averyone can use public function new(): Response
{
$entity = new Article();
$form = $this->createForm(ArticleType::class, $article);
if ($this->isSubmittedFormValid($form)) {
// ...
}
return $this->render('@Twig/template.html.twig', [
'form' => $form, // Generate 422 status code if form is'nt valid (see doRender method)
]);
} And there is less risk of bugs for beginners by forgetting |
isSubmittedFormValid
method to AbstractController
I agree, but that's a solvable naming issue.
Yes, that's the danger with all of those utility functions that the abstract controller ships. However, developers can always look into how that method is implemented.
Okay, but if they do, what's the worst that could happen? The form is not processed at all? 🤷🏻♂️ |
Maybe renaming the method (Maybe just me, but i feel |
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.
This is a 👎 on my side.
/** | ||
* Call FormInterface::handleRequest() and return the result of the condition FormInterface::isSubmitted() and FormInterface::isValid(). | ||
*/ | ||
protected function isSubmittedFormValid(FormInterface $form): 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.
Honestly, I don't like this alternative. It makes it less obvious that we're dealing with a request here.
Since all methods involved concern the form only, I don't see why the controller should communicate with the form when the form can communicate directly with the request, making this shortcut helpful for controllers not extending AbstractController
.
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.
Like my old PR: #54726 ?
I agree with @yceruto 👎 |
Thank you for your interesting feedbacks, now, I don't know what I need to do :) |
@Oviglo I think, we won't find a solution that everybody's happy with. My personal recommendation would be to create a trait with this utility function in your own codebase and use that. I'm going to close this proposal because of too many downvotes. Thank you for giving it a try, though. |
I understand, thanks you all, I won personnal XP with your comments. |
In controllers, we often have to call three functions to validate a
form:handleRequest
,isSubmitted
andisValid
. I wrote a new method that calls handlerequest and return the result of the conditionisSubmitted() && isValid()