[FrameworkBundle] [DX] Add `Controller::json` method to make it easy to send json #17642

mcfedr · 2016-02-01T17:38:23Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

Its currently a awkward to use Serializer component to send a JsonResponse.

I have tried two approaches

use Serializer::normalize and JsonResponse
use Serializer::serialize and a plain Response, and set the content-type

In either cases there is need for a custom json function so as not to repeat yourself and there are disadvantages.

In the first case you are only partly using Serializer and any custom Encoder would be skipped
In the second you are not making use of JsonResponse, particular disadvantage if you want to support JSONP.

This new json method uses the serializer component is enabled it is used to generate the json data, and falls back to normal JsonResponse when its not.

stof · 2016-02-01T18:05:41Z

src/Symfony/Bundle/FrameworkBundle/Response/JsonSerializedResponse.php

this is broken in case you call the setter after setting data

dunglas · 2016-02-01T20:11:25Z

Why not something like https://github.com/dunglas/DunglasApiBundle/blob/1.x/Controller/ResourceController.php#L79-L93?

I agree with @stof, Response should be a value object. The serializer should not be injected in it. An helper method in the controller like this one API Platform would be better IMO. Btw, we should wait for a decision about #16863 before adding new methods to the controller IMO.

mcfedr · 2016-02-01T20:43:22Z

Now I look at it again, I completely agree on the weirdness of inserting the Serializer into the Response.

How about what I have done now? Its very similar to how DunglasApiBundle does it, but keeping the JsonResponse.

dunglas · 2016-02-01T20:53:08Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

What about renderJson or something similar to existing methods?

json is consistent with stream to return a stream response.

I was looking at redirect and stream as similar names, also renderView for example returns a string, not a Response

mcfedr · 2016-02-01T21:08:37Z

@dunglas That was a long read! Would certainly wait until a decision is made there. I see some real value in breaking up an every growing Controller class.

fabpot · 2016-02-29T16:46:52Z

LGTM. ping @symfony/deciders

xabbuh · 2016-03-01T07:25:03Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+            $json = $this->container->get('serializer')->serialize($data, 'json', array_merge(array(
+                'json_encode_options' => JsonResponse::DEFAULT_ENCODING_OPTIONS,
+            ), $context));
+


~~The fourth argument should be removed.~~

Sorry, I missed the change below.

But you now have to increase the required version of symfony/http-foundation.

mcfedr · 2016-03-01T07:58:53Z

src/Symfony/Bundle/FrameworkBundle/composer.json

        "symfony/config": "~2.8|~3.0",
        "symfony/event-dispatcher": "~2.8|~3.0",
-        "symfony/http-foundation": "~2.8|~3.0",
+        "symfony/http-foundation": "~3.1",


@xabbuh is this the best way to do it? I see at least one other component has a ~3.0 requirement, I assume this is expected to change as new functionality is added to Symfony 3

yes, that's right

mcfedr · 2016-03-01T08:23:10Z

@xabbuh Does anything else need to be done?

Taluu · 2016-03-01T12:26:57Z

src/Symfony/Component/HttpFoundation/JsonResponse.php

-    protected $encodingOptions = 15;
+    const DEFAULT_ENCODING_OPTIONS = 15;
+
+    protected $encodingOptions = self::DEFAULT_ENCODING_OPTIONS;


IRC, the possibility to assign a property from a constant value is available only on php >= 5.6, and Sf3 is php >= 5.5...

As far as I can tell this usage has always been part of PHP 5. It certainly works in 5.5. You made me doubt it and I ran the tests for this class in 5.5 to double check!

I know that from PHP 7 its possible to write this how I would have really liked

const DEFAULT_ENCODING_OPTIONS = JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT

Ow, yeah I may have mistaken with arithmetics such as const DEFAULT_ENCODING_OPTIONS = JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT indeed. My bad !

xabbuh · 2016-03-01T14:25:44Z

👍

mcfedr · 2016-03-01T19:20:14Z

@xabbuh does the 'needs work' label still need to be here?

xabbuh · 2016-03-01T19:58:34Z

@mcfedr No, you're right.

Status: Reviewed

nicolas-grekas · 2016-03-02T12:53:02Z

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

+3.1.0
+-----
+
+ * Added `Controller:json` to simplify creating JSON responses when using the Serializer component


typo: missing double colon

If the serializer component is enabled it is used to generate the json data, if not the standard `json_encode` function is used

fabpot · 2016-03-03T13:19:06Z

Thank you @mcfedr.

…o make it easy to send json (mcfedr) This PR was merged into the 3.1-dev branch. Discussion ---------- [FrameworkBundle] [DX] Add `Controller::json` method to make it easy to send json | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Its currently a awkward to use Serializer component to send a `JsonResponse`. I have tried two approaches 1. use `Serializer::normalize` and `JsonResponse` 1. use `Serializer::serialize` and a plain `Response`, and set the `content-type` In either cases there is need for a custom `json` function so as not to repeat yourself and there are disadvantages. 1. In the first case you are only partly using `Serializer` and any custom `Encoder` would be skipped 1. In the second you are not making use of `JsonResponse`, particular disadvantage if you want to support JSONP. This new `json` method uses the serializer component is enabled it is used to generate the json data, and falls back to normal `JsonResponse` when its not. Commits ------- f904a2b Add a Controller function to make it easy to return json

taylorotwell · 2016-03-03T17:41:30Z

This breaks BC by adding a new property to public setData method of JsonResponse, which Laravel overrides.

taylorotwell · 2016-03-03T17:41:57Z

src/Symfony/Component/HttpFoundation/JsonResponse.php

     * @throws \InvalidArgumentException
     */
-    public function setData($data = array())
+    public function setData($data = array(), $preEncoded = false)


