Fix symfony 5.2 api tests #5669

alexander-schranz · 2020-11-30T13:15:44Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Fixed tickets	fixes -
Related issues/PRs	symfony/symfony#38591
License	MIT
Documentation PR	-

What's in this PR?

A new jsonRequest trait to be used instead of client::request.

Why?

The json request need to be send as json body as symfony 5.2 browser kit does not longer allow none string values as parameters as they follow the http specification. So we need to do 5.2

Example Usage

Use search and replace to use the new method:

-$this->client->request(
+static::jsonRequest($this->client,

And:

-$client->request(
+static::jsonRequest($client,

danrot · 2020-12-01T08:12:36Z

src/Sulu/Bundle/CategoryBundle/Tests/Functional/Controller/KeywordControllerTest.php

@@ -104,7 +104,7 @@ public function testGet()
    {
        $keyword = $this->testPost('keyword1', 'de', $this->category1->getId());

-        $this->client->request(
+        static::jsonRequest($this->client,


The parameter should be in a separate line, like in all other places in this PR as well.

danrot · 2020-12-01T08:22:47Z

src/Sulu/Bundle/TestBundle/Testing/JsonRequestTrait.php

+     * @param mixed[] $parameters
+     * @param mixed[] $server
+     */
+    protected static function jsonRequest(KernelBrowser $client, string $method, string $uri, array $parameters = [], array $server = [], bool $changeHistory = true): Crawler


Still not a huge fan of traits in PHP in general... But apart from that, why is that function static? It would be a lot easier to use, because the $client would not have to be passed (other traits also add member variables). And what is the changeHistory parameter for?

Traits for tests are fine and we use here lot of traits to inject our logic into SuluTestCase but also use them in third party test cases like PantherTestCase.
The method parameters are the same as for the normal request and have the same defaults so there is no bc break and easy to refractor this project by just replace $this->client->request( with static::jsonRequest($this->client, , think we should make the upgrade for our uses as easy as possible.

I know that traits are kind of OK for tests, that's why I am not saying we should remove that trait. But I would still not make the function static, because it would be even easier to change. It would basically be replacing $this->client->request with $this->jsonRequest (or should it be $this->requestJson, to have the verb first in the method name?), and it would be easier to call, because there would be a parameter less to pass.

I would make it static as we don't know where the method are used sometimes you have things in the setUpBeforeClass and you can only call static function where needed. Please keep in mind this need to be refractored in all our projects this method is not just for us in our tests and I just want to make there the upgrade as easy as possible by doing the replace. As we make it not static this will make it more complicated as it should be and the project query something in static methods can not longer use them.

I think there should not be any requests in setUpBeforeClass, or are there? Apart from that, this is a newly introduced function, so people have to change their code anyway, it won't work out of the box in any case. Therefore I think it is not worth paying the high price of a more complicated function call.

Actually I think I would not even have introduced the trait at all, but would have fixed the code in places where it is needed (not too many tests were failing). But I can see why you would want to have a trait, but I wouldn't have introduced it, since it just more code we have to maintain. But if we are doing it, I would make it easy to use in the future, and not making it complicated to keep some kind of BC, which is not really there anyway.

@danrot

I think there should not be any requests in setUpBeforeClass, or are there?

Sadly yes there are projects which create fixtures over apis for tests instead of creating them manually over the entity manager.

not too many tests were failing

But if we look all api tests which sends the json as request parameters instead as content are false test as we don't really test if the Api works really with json content thats why I think it correct to update all tests as I think all the tests are false implemented and not really testing there how the apis are used.

I would make it easy to use in the future, and not making it complicated to keep some kind of BC

Another method would be is override the createClient method to write the $client into a private static property and has private static::getClient which symfony is using in there BrowserKitAssertionsTrait:

https://github.com/symfony/symfony/blob/ff97b5f17b63d5198cc56584623d1bb09f923b5b/src/Symfony/Bundle/FrameworkBundle/Test/BrowserKitAssertionsTrait.php#L144-L157

sadly we can currently not reuse the getClient of symfony as the method is private so we would need to implement the same behaviour by override the createClient method call there a setter or implement something like symfony does which call the getClient but this did look for me hackier then just give the client to the method.

Sounds more and more to me like we should fix that properly and use the client as Symfony intends it instead of creating a new abstraction that we have to maintain, is harder to understand and debug, and might lead to some problems in the future, especially if we also want to keep BC for that.

But if I am the only person thinking like that I am happy to be outvoted by @sulu/core-team.

I have just seen that you have contributed a similar function in Symfony. Why not implementing it now in a way that it is compatible with that one, so that we can delete our code once we don't need it anymore? Even if it is more hacky (not sure if that is the case, since I don't know the implementation details) I would prefer that, because it is less likely to stay in our codebase than the current solution.

I've just had a deeper look, and I think all we have to do in order to make the new jsonRequest function work you've introduced in your Symfony PR, is to decorate the test.client service from symfony, because that seems to be what the createClient method uses: https://github.com/symfony/symfony/blob/4.3/src/Symfony/Bundle/FrameworkBundle/Test/WebTestCase.php#L46

Then we can make use of that new function everywhere, and delete the code we copied from your Symfony PR (or maybe even the entire decorator) as soon as we only support Symfony versions containing this function. Or am I missing something?

danrot · 2020-12-01T08:24:43Z

src/Sulu/Bundle/TestBundle/Testing/JsonRequestTrait.php

+        if ('GET' !== $method) {
+            // JSON_THROW_ON_ERROR requires at least php 7.3
+            $content = \json_encode($parameters, \defined('JSON_THROW_ON_ERROR') ? \JSON_THROW_ON_ERROR : 0);
+            $parameters = [];


I don't really get what's happening here... Is it logical to the user calling this function? Wouldn't it maybe be cleaner to separate $content and $parameters?

GET can not have content so and are added as query parameter in symfony when its a GET request. When its POST or request Methods its converted by symfony into request parameter. Here is the difference now that we convert it into the content instead of request parameter. I will add a comment here so its clear.

But what if we want to do a POST request with query parameters and some content in the body? It's probably still possible by adding it as a string to the url like /admin?test=value, but the $parameters parameter would not work for that, even though I would have assumed that, since Symfony in general is working that way. For these reasons I would split the $parameters and $content, so that it works the same as in Symfony (you have also mentioned that the parameters are the same as in the Symfony request, so they should also work in the same way IMO.

@danrot also before when using POST query parameters has to be added as ?test=123 as symfony converts $parameters into request parameters. Did update to use the same logic as symfony and linked the code. So it should be more clear now what happens here that instead of converting to request it is converted into json for the specific methods.

…fony implementation in the future

alexander-schranz

We need to check if the definition exists with as the test.client is only available in test env:

        if ($container->hasDefinition('test.client')) {
            $container->getDefinition('test.client')->setClass(SuluKernelBrowser::class);
        }

alexander-schranz · 2020-12-02T16:11:36Z

src/Sulu/Bundle/MediaBundle/Tests/Functional/Controller/MediaWebsiteControllerTest.php

@@ -233,7 +233,7 @@ public function testResponseHeader()
        $date = new DateTime();
        $date->modify('+1 month');

-        $this->client->request(
+        $this->client->jsonRequest(


seems like i forgot this. this should be a normal request as it is no json api:

Suggested change

$this->client->jsonRequest(

$this->client->request(

danrot · 2020-12-02T16:16:01Z

Already realized the missing check 🙈 Also updated the request you've mentioned, that something I thought when I started working on this, but forgot to check afterwards 🙈

alexander-schranz force-pushed the bugfix/symfony-5-2-tests branch 3 times, most recently from 25bb58c to 7df0833 Compare November 30, 2020 13:33

alexander-schranz added the Bug Error or unexpected behavior of already existing functionality label Nov 30, 2020

alexander-schranz force-pushed the bugfix/symfony-5-2-tests branch 4 times, most recently from 3ec5dd2 to fbd924d Compare November 30, 2020 14:44

alexander-schranz added 3 commits November 30, 2020 16:14

Fix symfony 5.2 api tests

61be2ac

Update all request test methods

5b94e15

Fix flat true query parameter

1afb574

alexander-schranz force-pushed the bugfix/symfony-5-2-tests branch from fbd924d to 1afb574 Compare November 30, 2020 15:18

Prokyonn added 3 commits November 30, 2020 16:48

Fix tests

da1c1a0

Fix boolean query parameters

0ffb251

Fix mediabundle test for prefer lowest

a384faa

danrot reviewed Dec 1, 2020

View reviewed changes

Add comment for json request trait

cbeb1d0

alexander-schranz force-pushed the bugfix/symfony-5-2-tests branch from 717b6a8 to cbeb1d0 Compare December 1, 2020 09:17

Use same logic as in symfony for parameters

92a9819

alexander-schranz force-pushed the bugfix/symfony-5-2-tests branch from 0c0cd7f to 92a9819 Compare December 1, 2020 10:35

niklasnatter mentioned this pull request Dec 2, 2020

Fallback to content-locale of user in ResourceLocator field-type #5670

Merged

Use introduced jsonRequest function that can be replaced with the Sym…

39ec403

…fony implementation in the future

danrot force-pushed the bugfix/symfony-5-2-tests branch from 7ac3de0 to 39ec403 Compare December 2, 2020 16:13

alexander-schranz commented Dec 2, 2020

View reviewed changes

alexander-schranz mentioned this pull request Dec 2, 2020

Test dev dependencies for symfony 5.2 #5554

Closed

Request for image should not be json

7a3a1ec

danrot marked this pull request as ready for review December 2, 2020 16:17

danrot merged commit 79ea309 into sulu:release/2.1 Dec 2, 2020

alexander-schranz deleted the bugfix/symfony-5-2-tests branch December 3, 2020 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix symfony 5.2 api tests #5669

Fix symfony 5.2 api tests #5669

alexander-schranz commented Nov 30, 2020 •

edited

danrot Dec 1, 2020

danrot Dec 1, 2020

alexander-schranz Dec 1, 2020

danrot Dec 1, 2020

alexander-schranz Dec 1, 2020

danrot Dec 1, 2020

alexander-schranz Dec 1, 2020

danrot Dec 2, 2020

danrot Dec 2, 2020

danrot Dec 2, 2020

danrot Dec 1, 2020

alexander-schranz Dec 1, 2020

danrot Dec 1, 2020

alexander-schranz Dec 1, 2020

alexander-schranz left a comment

alexander-schranz Dec 2, 2020

danrot commented Dec 2, 2020

Fix symfony 5.2 api tests #5669

Fix symfony 5.2 api tests #5669

Conversation

alexander-schranz commented Nov 30, 2020 • edited

What's in this PR?

Why?

Example Usage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-schranz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danrot commented Dec 2, 2020

alexander-schranz commented Nov 30, 2020 •

edited