Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[MonologBridge] Add support for Monolog 3#46153
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bcd91bb to7e81edeCompareUh oh!
There was an error while loading.Please reload this page.
stof commentedApr 25, 2022
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. |
derrabus commentedApr 25, 2022
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. |
Seldaek commentedApr 25, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 in22b7ae6 I added compatibility traits to get around BC break. |
Uh oh!
There was an error while loading.Please reload this page.
c4f1ffc to22b7ae6CompareUh oh!
There was an error while loading.Please reload this page.
6a6282c to7129c6fCompareSeldaek commentedApr 25, 2022
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. |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
Uh oh!
There was an error while loading.Please reload this page.
| * @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Seldaek commentedMay 2, 2022
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? |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 ;)
👍
nicolas-grekas commentedMay 4, 2022
Thank you@Seldaek. |
… (Seldaek)This PR was merged into the 6.1 branch.Discussion----------[MonologBridge] Fix LevelName being removed in Monolog 3.0| Q | A| ------------- | ---| Branch? | 6.1| Bug fix? | yes| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->A couple last fixes as follow-up to#46153 now that the Monolog 3.0.0 release is out.Commits-------7ba3e1a [MonologBridge] Fix LevelName being removed in Monolog 3.0
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.