-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] add isSubmittedRequestValid function #54726
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
Conversation
Hey! Thanks for your PR. You are targeting branch "7.1" but it seems your PR description refers to branch "7.1 for features / 5.4, 6.4, or 7.0 for bug fixes". Cheers! Carsonbot |
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.
LGTM!
There is a minor CS remaining spotted by fabbot https://fabbot.io/report/symfony/symfony/54726/6b8a8b268f32cd4037a04d217aa8bb551774cf51 |
https://fabbot.io/patch/symfony/symfony/54726/6b8a8b268f32cd4037a04d217aa8bb551774cf51/exception_messages.diff |
I'm not entirely convinced, we need this feature, tbh. My biggest concern is that we change a well established interface and add an unnecessary method to it. If your main motivation is saving a few keystrokes of boilerplate in controllers, let's add that method to the |
This is an interesting observation, I can make a new PR to write this in AbstractController: $form = $this->createForm(FormType::class);
if ($this->isSubmittedFormValid($form, $request)) {
} I don't think this will be useful except to not forget the |
@@ -416,6 +417,13 @@ public function handleRequest(mixed $request = null): static | |||
return $this; | |||
} | |||
|
|||
public function isSubmittedRequestValid(?Request $request = null): 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.
This signature breaks the component's abstraction, btw: handleRequest()
has no opinion on how a request is represented, but this method surprisingly requires an HttpFoundation request.
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 changed this at @yceruto 's request: (cf: #54726 (comment))
If we want to have such a utility function in the core at all, I think |
It rewrite the method in AbstractController: #54743 |
In controllers, we often have to call three functions to validate a form:
handleRequest
, ìsSubmittedand
isValid. I wrote a new method that calls
handlerequestand return the result of the condition
isSubmitted() && isValid()`