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] Monolog 2 compatibility#27905
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
Seldaek commentedNov 19, 2018
Hey, I'd appreciate if you can check how this fares against latest dev-master of monolog. Interfaces are now considered stable and while there are a few things I still want to look at before releasing, I'm trying to wrap up soon (and definitely before Symfony 4.3). I'd like to make sure it won't prevent Symfony projects from upgrading before I tag anything though. |
stof commentedNov 20, 2018
what is broken here ? |
derrabus commentedNov 20, 2018
@Seldaek I can look into this on the weekend. |
derrabus commentedNov 20, 2018
|
ebffece to75c52f2Comparederrabus commentedDec 2, 2018
I've updated the PR for the latest master of Monolog. |
Uh oh!
There was an error while loading.Please reload this page.
7e243b7 to085cf24Comparederrabus commentedApr 6, 2019
I've updated the PR during the |
Uh oh!
There was an error while loading.Please reload this page.
derrabus commentedApr 6, 2019
@Seldaek Have you ever considered making php 7.2 a requirement for Monolog 2? That would save us quite some pain here. 😅 |
Seldaek commentedApr 8, 2019
I already bumped from 7.0 to 7.1.. I am not sure what the 7.2 change would help with, but I feel like that's a bit much. I really need to get there with finalizing monolog 2 though :/ |
stof commentedMay 16, 2019
@Seldaek PHP 7.2 is less strict than PHP 7.1 about adding/removing typehints in child classes for some cases. See the PR description. |
Seldaek commentedMay 16, 2019
Ok well I guess it could be considered as the release was delayed so much anyway.. But it means it won't be compatible with Symfony 4 requirements at all then. |
derrabus commentedMay 29, 2019
Since it does not look like this will be merged into 4.3 anymore: I would suggest to rebase the patch against master, set all handlers to final and apply the interface changes required for Monolog 2. That would mean that version 5.0 of the bridge would be compatible with Monolog 1 and 2 while the Bridge 4.4 would support Monolog 1 only. |
Seldaek commentedJun 30, 2019
@derrabus if 4.4 is not an option then I guess this sounds good. I will try and push for a 2.0 again.. sorry for the delay. |
Seldaek commentedJun 30, 2019
Sorry my comment above was regarding the symfony 5/4 note, but regarding monolog requiring php 7.2 I am still not 100% sure. Will need to think about it some more. I'll add an issue too to see if anyone has feedback. |
derrabus commentedJul 1, 2019
I've updated the PR for master. It should be safe to merge now.
If we made Monolog 2 support a Symfony-5-only feature, the php requirement of Monolog would not matter anymore because Symfony 5 itself will require php 7.2. |
Seldaek commentedJul 8, 2019
Looks all good to me from a quick glance.. |
nicolas-grekas commentedJul 8, 2019
There's no way to M2 compatible with SF4.4, right? |
Seldaek commentedJul 8, 2019
I think it was mostly possible, at least if you are on php 7.2, if not I think then there was a hack needed for swiftmailer handler?@derrabus probably has more of an overview.. I'd definitely prefer having Symfony 4.4 support Monolog 2 as well, but regardless Monolog 1 is super stable at this point so keeping maintenance up for a few more years is not that much work. |
fabpot commentedJul 8, 2019
Thank you@derrabus. |
This PR was merged into the 5.0-dev branch.Discussion----------[MonologBridge] Monolog 2 compatibility| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets |#27857| License | MIT| Doc PR | N/ASee#27857 for the discussion.This PR adds return types to methods that need to have one in Monolog 2.Notes:* The PR will break userland handlers that extend handlers from Monolog Bridge.* I was unable to come up with a php 7.1 compatible version of `SwiftMailerHandler` that works with Monolog 1 and Monolog 2 because a method we're overriding now has a `string` type-hint on a parameter that wasn't there before. I've „solved“ this with a version switch, but I feel dirty doing this. 🙈 If you have a better solution, please enlighten me.Commits-------ca1cfec Monolog 2 compatibility.
derrabus commentedJul 8, 2019
Monolog 2 forces us to add return types to several classes. That is a BC break if userland code extends classes from the Monolog bridge. If we wanted to have Monolog 2 in Symfony 4.4, we could either…
|
nicolas-grekas commentedJul 8, 2019
|
Uh oh!
There was an error while loading.Please reload this page.
See#27857 for the discussion.
This PR adds return types to methods that need to have one in Monolog 2.
Notes:
SwiftMailerHandlerthat works with Monolog 1 and Monolog 2 because a method we're overriding now has astringtype-hint on a parameter that wasn't there before. I've „solved“ this with a version switch, but I feel dirty doing this. 🙈 If you have a better solution, please enlighten me.