[MonologBridge] Fix Monolog/Logger final deprecation #49759
Conversation
This shouldn't break any BC. An adapter for the public Monolog\Logger methods has been created as a trait. The trait has been created as "deprecated", so it can be removed later. This change depends on a change in the upstream Monolog class. An MR has been opened for monolog to cover that
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
Hey! Thanks for your PR. You are targeting branch "6.3" but it seems your PR description refers to branch "6.3 (maybe 6.2?)". Cheers! Carsonbot |
|
This would still break cases where objects of this class are passed around but the called code expects an instance of Monolog's |
|
That's true.
What if I leave the extends for 6.3, and remove the inheritance for 7.0?
…On Sat, Mar 25, 2023 at 09:09, Christian Flothmann - notifications at github.com ***@***.***> wrote:
This would still break cases where objects of this class are passed around but the called code expects an instance of Monolog's Logger class (which we now no longer would extend).
—
Reply to this email directly, [view it on GitHub](#49759 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAHMSUPJKEKUJNMTIHRUCJ3W52Y2ZANCNFSM6AAAAAAWCYFLKA).
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
There are still two paint points with that solution:
|
It wouldn't go away for the next minor versions, but with a clear idea of when its going to be fixed.
I'm not sure tbh. The best solution I see is to deprecate the entire class, and replace it with something like "SymfonyLogger". Besides that, I've pushed a few changes that, from my point of view, would be enough to trigger a deprecation when extending the original Monolog\Logger. |
|
I don't think that this is the right approach. Why do we need the |
| "minimum-stability": "dev", | ||
| "repositories": [ | ||
| { | ||
| "type": "vcs", | ||
| "url": "git@github.com:joaojacome/monolog" | ||
| } | ||
| ] |
There was a problem hiding this comment.
| "minimum-stability": "dev", | |
| "repositories": [ | |
| { | |
| "type": "vcs", | |
| "url": "git@github.com:joaojacome/monolog" | |
| } | |
| ] | |
| "minimum-stability": "dev" |
possibly unintended highjack of the monoglog-bridge installation by source :)
There was a problem hiding this comment.
ah this was intended - this fix depends on a change in Monolog. This would be removed if this MR (and the Monolog one) were to be accepted.
|
the right fix is to deprecate the MonologBridge Logger class and use the Monolog Logger directly for the logger. I will soon open a PR on MonologBundle to stop using that class (which will remove the deprecation warning as the class won't be used anymore and will allow deprecating the class in Symfony 6.4) |
Updated MonologBridge\Logger so it doesn't extend Monolog\Logger
This shouldn't break any BC. An adapter for the public Monolog\Logger methods has been created as a trait. The trait has been created as "deprecated", so it can be removed later.
This change depends on a change in the upstream Monolog class. An MR has been opened for monolog to cover that. (Seldaek/monolog#1796)