[MonologBridge] Add support for Monolog 3 #46153
Conversation
Seldaek
force-pushed
the
monolog3
branch
2 times, most recently
from
bcd91bb
to
7e81ede
Compare
|
The change being a BC break for classes extending Symfony handlers is also the reason why Symfony 4.x never added support for Monolog 2. |
That would be a very unfortunate timeframe. We could work with conditional traits to achieve compatibility for Monolog 2 + 3 and maintain BC if Monolog 2 is installed. Ideally, we could even drop Monolog 2 support in Symfony 7.0 then. And yes, implementations of external interfaces should be final. |
Indeed waiting for Symfony 7 would kinda suck, especially as Symfony 6.1 bumping PHP to 8.1 is what motivated me to do the same in Monolog and take advantage of readonly props & enums.. Dropping Monolog 1/2 in Symfony 7 though absolutely would be a good idea. Anyway in 22b7ae6 I added compatibility traits to get around BC break. |
Seldaek
force-pushed
the
monolog3
branch
2 times, most recently
from
c4f1ffc
to
22b7ae6
Compare
Seldaek
force-pushed
the
monolog3
branch
3 times, most recently
from
6a6282c
to
7129c6f
Compare
|
OK tests pass now on Monolog 1/2/3 for me. The psalm errors are bogus and should partially be resolved once it can run against Monolog 3. |
What's the plan for Symfony 7? Provide support for only monolog 3? We should then figure out a way to throw deprecations when monolog 1/2 is used (once monolog 3 is out)
| @@ -19,9 +20,13 @@ | |||
| * | |||
| * @author Dany Maillard <danymaillard93b@gmail.com> | |||
| * @author Igor Timoshenko <igor.timoshenko@i.ua> | |||
| * | |||
| * @internal since Symfony 6.1 | |||
There was a problem hiding this comment.
shouldn't be internal as this abstract class is meant to be used also by userland to me
There was a problem hiding this comment.
Can we finalize the __invoke() method instead? It is the implementation of an external contract and not being able to change its signature was one of the challanges we've faced here.
There was a problem hiding this comment.
if CompatibilityProcessor is not internal as I suggested, then ppl should use it and implement doInvoke instead,
this would solve the issue without requiring final/internal
| * | ||
| * @author Jordi Boggiano <j.boggiano@seld.be> | ||
| * | ||
| * @internal |
There was a problem hiding this comment.
Shouldn't we make all the traits NON-internal, to make it easy for third parties to provide monolog 1+2+3 compat?
There was a problem hiding this comment.
IMO most people can easily add the new LogRecord or simply do __invoke($record) without type hints and have it work with everything. This is only a problem in symfony due to the strict BC policy and I suspect most impl can get away with just changing this or supporting only monolog 3 going forward.. So I don't think it's worth having this stuff in the public api, but if the core team consensus is to do it I can update the PR.
|
Yes I think I'd add a deprecation for monolog 1/2 in 6.2 or 6.3 latest. Doing it in 6.1 would be a bit early probably. I'll definitely try to get a release of monolog out this week anyway, I hope this PR can be finalized/merged soon as well. It should be fairly easy to do in the if/else we have for the traits, no? |
Let's ignore my comment about @internal: while making things internal might be too closed (think open/closed principle), we can wait for actual feedback from userland before considering it is ;)
|
Thank you @Seldaek. |
Adds support for the upcoming version of Monolog. I mostly wanted to do this before releasing it to ensure it is possible to integrate with it and Monolog 2 using the same codebase with only minimal amounts of version-based forks.
I think the result is pretty good, so I will try and keep pushing for a beta release soon. I still need to add a SymfonyMailerHandler to replace the SwiftMailerHandler which got axed though.
To try it with Monolog 3 one needs to switch the requirement to
^3@devfor now, but I did that locally and all tests pass with v3.