[FrameworkBundle] add isSubmittedFormValid method to AbstractController

[Oviglo]

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues
License	MIT

In controllers, we often have to call three functions to validate a form:handleRequest, isSubmitted and isValid. I wrote a new method that calls handlerequest and return the result of the condition isSubmitted() && isValid()

[smnandre]

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 :|

[yceruto]

For reference, this same feature was implemented on the controller before #24576 and reverted in #29813.

[smnandre]

For reference, this same feature was implemented on the controller before #24576 and reverted in #29813.

Thank you, very interesting reading!

Most convincing arguments (imho):

• prefix is should not do something
• it will be harder to learn or explain

[Oviglo]

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 handleRequest() and isSubmitted() && isValid() process or write like this:

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 handleRequest

[derrabus]

prefix is should not do something

I agree, but that's a solvable naming issue.

it will be harder to learn or explain

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.

---

And there is less risk of bugs for beginners by forgetting handleRequest

Okay, but if they do, what's the worst that could happen? The form is not processed at all? 🤷🏻‍♂️

[smnandre]

Maybe renaming the method isFormSubmittedAndValid would better describe what it does ?

(Maybe just me, but i feel isSubmittedForm imply the form is submitted)

[yceruto]

This is a 👎 on my side.

[yceruto]

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.

[Oviglo]

Like my old PR: #54726 ?

[OskarStark]

I agree with @yceruto 👎

[Oviglo]

Thank you for your interesting feedbacks, now, I don't know what I need to do :)

[derrabus]

@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.

[Oviglo]

I understand, thanks you all, I won personnal XP with your comments.