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

Improve error handling in Ajax #7922

Merged

Conversation

jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented Sep 5, 2022

Subject

I am targeting this branch, because error output for ajax calls is kinda broken atm and it is needed for page bundle.

Changelog

### Changed
- [BC break] Change json error output for ajax calls to create or edit admin endpoints.

@jordisala1991
Copy link
Member Author

To be able to properly fix this: sonata-project/SonataPageBundle#1495 (comment)

We will need to properly work with the errors. Currently there is no context on what field is generating the error.

On 3.x branch of AdminBundle, due to this return null: https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Controller/CRUDController.php#L1712-L1722 we were relying on html generated for the error page.

So, in 3.x of PageBundle:

If you are calling the createAction via ajax and the response is Success, it will arrive in the form of json, BUT if the response was failure, it was arriving in the form of HTML with the errors already in place.

It looks very bad, but now, in 4.x always the response is get via json, so without this PR it is impossible to know where to place each error. This PR gives each error a key which is the ID of the form field. (Also each field can have multiple errors, so we need to take into account that too.

@jordisala1991
Copy link
Member Author

wdyt @VincentLanglet ? Do you know any other solution for this?

src/Controller/CRUDController.php Outdated Show resolved Hide resolved
src/Controller/CRUDController.php Outdated Show resolved Hide resolved
src/Controller/CRUDController.php Outdated Show resolved Hide resolved
src/Controller/CRUDController.php Outdated Show resolved Hide resolved
return $errors;
}

private function buildName(FormInterface $form): string
Copy link
Member Author

Choose a reason for hiding this comment

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

Again, the same with buildId, it can't be reused by symfony. It is built on createView thanks to: https://github.com/symfony/symfony/blob/6.2/src/Symfony/Component/Form/Extension/Core/Type/BaseType.php#L49

@jordisala1991
Copy link
Member Author

I know that this needs probably to move some logic outside crud controller and test it and all, but does the code look good or should I find another way? @VincentLanglet

@jordisala1991 jordisala1991 force-pushed the hotfix/improve-ajax-error-parsing branch from dee070b to 03c9962 Compare September 19, 2022 07:40
@VincentLanglet
Copy link
Member

I know that this needs probably to move some logic outside crud controller and test it and all, but does the code look good or should I find another way? @VincentLanglet

Seems an OK strategy to me if it works for you.
I assume you plan to move the private method to a service.

@jordisala1991 jordisala1991 force-pushed the hotfix/improve-ajax-error-parsing branch from 03c9962 to 45b892f Compare September 23, 2022 06:51
@jordisala1991 jordisala1991 force-pushed the hotfix/improve-ajax-error-parsing branch from e0c214c to 2c93565 Compare September 26, 2022 08:53
@jordisala1991 jordisala1991 marked this pull request as ready for review September 26, 2022 08:58
@jordisala1991
Copy link
Member Author

This should be ready, have added tests, changelog and upgrade note.

Also took care of all comments and tried the compat with page bundle.

In the changelog and in the upgrade note is mentioned the possible (but unlikely) bc break.

Unlikely because I dont think anyone is using sonata as an api (and if they do, they probably got the same problems as we did on the page bundle: error output was broken)

@jordisala1991 jordisala1991 merged commit 52ec43b into sonata-project:4.x Sep 27, 2022
@jordisala1991 jordisala1991 deleted the hotfix/improve-ajax-error-parsing branch September 27, 2022 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants