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.

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

III-4424 Improve json encoding #202

Merged
merged 18 commits into from
Jan 26, 2022

Conversation

LucWollants
Copy link
Contributor

@LucWollants LucWollants commented Jan 26, 2022

Added

  • Added Json class to have a more consistent way of encoding and decoding to Json

Changed

  • Replaced occurrences of json_encode with Json::encode
  • Replaced occurrences of json_decode with Json::decode
  • Boy scouting on the changed classes

Ticket: https://jira.uitdatabank.be/browse/III-4424

@LucWollants LucWollants marked this pull request as ready for review January 26, 2022 11:32
@@ -30,7 +31,7 @@ public static function fromPsr7Response(ResponseInterface $response): self
public function withData($data)
{
$body = $this->response->getBody();
$body->write(json_encode($data, self::JSON_OPTIONS));
$body->write(Json::encodeWithOptions($data, self::JSON_OPTIONS));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the most important usages because it determines how the JSON response of searches is encoded. It's interesting then that exactly this encode uses extra options which are not standardized in the Json class. Maybe we should add it as a encodeForHttpResponse() or something like that so it's clear it should be used in that context, and it will always use the right options for http responses then? Also applicable to udb3-silex I think, where we encode JSON for responses, but also for serializing things in our database or queuing messages for example. So it might be good to differentiate between context, like encodeForHttpResponse() and encodeForSerialization() for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I changed the specific case to encodeForHttpResponse, if we get more flavours better naming is indeed needed.

Copy link
Contributor

@bertramakers bertramakers Jan 26, 2022

Choose a reason for hiding this comment

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

Yes in this app that is sufficient. I meant that in udb3-silex it might be good to add different flavors when we replace all usages of json_encode() there. I think right now we always use Json::encode() which just uses THROW_ON_ERROR. But even if it's always the same right now, it might be good to already differentiate a bit based on context so we can easily add options later depending on the context.

Base automatically changed from feature/III-4424-boy-scouting to main January 26, 2022 13:06
@LucWollants LucWollants merged commit 941751a into main Jan 26, 2022
@LucWollants LucWollants deleted the feature/III-4424-improve-json-encoding branch January 26, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants