Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:masterfromderrabus:monolog-2-compat
Jul 8, 2019

Conversation

@derrabus
Copy link
Member

@derrabusderrabus commentedJul 9, 2018
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?yes
Deprecations?no
Tests pass?yes
Fixed tickets#27857
LicenseMIT
Doc PRN/A

See#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 ofSwiftMailerHandler that works with Monolog 1 and Monolog 2 because a method we're overriding now has astring 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.

@Seldaek
Copy link
Member

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.

dbrumann reacted with thumbs up emoji

@stof
Copy link
Member

* SwiftMailerHandler is broken with Monolog 2 and php 7.1.

what is broken here ?

@derrabus
Copy link
MemberAuthor

Hey, I'd appreciate if you can check how this fares against latest dev-master of monolog.

@Seldaek I can look into this on the weekend.

@derrabus
Copy link
MemberAuthor

what is broken here ?

@stof seethis comment

@derrabusderrabusforce-pushed themonolog-2-compat branch 4 times, most recently fromebffece to75c52f2CompareDecember 2, 2018 11:49
@derrabus
Copy link
MemberAuthor

I've updated the PR for the latest master of Monolog.

@derrabusderrabusforce-pushed themonolog-2-compat branch 3 times, most recently from7e243b7 to085cf24CompareApril 6, 2019 16:26
@derrabusderrabus changed the title[MonologBridge] WIP: Monolog 2 compatibility[MonologBridge] Monolog 2 compatibilityApr 6, 2019
@derrabus
Copy link
MemberAuthor

I've updated the PR during theEU-FOSSA hackathon. With my modifications, I was able to integrate Monolog 2 into a Symfony 4 application.

@derrabus
Copy link
MemberAuthor

@Seldaek Have you ever considered making php 7.2 a requirement for Monolog 2? That would save us quite some pain here. 😅

dbrumann reacted with thumbs up emoji

@Seldaek
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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
Copy link
MemberAuthor

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
Copy link
Member

@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
Copy link
Member

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
Copy link
MemberAuthor

I've updated the PR for master. It should be safe to merge now.

regarding monolog requiring php 7.2 I am still not 100% sure.

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
Copy link
Member

Looks all good to me from a quick glance..

@nicolas-grekas
Copy link
Member

There's no way to M2 compatible with SF4.4, right?
This means M1 should accept bug fixes for 3-4 more years ideally.

@Seldaek
Copy link
Member

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
Copy link
Member

Thank you@derrabus.

@fabpotfabpot merged commitca1cfec intosymfony:masterJul 8, 2019
fabpot added a commit that referenced this pull requestJul 8, 2019
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
Copy link
MemberAuthor

There's no way to M2 compatible with SF4.4, right?

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…

  • deprecate the affected classes and introduce new ones with return types or…
  • document the BC break and pray that userland code does not extend the bridge classes.

@derrabusderrabus deleted the monolog-2-compat branchJuly 8, 2019 10:38
@nicolas-grekas
Copy link
Member

  • or conditionally load an implementation with types when PHP 7.2 is used. but let's make things simple for now at least - until someone explains why the current way is not enough.
derrabus reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

+2 more reviewers

@ro0NLro0NLro0NL left review comments

@dbrumanndbrumanndbrumann left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

9 participants

@derrabus@Seldaek@stof@nicolas-grekas@fabpot@ro0NL@dbrumann@michaelcullum@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp