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

[MonologBridge] Fix compatibility of ServerLogHandler with Monolog 2 #34697

Merged
merged 1 commit into from Nov 29, 2019

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Nov 28, 2019

Q A
Branch? 5.0
Bug fix? yes
New feature? no
Deprecations? no
Tickets #34520
License MIT
Doc PR NA

This is an alternative to #34521 that keep compatibility with "processors" and "formaters".

@nicolas-grekas
Copy link
Member

If we can prevent the BC break, it's better, isn't it?

@jderusse
Copy link
Member Author

Done

@xabbuh
Copy link
Member

xabbuh commented Nov 29, 2019

I fail to see how this prevents the BC break when Monolog 2 is installed. In that case we still add the return type declaration.

@jderusse
Copy link
Member Author

In Monolog 2, the interfaces ProcessableHandlerInterface and FormattableHandlerInterface have been removed from AbstractHandler`.

If you don't ilmplement them, things won't behave as expected

Your PR has a bug, as the method formatRecord call getFormater which does not exists anymore

$recordFormatted = $this->getFormatter()->format($record);

the BC break is coming from Monolog: 2. if one stays at Monolog 1, there is no PB break
if one moves to 2, we follow Monolog2 API.

@xabbuh
Copy link
Member

xabbuh commented Nov 29, 2019

sounds reasonable 👍

@nicolas-grekas
Copy link
Member

Thank you @jderusse.

nicolas-grekas added a commit that referenced this pull request Nov 29, 2019
… Monolog 2 (jderusse)

This PR was merged into the 5.0 branch.

Discussion
----------

[MonologBridge] Fix compatibility of ServerLogHandler with Monolog 2

| Q             | A
| ------------- | ---
| Branch?       | 5.0
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #34520
| License       | MIT
| Doc PR        | NA

This is an alternative to #34521 that keep compatibility with "processors" and "formaters".

Commits
-------

bdb10f7 Fix compatibility with Monolog 2
@nicolas-grekas nicolas-grekas merged commit bdb10f7 into symfony:5.0 Nov 29, 2019
@fabpot fabpot mentioned this pull request Dec 1, 2019
nicolas-grekas added a commit that referenced this pull request Dec 3, 2019
This PR was merged into the 4.3 branch.

Discussion
----------

[MonologBridge] Add test on ServerLogHandler

| Q             | A
| ------------- | ---
| Branch?       | 5.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | /
| License       | MIT
| Doc PR        | /

After writing #34697 (comment) I realized that  ServerLogHandler wasn't tested.

Tell me if it's a BugFix and should be rebased on 4.3

Commits
-------

8c7947f Add test on ServerLogHandler
@jderusse jderusse deleted the fix-34520 branch March 5, 2020 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants