Skip to content
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

Return 422 status code when the form fails #488

Merged

Conversation

Zales0123
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets replaces #469
License MIT

I've taken over the #469 PR, added some tests and clean up the code. Thank you, @belmeopmenieuwesim, for the contribution 🖖

@Zales0123 Zales0123 added the Bug Confirmed bugs or bugfixes. label Oct 18, 2022
@Zales0123 Zales0123 requested a review from a team as a code owner October 18, 2022 13:47
@Zales0123 Zales0123 force-pushed the return-422-status-code-when-form-fails branch from 78d37aa to 2f3dfc2 Compare October 18, 2022 13:50
@@ -212,6 +212,9 @@ public function createAction(Request $request): Response

return $this->redirectHandler->redirectToResource($configuration, $newResource);
}
if ($request->isMethod('POST') && $form->isSubmitted() && !$form->isValid()) {
Copy link
Member

Choose a reason for hiding this comment

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

Small improvement - we could add a blank line before if and extract the content of the check to the separate method

Suggested change
if ($request->isMethod('POST') && $form->isSubmitted() && !$form->isValid()) {
if ($this->isFormInValid($request, $form)) {

'science_book[author][lastName]' => $newBookAuthorLastName,
]);

$this->assertResponseCode($this->client->getResponse(), Response::HTTP_UNPROCESSABLE_ENTITY);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it is not bc-break and if we didn't send a 400 error. But looking at the code and related PR, I assume we didn't, so I'm fine with this change

@lchrusciel lchrusciel merged commit 27f4cf3 into Sylius:1.10 Oct 18, 2022
@Zales0123 Zales0123 deleted the return-422-status-code-when-form-fails branch October 18, 2022 14:05
Zales0123 added a commit that referenced this pull request Oct 18, 2022
- [#341](#341) Dropping usage of Request->get ([@loic425](https://github.com/loic425), [@Zales0123](https://github.com/Zales0123))
- [#450](#450) Adjust when some runtime deprecation notices are triggered and use Symfony's trigger_deprecation() helper ([@mbabker](https://github.com/mbabker))
- [#467](#467) [README] Add development section and update links ([@lchrusciel](https://github.com/lchrusciel))
- [#478](#478) Add tests with grids ([@loic425](https://github.com/loic425))
- [#487](#487) Make CsrfTokenManager public ([@Zales0123](https://github.com/Zales0123))
- [#488](#488) Return 422 status code when the form fails ([@belmeopmenieuwesim](https://github.com/belmeopmenieuwesim), [@Zales0123](https://github.com/Zales0123))
lchrusciel added a commit to Sylius/Sylius that referenced this pull request Oct 19, 2022
…les0123)

This PR was merged into the 1.12 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.12                 |
| Bug fix?        | yes                                                       |
| New feature?    | no                                                       |
| BC breaks?      | no                                                       |
| Deprecations?   | no |
| Related tickets |  |
| License         | MIT                                                          |

[This change](Sylius/SyliusResourceBundle#488) resulted in a different response code which we checks in our Behats. It\'s a hot-fix, as we should change it in the SyliusResourceBundle itself 💃 But I would like to unblock builds

Commits
-------

5eca8c6 [Hot-fix] Do not fail scenario after 422 response page
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bugs or bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants