php-http / message

HTTP Message related tools
http://php-http.org
MIT License
1.29k stars 41 forks source link

FullHttpMessageFormatter should be smarter about formatting binary request/response bodies #126

Closed ostrolucky closed 4 years ago

ostrolucky commented 4 years ago

Like most websites, we have some file uploads. We also use Monolog to store logs into Google's Stackdriver. The way how this works is that Google encodes such log record via json_encode. This fails for such requests. Specificially, this is what you get: PHP Warning: InvalidArgumentException: json_encode error: Malformed UTF-8 characters, possibly incorrectly encoded. I can imagine lot of people with different loggers will have problems with your formatters in combination with binary content, not mentioning that it doesn't make much sense to store these messages as binary in traditional log files anyway as well.

What I would suggest is that your FullHttpMessageFormatter detects if content is binary similar as CurlCommandFormatter does and if that's the case, just do bin2hex. I can prepare PR if you are OK with this solution.

dbu commented 4 years ago

yeah, that sounds like a good idea to do. we could also just output a text that says its binary data, but as this is the FullHttpMessageFormatter, bin2hex sounds like a cool solution. i wonder if we then should truncate the bin2hex result, or just output something like -- body is 123456 bytes of binary data -- if the body is longer than the maximum length.

ostrolucky commented 4 years ago

That's a good question. But I would say it's a different thing unrelated to binary problem, because right now FullHttpMessageFormatter does just simple cut and doesn't care about informing user in a log that it indeed did some cutting. So we can perhaps take care of that in separate PR.

dbu commented 4 years ago

a fragment of a binary stream is likely less useful than a fragment of json or form-data, but i don't mind either way. doing just the bin2hex and then keep the normal flow would indeed be easiest, we can start with that.