weaverryan · 2016-03-04T00:59:12Z

I agree - it looks like this violates our BC policy - http://symfony.com/doc/current/contributing/code/bc.html#changing-classes - sub-item "Public Methods -> Add argument with a default value". I don't think we allow a change like this.

dunglas · 2016-03-04T05:59:25Z

What we can do instead is adding a new public setPreEncoded method.

nicolas-grekas · 2016-03-04T06:15:57Z

Or "setJson" :-) which should be called by setData internally btw

fabpot · 2016-03-04T07:30:28Z

I think the setJson() method is indeed a good solution. @nicolas-grekas Can you work on a PR?

@taylorotwell

This PR was squashed before being merged into the 3.1-dev branch (closes #18006). Discussion ---------- [HttpFoundation] Fix BC break introduced by #17642 | Q | A | ------------- | --- | Branch | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17642 | License | MIT | Doc PR | n/a @taylorotwell can you confirm it fixes the issue with Laravel? Commits ------- 73ef995 [HttpFoundation] Fix BC break introduced by #17642

HeahDude · 2016-04-01T05:55:43Z

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

+3.1.0
+-----
+
+ * Added `Controller::json` to simplify creating JSON responses when using the Serializer component


Missing [BC break] ?

AFAIK, it's not a BC break.

Agreed, sorry I got confused by #18373.

…n() method (derrabus) This PR was merged into the 3.1-dev branch. Discussion ---------- [FrameworkBundle] Upgrade notice for the Controller::json() method | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A PR #17642 added a `json()` method to the `Controller` class. This might break existing code extending the class. This PR adds a note about this to the UPGRADE-3.1 document. Commits ------- ca6694a Upgrade notice for the Controller::json() method.

avtehnik · 2016-04-20T12:32:16Z

Awsome!

Etheriq · 2016-04-20T12:32:45Z

nice job, Fred!

carsonbot added the Status: Needs Review label Feb 1, 2016

mcfedr force-pushed the add-controller-json branch from 0ccff59 to fe42ab8 Compare February 1, 2016 17:45

stof reviewed Feb 1, 2016
View reviewed changes

mcfedr force-pushed the add-controller-json branch from fe42ab8 to 11bc151 Compare February 1, 2016 20:40

dunglas reviewed Feb 1, 2016
View reviewed changes

mcfedr force-pushed the add-controller-json branch 3 times, most recently from 273a896 to ecd3587 Compare February 5, 2016 08:07

xabbuh reviewed Mar 1, 2016
View reviewed changes

xabbuh added Status: Needs Work and removed Status: Needs Review labels Mar 1, 2016

mcfedr force-pushed the add-controller-json branch from ecd3587 to c37096a Compare March 1, 2016 07:55

mcfedr reviewed Mar 1, 2016
View reviewed changes

mcfedr force-pushed the add-controller-json branch from c37096a to d4511eb Compare March 1, 2016 12:04

Taluu reviewed Mar 1, 2016
View reviewed changes

xabbuh removed the Status: Needs Work label Mar 1, 2016

carsonbot added the Status: Reviewed label Mar 1, 2016

nicolas-grekas reviewed Mar 2, 2016
View reviewed changes

Add a Controller function to make it easy to return json

f904a2b

If the serializer component is enabled it is used to generate the json data, if not the standard `json_encode` function is used

mcfedr force-pushed the add-controller-json branch from d4511eb to f904a2b Compare March 2, 2016 14:31

javiereguiluz added Feature FrameworkBundle DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Mar 3, 2016

fabpot merged commit f904a2b into symfony:master Mar 3, 2016

taylorotwell reviewed Mar 3, 2016
View reviewed changes

dunglas added a commit to dunglas/symfony that referenced this pull request Mar 4, 2016

[HttpFoundation] Fix BC break introduced by symfony#17642

f54f7d2

dunglas mentioned this pull request Mar 4, 2016

[HttpFoundation] Fix BC break introduced by #17642 #18006

Closed

fabpot pushed a commit that referenced this pull request Mar 4, 2016

[HttpFoundation] Fix BC break introduced by #17642

73ef995

derrabus mentioned this pull request Mar 30, 2016

[FrameworkBundle] Upgrade notice for the Controller::json() method #18373

Merged

HeahDude reviewed Apr 1, 2016
View reviewed changes

wouterj mentioned this pull request Apr 2, 2016

[3.1] Document Controller#json() helper symfony/symfony-docs#6425

Closed

fabpot mentioned this pull request May 13, 2016

Release v3.1.0-BETA1 #18776

Merged

goetas mentioned this pull request Oct 28, 2016

Not compatible with Symfony 3's Controller::json() schmittjoh/serializer#663

Closed

Uh oh!

[FrameworkBundle] [DX] Add Controller::json method to make it easy to send json #17642

[FrameworkBundle] [DX] Add Controller::json method to make it easy to send json #17642

Uh oh!

Conversation

mcfedr commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Feb 1, 2016

Uh oh!

mcfedr commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcfedr commented Feb 1, 2016

Uh oh!

fabpot commented Feb 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcfedr commented Mar 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Mar 1, 2016

Uh oh!

mcfedr commented Mar 1, 2016

Uh oh!

xabbuh commented Mar 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Mar 3, 2016

Uh oh!

taylorotwell commented Mar 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Mar 4, 2016

Uh oh!

dunglas commented Mar 4, 2016

Uh oh!

nicolas-grekas commented Mar 4, 2016

Uh oh!

fabpot commented Mar 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avtehnik commented Apr 20, 2016

Uh oh!

Etheriq commented Apr 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

[FrameworkBundle] [DX] Add `Controller::json` method to make it easy to send json #17642

[FrameworkBundle] [DX] Add `Controller::json` method to make it easy to send json #17642