Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for Monolog 2 #300

Merged
merged 1 commit into from Sep 2, 2019

Conversation

@derrabus
Copy link
Contributor

commented Apr 7, 2019

Allows installing Monolog 2 as soon as symfony/symfony#27905 is merged.

@derrabus derrabus force-pushed the derrabus:monolog2-compat branch from 6eb7e37 to a0ee880 Jul 1, 2019

@Seldaek

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

I wonder if this should error if monolog-bridge is in version <5 or not.. Otherwise it might be confusing if Composer lets you update to monolog 2 but then it fails to run.. Maybe worth making a new major release of the bundle for monolog2 + symfony5 support? Thoughts @lyrixx?

Although I guess the monolog-bridge 4.x will prevent installing monolog 2.x and that fixes it? Except for people using symfony/symfony still.

@lyrixx

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

I'm a bit lost :/ Is this possible to have every version work with every other version?
If not (or too hard), I'm fine with a next major release

@Seldaek

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

I think it's possible to have symfony5 + monolog1, but not symfony3/4 + monolog2 as the bridge is not going to be made compatible as far as I understand?

@derrabus

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Although I guess the monolog-bridge 4.x will prevent installing monolog 2.x and that fixes it?

Yes, MonologBridge 4.4 and below depends on Monolog 1 and thus will block the installation of Monolog 2.

Except for people using symfony/symfony still.

That practice is discouraged for over two years now, but yes: Composer won't prevent the installation of Monolog 2 in that case. Anything we can do to detect that case?

@derrabus

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

I think it's possible to have symfony5 + monolog1, but not symfony3/4 + monolog2 as the bridge is not going to be made compatible as far as I understand?

If my PR is merged as it is right now, that would be the plan.

@Seldaek

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Sounds fine to me to merge as is for now then and avoid a major version of the bundle.

@derrabus

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

Except for people using symfony/symfony still.

On second thought: This might still be true for Symfony 3.4 applications. In Symfony 3, it was still best practice to depend on the monorepo.

https://github.com/symfony/symfony-standard/blob/ffb00c045b980f8f721ddf5531c685bfcd79130c/composer.json#L26

I'll try to build some detection logic for that case.

@derrabus derrabus force-pushed the derrabus:monolog2-compat branch 2 times, most recently from 2fdd057 to 0ac0d84 Jul 9, 2019

@derrabus

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

I've added a check for the monorepo case. Composer cannot prevent the installation of symfony/symfony 3.4 alongside Monolog 2, but at least we can blow up the container compilation in that case. 🙈

@derrabus derrabus force-pushed the derrabus:monolog2-compat branch from 0ac0d84 to 501e3a2 Jul 9, 2019

@lyrixx

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

I've added a check for the monorepo case. Composer cannot prevent the installation of symfony/symfony 3.4 alongside Monolog 2, but at least we can blow up the container compilation in that case.

Why not using the conflict directive of composer?

@derrabus

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

Why not using the conflict directive of composer?

🤔 That could work as well.

In symfony/symfony:

"conflict": {
    "monolog/monolog": ">=2"
}

If we do this now, Symfony 3.4.30, 4.2.11 and 4.3.3 will have that conflict rule. Then we change composer.json of this PR into

"require": {
    "symfony/monolog-bridge": "^3.4.30|~4.2.11|^4.3.3|^5.0"
}

and we should be fine. Or am I thinking too complicated? 😅

@derrabus

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

See symfony/symfony#32469. If that PR gets merged, I will revert my changes to the extension.

@Seldaek

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

I think I'd merge as is.

@inverse

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

What's the best way to start working on adding support for new handlers that'll come in the v2 series? I know what application side needs to be wired up, but I guess this part of this PR will need to be solved first.

@Seldaek

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

I think we should first merge this (any objection @lyrixx) ? And then could add support for new handlers, but in the Extension class it should check that Monolog\Logger::API === 2 before allowing using them.

@derrabus derrabus force-pushed the derrabus:monolog2-compat branch from f84e993 to bc52eae Aug 16, 2019

@versh23

This comment has been minimized.

Copy link

commented Aug 30, 2019

monolog 2 has been released

@lyrixx
lyrixx approved these changes Sep 2, 2019

@Seldaek Seldaek merged commit 1a66ff0 into symfony:master Sep 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.