Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Return 422 status code when the form fails #488

Merged

Conversation

Zales0123
Copy link
Contributor

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
Contributor

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
Contributor

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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Bug Confirmed bugs or bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants