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.

Sign up for GitHub

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

Jump to bottom

[BrowserKit] Add jsonRequest function to the browser-kit client #38596

Merged
merged 1 commit into from
Nov 11, 2020
Merged

[BrowserKit] Add jsonRequest function to the browser-kit client #38596

merged 1 commit into from
Nov 11, 2020

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranz alexander-schranz commented Oct 16, 2020 •
edited
Loading

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

If you use the FOSRestBundle for your Api's you have maybe many tests using just:

$client->request('POST', '/api/contacts', ['param' => 1]);

To test your JSON api as in the real browser request FOSRestBundle converts the json body into the $request->request object. I think something similar is done by ApiPlatform. If you have tests like above they will now fail as the integer is converted to string see also #38591.

This PR add a new jsonRequest which will look like the following and will so fix the above problem:

$client->jsonRequest('POST', '/api/contacts', ['param' => 1]);

@alexander-schranz alexander-schranz force-pushed the feature/browser-kit-json-request branch 2 times, most recently from e57df9b to 4fdf2fa Compare October 16, 2020 09:45
@alexander-schranz alexander-schranz mentioned this pull request Oct 16, 2020
Closed
@chalasr chalasr added this to the 5.x milestone Oct 16, 2020
@alexander-schranz alexander-schranz force-pushed the feature/browser-kit-json-request branch 2 times, most recently from 4fdf2fa to e2edfc1 Compare October 16, 2020 09:59
@Nyholm
Copy link
Member

Nyholm commented Oct 16, 2020

Thank you

I like the idea of this because I know there are a lot of parameters to set when you want to test some custom requests. I am not sure it is a good idea to hard code the content type to application/json. I build all my APIs with Json:API format which requires a content type of application/vnd.api+json. So I cannot leverage this feature in the current state.

Just FYI. There is a small package from @nebkam that I like. It has a RequestBuilder to solve the same problem as this PR is trying to solve.

yceruto
yceruto approved these changes Oct 16, 2020
View reviewed changes
Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

LGTM.

@Nyholm about to hard code the content type to application/json I think it's fine as default value. Anyway, you'll can change these server parameters through the $server argument if you want.

symfony/src/Symfony/Component/BrowserKit/AbstractBrowser.php

Line 353 in 6088728

$server = array_merge($this->server, $server);

@nicolas-grekas nicolas-grekas changed the title Add jsonRequest function to the browser-kit client [BrowserKit] Add jsonRequest function to the browser-kit client Oct 19, 2020
nicolas-grekas
nicolas-grekas reviewed Oct 19, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas 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.

Thanks. Don't miss adding a changelog entry in the component.

@alexander-schranz
Copy link
Contributor Author

@nicolas-grekas changelog is updated.

fabbot is failing because of #38596 (comment) and #38596 (comment).

Let me now if I need to change something.

nicolas-grekas
nicolas-grekas reviewed Oct 20, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas 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.

What about @Nyholm's comment?
(yes you can ignore fabbot here, the report is a false-positive)

src/Symfony/Component/BrowserKit/CHANGELOG.md Outdated Show resolved Hide resolved
src/Symfony/Component/BrowserKit/AbstractBrowser.php Outdated Show resolved Hide resolved
@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented Oct 20, 2020 •
edited
Loading

@Nyholm I think we should go here with application/json as we do the same in JsonResponse which sets there the Content-Type to application/json and I think this both should match.

@alexander-schranz alexander-schranz force-pushed the feature/browser-kit-json-request branch 2 times, most recently from 4948af9 to b1123d2 Compare October 20, 2020 12:30
@alexander-schranz alexander-schranz mentioned this pull request Oct 20, 2020
Merged
derrabus added a commit that referenced this pull request Oct 20, 2020
@derrabus
minor #38646 Refractor AbstractBrowserTest to assertSame (alexander-s…
b2821ec 
…chranz)

This PR was merged into the 4.4 branch.

Discussion
----------

Refractor AbstractBrowserTest to assertSame

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #...instead -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

As mentioned by @nicolas-grekas at #38596 (comment) the AbstractBrowserTest should be refractored to use `assertSame` instead of `assertEquals` to compare string.

Commits
-------

3f320c8 Refractor AbstractBrowserTest to assertSame
javiereguiluz
javiereguiluz approved these changes Oct 22, 2020
View reviewed changes
src/Symfony/Component/BrowserKit/AbstractBrowser.php Outdated
@@ -161,6 +161,24 @@ public function xmlHttpRequest(string $method, string $uri, array $parameters =
}
}

/**
* Converts the request parameters into a json string and used it as request content.
Copy link
Member

Choose a reason for hiding this comment

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

json string -> JSON string

and used it -> and uses it

@chalasr chalasr force-pushed the feature/browser-kit-json-request branch from 52efab2 to c2fa2cb Compare November 11, 2020 13:22
@chalasr
Copy link
Member

chalasr commented Nov 11, 2020

Thank you @alexander-schranz.

@chalasr chalasr mentioned this pull request Nov 11, 2020
Closed
@chalasr chalasr merged commit 16fb94b into symfony:5.x Nov 11, 2020
@alexander-schranz alexander-schranz deleted the feature/browser-kit-json-request branch November 11, 2020 13:24
beongohl997
beongohl997 reviewed Nov 11, 2020
View reviewed changes
Copy link

@beongohl997 beongohl997 left a comment

Choose a reason for hiding this comment

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

A

beongohl997
beongohl997 reviewed Nov 11, 2020
View reviewed changes
Copy link

@beongohl997 beongohl997 left a comment

Choose a reason for hiding this comment

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

src/Symfony/Component/BrowserKit/AbstractBrowser.php

@alexander-schranz alexander-schranz mentioned this pull request Dec 2, 2020
Closed
@fabpot fabpot mentioned this pull request Apr 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@jderusse jderusse jderusse left review comments

@derrabus derrabus derrabus left review comments

@beongohl997 beongohl997 beongohl997 left review comments

@javiereguiluz javiereguiluz javiereguiluz approved these changes

@yceruto yceruto yceruto approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
BrowserKit Feature Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

None yet