NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Fix jsonRequest in`AbstractBrowser` for GET methods#39281

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

alexander-schranz wants to merge2 commits intosymfony:6.3fromalexander-schranz:bugfix/abstract-browser-json-request

Closed

Fix jsonRequest in`AbstractBrowser` for GET methods#39281

alexander-schranz wants to merge2 commits intosymfony:6.3fromalexander-schranz:bugfix/abstract-browser-json-request

Conversation

Copy link

Contributor

alexander-schranz commentedDec 2, 2020•
edited by nicolas-grekas
Loading

Q	A
Branch?	5.x
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #...
License	MIT
Doc PR	symfony/symfony-docs#...

When implementing#38596 I was not aware of that $parameters are also used as queryParameters and are converted based on the Methodhere into query or request data. This I think should be handled the same way for the jsonRequest also instead of just unset the server variables we should to reset to the predefined server variables.

carsonbot added Status: Needs Review Bug labels

Dec 2, 2020

alexander-schranz force-pushed thebugfix/abstract-browser-json-request branch 3 times, most recently fromcc1747f to4d90a87Compare

December 2, 2020 09:59

Fix jsonRequest in AbstractBrowser for GET Methods

589d06f

alexander-schranz force-pushed thebugfix/abstract-browser-json-request branch from4d90a87 to589d06fCompare

December 2, 2020 10:07

alexander-schranz changed the title~~Fix jsonRequest in AbstractBrowser for GET Methods~~[BrowserKit] Fix jsonRequest in AbstractBrowser for GET Methods

Dec 2, 2020

nicolas-grekas reviewed

Dec 10, 2020

View reviewed changes

src/Symfony/Component/BrowserKit/AbstractBrowser.php OutdatedShow resolvedHide resolved

Remove JSON_THROW_ERROR conditional to behave the same in all PHP Ver…

0411c79

…sions

nicolas-grekas added this to the5.x milestone

Dec 23, 2020

Copy link

ContributorAuthor

alexander-schranz commentedJan 8, 2021

@nicolas-grekas anything missing here from my side?

nicolas-grekas approved these changes

Jan 26, 2021

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(failures are false-positives)

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Jan 26, 2021

nicolas-grekas requested changes

Jan 26, 2021

View reviewed changes

src/Symfony/Component/BrowserKit/AbstractBrowser.php

		break;
		default:
		$content =null;
		$query =$parameters;

Copy link

Member

nicolas-grekasJan 26, 2021•
edited
Loading

Choose a reason for hiding this comment

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

wait, I'm not sure anymore that this makes sense:$parameters doesn't go through the processing ofjson_encode().
That means objects (that do or don't implementJsonSerializable) are going to be passed differently when usingGET vs other verbs.

Copy link

ContributorAuthor

alexander-schranzJan 26, 2021

Choose a reason for hiding this comment

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

Yeah based on the method we set it a request body or not. This is based on the linked logic which symfony currently use here:https://github.com/symfony/symfony/blob/v5.2.0/src/Symfony/Component/HttpFoundation/Request.php#L388-L404
It does not make sense to convert query parameters to JSON as nobody will converted it back. Which is in the FosRestBundle and ApiPlatform converted into the$request->request attributes by theBodyListener.

carsonbot added Status: Needs Work and removed Status: Reviewed labels

Jan 26, 2021

Copy link

ContributorAuthor

alexander-schranz commentedApr 26, 2021•
edited
Loading

@nicolas-grekas any update to this I think this still should match the same logic as the http foundationRequest object, we are using this in@sulu and without this change it was really buggy and not usable in all cases with GET.

OskarStark changed the title~~[BrowserKit] Fix jsonRequest in AbstractBrowser for GET Methods~~[BrowserKit] Fix jsonRequest inAbstractBrowser for GET methods

Aug 4, 2021

fabpot modified the milestones:5.4,6.1

Nov 16, 2021

Copy link

ContributorAuthor

alexander-schranz commentedMay 19, 2022

@nicolas-grekas I still think this change is valid because of:

symfony/src/Symfony/Component/HttpFoundation/Request.php

Lines 375 to 391 ind303433

	switch (strtoupper($method)) {
	case'POST':
	case'PUT':
	case'DELETE':
	if (!isset($server['CONTENT_TYPE'])) {
	$server['CONTENT_TYPE'] ='application/x-www-form-urlencoded';
	}
	// no break
	case'PATCH':
	$request =$parameters;
	$query = [];
	break;
	default:
	$request = [];
	$query =$parameters;
	break;
	}

And because of GET, HEAD, OPTION parameters are query strings and the content has no meaning:https://stackoverflow.com/questions/978061/http-get-with-request-body

But think this will not being merged as it is now open a long time?

fabpot modified the milestones:6.1,6.2

May 20, 2022

nicolas-grekas modified the milestones:6.2,6.3

Nov 5, 2022

maxhelias approved these changes

Feb 16, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Feb 16, 2023

carsonbot changed the title~~[BrowserKit] Fix jsonRequest inAbstractBrowser for GET methods~~Fix jsonRequest inAbstractBrowser for GET methods

Feb 16, 2023

Copy link

Member

nicolas-grekas commentedFeb 21, 2023

I think nobody understands the change :)
Also, GET with body can exist, eg with Elasticsearch
I'm closing because this didn't get traction and it's always possible to use another method for GET when you want that behavior I suppose.