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

fix ConsoleFormatter - call to a member function format() on string #31326

Merged
merged 1 commit into from May 1, 2019

Conversation

@keksa
Copy link
Contributor

keksa commented Apr 30, 2019

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

The ConsoleFormatter crashes when there is not a DateTime object in $record['datetime']. As this parameter is not documented anywhere (i.e. FormatterInterface does not say it must be a DateTime object), I think the proper fix is to check if there is DateTimeInterface object and only call the format method then.

We use a custom LogProcessor (https://symfony.com/doc/current/logging/processors.html) to add some extra data and format the DateTime in the $record['datetime']. We need to format the DateTime in the processor, because we use JsonFormatter in prod environment and it does not support changing the date format. We use ConsoleFormatter only in dev environment and as the processor is called before the formatter, we get the crash.

There were no tests whatsoever for ConsoleFormatter, so I've added a basic one, that passes before and after, and another one that tests the crash (failed before, passed after).

There is a theoretical BC break, as someone could have sent an object with a format method to the formatter and it would have worked, but I'm not sure if it's considered BC break by Symfony, please let me know, if it is.


namespace Symfony\Bridge\Monolog\Tests\Formatter;

use DateTime;

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 30, 2019

Contributor

we dont use root classes :) should be prefixed with \

This comment has been minimized.

Copy link
@keksa

keksa Apr 30, 2019

Author Contributor

fixed, thank you

@keksa

This comment has been minimized.

Copy link
Contributor Author

keksa commented Apr 30, 2019

The failing tests seem unrelated, they're failing in other PRs too.

@ro0NL
ro0NL approved these changes Apr 30, 2019
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Apr 30, 2019
@fabpot
fabpot approved these changes May 1, 2019
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented May 1, 2019

Thank you @keksa.

@fabpot fabpot merged commit 6488328 into symfony:3.4 May 1, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details
fabpot added a commit that referenced this pull request May 1, 2019
…on string (keksa)

This PR was merged into the 3.4 branch.

Discussion
----------

fix ConsoleFormatter - call to a member function format() on string

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

The ConsoleFormatter crashes when there is not a DateTime object in `$record['datetime']`. As this parameter is not documented anywhere (i.e. `FormatterInterface` does not say it must be a DateTime object), I think the proper fix is to check if there is DateTimeInterface object and only call the `format` method then.

We use a custom LogProcessor (https://symfony.com/doc/current/logging/processors.html) to add some extra data and format the DateTime in the `$record['datetime']`. We need to format the DateTime in the processor, because we use `JsonFormatter` in prod environment and it does not support changing the date format. We use `ConsoleFormatter` only in dev environment and as the processor is called before the formatter, we get the crash.

There were no tests whatsoever for `ConsoleFormatter`, so I've added a basic one, that passes before and after, and another one that tests the crash (failed before, passed after).

There is a theoretical BC break, as someone could have sent an object with a `format` method to the formatter and it would have worked, but I'm not sure if it's considered BC break by Symfony, please let me know, if it is.

Commits
-------

6488328 fix ConsoleFormatter - call to a member function format() on string
@keksa keksa deleted the keksa:feature/console-formatter-datetime-fix branch May 1, 2019
This was referenced May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.