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

Wrong/missing charset on JSON response #1607

Closed
FabulousGee opened this issue Feb 20, 2021 · 4 comments
Closed

Wrong/missing charset on JSON response #1607

FabulousGee opened this issue Feb 20, 2021 · 4 comments

Comments

@FabulousGee
Copy link
Contributor

FabulousGee commented Feb 20, 2021

I had some issues with Umlauts and so I double-checked everything on my side. All fine.
But when I checked the header-handling in atk I found a possible bug. When I set the charset on termination myself, it gets omitted.

I use the following line of code to output my JSON:

$this->_app->terminate(json_encode($data), ['Cache-Control' => 'no-transform,public,max-age=300,s-maxage=900', 'content-type' => 'application/json; charset=utf-8', 'Status' => ''.$code]);

Note the "charset=utf-8" to set the encoding (which should be standard on JSON but isn't necessary to set by HTTP guidelines)

When I now look at \atk4\ui\src\App.php I saw the content-type being hardcoded:

private function outputResponseJson($data, array $headers = []): void
    {
        if (!is_string($data)) {
            $data = $this->encodeJson($data);
        }
        $this->outputResponse(
            $data,
            array_merge($this->normalizeHeaders($headers), ['content-type' => 'application/json'])
        );
    }

I think it should be considered to either hardcode it to "['content-type' => 'application/json; charset=utf-8']" or check the original content-type more properly and reapply any attached strings again

FabulousGee added a commit to FabulousGee/ui that referenced this issue Feb 20, 2021
Check corresponding issue atk4#1607 for further discussion.

Maybe this should be made more generic?
@FabulousGee
Copy link
Contributor Author

FabulousGee commented Feb 21, 2021

@DarkSide666 @mvorisek regarding the discussion on my pull request, I want to continue the discussion about it here. The PR was just my first idea, I am sure there could be a better way. For example it would suffice to check, if the content-type has already been set and just skip it then (to let the user override it if they want to)

I am not into all of these things really, but I read a lot about it the last days and e.g. in this StackOverflow thread they mention that the charset (encoding) isn't implied as a default but furthermore should always be explicitely stated.
Also, there is a deeper conversation with some serious sources (like some HTTP specs) about the topic on the libraries page on GitHub as well: dart-lang/http#175 (comment)

So I still think a change on atk side should really be considered, no matter which way is preferred

@mvorisek
Copy link
Member

mvorisek commented Feb 22, 2021

Which browser/client does detect the encoding wrongly?

PS: your PR is for JSON mime, which should be defaulted to UTF-8. HTML5 has UTF-8 encoding as default too.

@FabulousGee
Copy link
Contributor Author

It's the HTTP library which I use in dart (Flutter) - I linked the issue above. Did you read through the post on their tracker? They describe very well which defaults apply and which do not.

I really understand your point, it doesn't seem necessary to set something which should be considered as default but actually it isn't (as described here )

And even if you insist on this being default behaviour, I don't agree with the way it's handled on atk4.
If I want to set my own charset, I am currently not able to do it in combination with the JSON output. If you take my first post again, IMHO the termination should use the charset I provide and not just delete my settings (that's how it's handled currently!)

@mvorisek
Copy link
Member

mvorisek commented Feb 22, 2021

You can extend App class and override the output/normalize headers method.

For me, it looks like an issue in Dart/Flutter. If you think is should be fixed in atk, please provide some data from major API libs/providers.

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

No branches or pull requests

2 participants