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

Disabling formatter JSON pretty-print actually toggles instead of disabling #1469

Closed
judgej opened this issue Jun 2, 2020 · 1 comment
Closed
Labels

Comments

@judgej
Copy link

judgej commented Jun 2, 2020

Monolog version 2.1.0 (used by Laravel 7.0)

https://github.com/Seldaek/monolog/blob/master/src/Monolog/Formatter/NormalizerFormatter.php#L102

$this->jsonEncodeOptions ^= JSON_PRETTY_PRINT

The NormalizerFormatter::setJsonPrettyPrint() function takes a boolean that should either enable or disable the "pretty print" formatting. I'm assuming that is the intention, though it is not explicly documented.

However, the parameter provided has this effect instead:

  • true - set JSON "pretty print" on.
  • false - toggle JSON "pretty print" on or off.

I believe the fix should be:

$this->jsonEncodeOptions &= ~JSON_PRETTY_PRINT;

I need to test that that further, in case the number of bits set for the inverse of JSON_PRETTY_PRINT is not enough for the datatype of $this->jsonEncodeOptions, requiring perhaps some longint casting (just a hunch, but I really haven't dug into it that deeply yet), but it works for me as a quick test.

There is a workaround at present:-

To enable JSON pretty print:

$formatter->setJsonPrettyPrint(true);

To disable JSON pretty print:

$formatter->setJsonPrettyPrint(true);
$formatter->setJsonPrettyPrint(false);

i.e. toggle from a known state.

Happy to provide a PR if you aggree with this, including some documentation clarifications. I guess it is not urgent, having been around for a long time, and there is a workaround (which I am using here) that will also be forward-compatible with this fix.

@judgej judgej added the Bug label Jun 2, 2020
@judgej
Copy link
Author

judgej commented Jun 2, 2020

I guess this is a follow-on from issue #1139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant