Skip to content

[HttpFoundation] Add named constructor on JsonResponse #19552

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

Merged
merged 1 commit into from
Aug 16, 2016

Conversation

tyx
Copy link
Contributor

@tyx tyx commented Aug 5, 2016

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

To make easier construction with raw json. It is a simple addition but having something like :

return new JsonResponse($content, Response::HTTP_OK, [], true);

and

return new JsonResponse($content, Response::HTTP_OK, []);

is not very obvious to get the difference without going to read the JsonResponse constructor.

@@ -59,6 +59,14 @@ public static function create($data = null, $status = 200, $headers = array())
}

/**
* Make easier the creation of JsonResponse from raw json
*/
public static function fromJsonString($data = null, $status = 200, $headers = array())
Copy link
Contributor

@ro0NL ro0NL Aug 6, 2016

Choose a reason for hiding this comment

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

What about createFromStringcreateFromJsonString? or just createFromJson

Copy link
Contributor

@ro0NL ro0NL Aug 7, 2016

Choose a reason for hiding this comment

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

Btw createFromString somewhat implies a callback (JSONP) is parsed as well.. so you know ;-)

Edit: i guess we dont wanna go that way.

Copy link
Contributor Author

@tyx tyx Aug 8, 2016

Choose a reason for hiding this comment

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

No problem to switch to createFromJsonString. I'm used to remove the create prefix but if everyone prefer with it, I'm ok !

To make easier construction with raw json
@tyx tyx force-pushed the feature/named-constructor-json-string branch from 4a14137 to 6d5a65d Compare August 8, 2016 08:00
@fabpot
Copy link
Member

fabpot commented Aug 16, 2016

👍

1 similar comment
@nicolas-grekas
Copy link
Member

👍

@fabpot
Copy link
Member

fabpot commented Aug 16, 2016

Thank you @tyx.

@fabpot fabpot merged commit 6d5a65d into symfony:master Aug 16, 2016
fabpot added a commit that referenced this pull request Aug 16, 2016
… (tyx)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[HttpFoundation] Add named constructor on JsonResponse

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

To make easier construction with raw json. It is a simple addition but having something like :
```php
return new JsonResponse($content, Response::HTTP_OK, [], true);
```

and
```php
return new JsonResponse($content, Response::HTTP_OK, []);
```

is not very obvious to get the difference without going to read the JsonResponse constructor.

Commits
-------

6d5a65d Add named constructor on JsonResponse
@tyx tyx deleted the feature/named-constructor-json-string branch August 16, 2016 15:50
@fabpot fabpot mentioned this pull request Oct 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants