Skip to content

Allow monolog 3 #53480

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Allow monolog 3 #53480

wants to merge 1 commit into from

Conversation

DavidPrevot
Copy link
Contributor

@DavidPrevot DavidPrevot commented Jan 10, 2024 •
edited
Loading

Q A
Branch? 6.3
Bug fix? no
New feature? no
Deprecations? no
Issues N/A
License MIT

This is a follow up from #52222.

@carsonbot carsonbot added this to the 5.4 milestone Jan 10, 2024
@xabbuh
Copy link
Member

xabbuh commented Jan 10, 2024

Are you sure about this? The PHP code in #52222 was adjusted for 6.3 while your PR targets 5.4.

@DavidPrevot
Allow monolog 3
d7a6593 
This is a follow up from 720444c.
@DavidPrevot DavidPrevot changed the base branch from 5.4 to 6.3 January 14, 2024 11:55
@DavidPrevot
Copy link
Contributor Author

Are you sure about this? The PHP code in #52222 was adjusted for 6.3 while your PR targets 5.4.

You’re right, thanks a lot, PR updated.

@derrabus
Copy link
Member

Can you elaborate which problem this change is going to solve? If we merge this PR, we will lose test coverage for Monolog 2.

@lyrixx lyrixx removed their request for review January 15, 2024 09:24
@DavidPrevot
Copy link
Contributor Author

Can you elaborate which problem this change is going to solve?

This change should allow one to install symfony/symfony with version 3 of monolog/monolog. It aims to update the symfony/symfony “metapackage”: since it replaces (among others) symfony/monolog-bridge that requires monolog/monolog in version ^1.25.1|^2|^3, it shouldn’t restrict the version required to ^1.25.1|^2 (no other package replaced actually require any version of monolog/monolog).

If we merge this PR, we will lose test coverage for Monolog 2.

That would be unfortunate, and not at all the expected outcome, but that seems to outline an issue in the test coverage workflow.

derrabus
derrabus requested changes Jan 16, 2024
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should allow one to install symfony/symfony with version 3 of monolog/monolog.

No. First of all, don't install symfony/symfony. Ever. This practice has been discouraged for half a decade now.

Secondly, we're talking about a dependency in require-dev. Changing the version range here has absolutely zero impact on projects that use symfony/symfony as a dependency.

That would be unfortunate

Indeed.

but that seems to outline an issue in the test coverage workflow.

Maybe. If you have a better idea how to solve that, I'm all ears. Note that in Symfony 7, the issue is solved because Monolog 3 is the only supported version there.

@xabbuh
Copy link
Member

xabbuh commented Jan 18, 2024

I am closing for the reasons given by @derrabus. Thank you for understanding.

@xabbuh xabbuh closed this Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus requested changes

@yceruto yceruto Awaiting requested review from yceruto

@chalasr chalasr Awaiting requested review from chalasr

@dunglas dunglas Awaiting requested review from dunglas

@OskarStark OskarStark Awaiting requested review from OskarStark

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Status: Needs Work Status: Waiting feedback
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
4 participants
@DavidPrevot @xabbuh @derrabus @carsonbot