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

Add correct accept header to ajaxSubmit to prevent 406 error #6222

Merged
merged 2 commits into from
Jul 29, 2020

Conversation

nieuwenhuisen
Copy link
Contributor

@nieuwenhuisen nieuwenhuisen commented Jul 23, 2020

Add correct accept header to ajaxSubmit to prevent 406 error

The modeltype save request gives a 406 (Not acceptable) error in my browser. This is causes because my browser sends an wildcard Accept header */* and the code checks on an exact match of application/json. To fix this I set the accept header before the ajax request.

I am targeting this branch, because the deprecation is introduce in 3.x and the error is given in the master branch.

Changelog

### Fixed
- Set the Accept header for ajaxSubmit in `Resources/views/CRUD/Association/edit_many_script.html.twig` to  "application/json" to prevent 406 (Not acceptable)  error.
- CRUDController::handleXmlHttpRequestErrorResponse also accepts the wildcard Accept header  "*/*".
- CRUDController::handleXmlHttpRequestSuccessResponse also accepts the wildcard Accept header  "*/*".

@nieuwenhuisen nieuwenhuisen force-pushed the accept-wildcard branch 2 times, most recently from 75e66d6 to 3704d47 Compare July 23, 2020 16:03
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@nieuwenhuisen nieuwenhuisen changed the base branch from master to 3.x July 23, 2020 16:04
@phansys phansys added the patch label Jul 23, 2020
jordisala1991
jordisala1991 previously approved these changes Jul 24, 2020
@nieuwenhuisen
Copy link
Contributor Author

The test are failing, I am working on that.

@nieuwenhuisen
Copy link
Contributor Author

nieuwenhuisen commented Jul 24, 2020

Schermafbeelding 2020-07-24 om 09 07 40
If I edit the deprecation error test, I am getting a syntax error. I have tried with escaping but without success (*//* and *\/*). Anyone an idea how to solve this?

This is error is caused because the use of a closing phpdoc tag. But this is not working:
New 1.2.0rc1: If you need to use the closing comment "/" in a DocBlock, use the special escape sequence "{@}."

@nieuwenhuisen nieuwenhuisen changed the title Also accept the wildcard Accept header for application/json Add correct accept header to ajaxSubmit to prevent 406 error Jul 24, 2020
@nieuwenhuisen
Copy link
Contributor Author

I have changed the solution strategy for this probleem, to a simpler solution.

@jordisala1991
Copy link
Member

jordisala1991 commented Jul 24, 2020

Schermafbeelding 2020-07-24 om 09 07 40
If I edit the deprecation error test, I am getting a syntax error. I have tried with escaping but without success (*//* and *\/*). Anyone an idea how to solve this?

This is error is caused because the use of a closing phpdoc tag. But this is not working:
New 1.2.0rc1: If you need to use the closing comment "/" in a DocBlock, use the special escape sequence "{@}."

To fix that you could have used the builtin phpunit methods, instead of that syntax (which is deprecated AFAIK).

$this->expectDeprecation(..)

@nieuwenhuisen
Copy link
Contributor Author

nieuwenhuisen commented Jul 24, 2020

@jordisala1991 Yes I have tried that, but is not working correctly in all versions. See https://travis-ci.org/github/sonata-project/SonataAdminBundle/jobs/711372401

What do you think of the new solution with Javascript. In this solution there is no PHP changes needed.

@jordisala1991
Copy link
Member

That could be a problem with phpunit-brdige 5.1.0 🤔

I like the new solution with only js changes, just wondering if someone in some case can still end up with that problem because of the restriction on php

@phansys
Copy link
Member

phansys commented Jul 24, 2020

The problem related to ExpectDeprecationTrait::expectDeprecation() was solved at symfony/symfony#37153, which is released since version 5.1.1.

@jordisala1991 jordisala1991 requested review from jordisala1991 and a team July 25, 2020 19:20
@phansys
Copy link
Member

phansys commented Jul 25, 2020

I like the new solution with only js changes, just wondering if someone in some case can still end up with that problem because of the restriction on php

I think we should make both changes:

  • Send the proper header in the request (Accept: application/json);
  • Allow to deliver JSON responses to requests accepting anything (Accept: */*).

@greg0ire
Copy link
Contributor

greg0ire commented Jul 26, 2020

You said you were getting a 406, yet the code in the controller does not produce a 406 yet, it will only do so in next major. Please link to the piece of code you get a 406 from (use the profiler to find the controller that is involved, etc.)

@nieuwenhuisen
Copy link
Contributor Author

Yes the 406 is produced in the master branch, I have switched the base branch. The 3.x branch only triggers a deprecation.
https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Controller/CRUDController.php#L1443

@VincentLanglet
Copy link
Member

.idea files should not commited.

@sonata-project/contributors Shouldn't we add this in .gitignore ?

@nieuwenhuisen
Copy link
Contributor Author

Yeah I added the .idea folder by mistake :-( It's now untracked

jordisala1991
jordisala1991 previously approved these changes Jul 29, 2020
@phansys
Copy link
Member

phansys commented Jul 29, 2020

.idea files should not commited.

@sonata-project/contributors Shouldn't we add this in .gitignore ?

I really don't like the idea (:trollface:) to track every possible IDE configuration path or other residual files which are related to the specific environment or the operating system.
For those cases, the users should be responsible to keep track of these paths, which are surely commonly present in every project they work, through a global .gitignore.

phansys
phansys previously approved these changes Jul 29, 2020
tests/Controller/CRUDControllerTest.php Outdated Show resolved Hide resolved
tests/Controller/CRUDControllerTest.php Outdated Show resolved Hide resolved
phansys
phansys previously approved these changes Jul 29, 2020
franmomu
franmomu previously approved these changes Jul 29, 2020
@nieuwenhuisen nieuwenhuisen dismissed stale reviews from franmomu and phansys via 8f7c365 July 29, 2020 10:10
@phansys phansys requested a review from franmomu July 29, 2020 10:12
@franmomu franmomu merged commit 7d92242 into sonata-project:3.x Jul 29, 2020
@franmomu
Copy link
Member

Thanks @nieuwenhuisen !

@fancyweb
Copy link
Contributor

Ref #5710 (comment)

This commit broke SonataPageBundle a little more. SonataPageBundle uses the CRUDController and does all edit with ajax requests with the */* Accept header. SonataPageBundle javascript only handle a HTML return, not a json one when there are errors.

@nieuwenhuisen
Copy link
Contributor Author

See #6250

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.

8 participants