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

Prepare compat with Monolog 2 #27857

Open
nicolas-grekas opened this Issue Jul 5, 2018 · 13 comments

Comments

Projects
None yet
6 participants
@nicolas-grekas
Member

nicolas-grekas commented Jul 5, 2018

The master branch of Monolog has some breaking changes discussed in Seldaek/monolog#903

We should open a PR on branch 3.4 that allows ~2.0 of monolog/monolog and make it green, ie fix our handlers to make them compatible with both versions.

We can also consider Monolog 2 compat as a new feature for 4.2. That'd be less friendly as ppl using 3.4 won't be able to use Monolog 2.

WDYT?

@fabpot

This comment has been minimized.

Member

fabpot commented Jul 5, 2018

@Taluu

This comment has been minimized.

Contributor

Taluu commented Jul 5, 2018

4.2 looks good, if it it released before then (well, even if it's only alpha or beta is fine too though).

@derrabus

This comment has been minimized.

Contributor

derrabus commented Jul 6, 2018

Making the bridge compatible with both versions can get interesting. For some handlers this might mean creating two versions of the class. Also, some handlers don't seem to have tests, unfortunately.

Would it be a good idea to deprecate Monolog-1-only functionality then and have Monolog 2 as a minimal requirement for Monolog Bridge in Symfony 5?

nicolas-grekas added a commit that referenced this issue Jul 9, 2018

minor #27882 [MonologBridge] Prefer PSR-3 to interact with Monolog in…
… tests (derrabus)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[MonologBridge] Prefer PSR-3 to interact with Monolog in tests

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | part of #27857
| License       | MIT
| Doc PR        | N/A

This PR changes tests in the Monolog bridge:
* Calls to `Logger::addInfo()` are changed to `Logger::info()`.
* The return value of `Logger::debug()`, `Logger::info()`, `Logger::notice()`, `Logger::warning()`, `Logger::error()` etc. is no longer asserted to be `true`.

This way, the tests only use PSR-3 compatible code to interact with the logger, which makes them forward-compatible with the changes in Monolog 2.

Commits
-------

78498d3 Prefer PSR-3 to interact with Monolog in tests.
@stof

This comment has been minimized.

Member

stof commented Jul 9, 2018

@derrabus AFAIK, there is no Monolog-1-only functionality. BC breaks are about requiring type hints in some handler APIs (and so forcing handlers to have them).

@nicolas-grekas based on my discussions with @Seldaek, a PHP 7.1+ codebase should be able support Monolog 1.x and 2.x with the same code (as PHP allows adding return types in child classes). If that's indeed true, I suggest doing this work in 4.2 (doing it in 3.4 would require shipping 2 implementations with a version check as the Monolog 2.x handler cannot work on PHP 7.0 or PHP 5.x due to the return types being used)

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jul 9, 2018

@stof 💯 PR welcome to start that work, maybe @derrabus you'd like to give it a try?

@derrabus

This comment has been minimized.

Contributor

derrabus commented Jul 9, 2018

PHP allows adding return types in child classes

PHP allows to add return types in child classes, but it doesn't allow to remove them. So for instance, if we add a bool return type to the isHandling() method of our handlers, we would still be compatible with the interface of Monolog 1.x, but we would break any application code that extends one of our handlers and overrides the isHandling method. That's a BC break.

Then there are cases like SwiftMailerHandler. Here we override the send() method that now has a scalar type-hint on its first parameter. That's a problem:

  • If the parent class does not have a parameter type-hint, we cannot add one: https://3v4l.org/HGqJS
  • If the parent class has a parameter type-hint, we cannot remove it (as long as we need to support php 7.1): https://3v4l.org/Ked4d

In this case, we won't be able to create a version of the class that is compatible with Monolog 1.x and 2.x.

maybe @derrabus you'd like to give it a try

Sure!

@derrabus

This comment has been minimized.

Contributor

derrabus commented Jul 9, 2018

I have submitted a first shot as #27905.

@stof

This comment has been minimized.

Member

stof commented Jul 9, 2018

but we would break any application code that extends one of our handlers and overrides the isHandling method. That's a BC break.

for that case, it would be true forever. So it would force to deprecate existing handlers, replacing by similar ones using the typehints

Then there are cases like SwiftMailerHandler. Here we override the send() method that now has a scalar type-hint on its first parameter.

this one is indeed an issue. As this extended SwiftMailerHandler is about supporting the memory spool, which is part of SwiftMailer itself, not of Symfony, I suggest upstreaming it instead (the 2 event listener methods could be replaced by a method allowing to mutate $this->instantFlush, which could then be called by a Symfony listener)

@derrabus

This comment has been minimized.

Contributor

derrabus commented Jul 9, 2018

So it would force to deprecate existing handlers, replacing by similar ones using the typehints

That would work, but I'm having trouble finding a proper naming for the replacements. For instance I would deprecate Symfony\Bridge\Monolog\Handler\ConsoleHandler and create a new one named…?

  • Symfony\Bridge\Monolog\Handler\Monolog2ConsoleHandler ➡️ That's ugly.
  • Symfony\Bridge\Monolog\Handler\TypeHintedConsoleHandler ➡️ Too technical.
  • Symfony\Bridge\Monolog\Monolog2Handler\ConsoleHandler ➡️ Feels redundant.
  • Symfony\Bridge\Monolog2\Handler\ConsoleHandler ➡️ That would mean creating a new package. After all, we don't want to deprecate the whole bridge, do we?

We could also think about applying the pattern that the PHPUnit bridge uses for this purpose, with a ConsoleHandlerForV1 and a ConsoleHandlerForV2 and ConsoleHandler would then alias one of them depending on the actual Monolog version. But that would make the code a lot less navigatable, I guess.

@stof

This comment has been minimized.

Member

stof commented Jul 9, 2018

would then alias one of them depending on the actual Monolog version. But that would make the code a lot less navigatable, I guess.

and that would still be a BC break, as the ConsoleHandler would still be the one extended by child classes (and so could require having the typehint when Monolog 2 was used)

@derrabus

This comment has been minimized.

Contributor

derrabus commented Jul 9, 2018

and that would still be a BC break

Only if the application itself upgrades from Monolog 1 to Monolog 2. But that's the point where a BC break would be expected.

@codedmonkey

This comment has been minimized.

Contributor

codedmonkey commented Jul 25, 2018

IMO if creating a new interface is the only option it should probably be Symfony\Bridge\Monolog\Handler\MonologConsoleHandler.

In theory we could deprecate it again in 5.0 and move development back to ConsoleHandler

@stof

This comment has been minimized.

Member

stof commented Jul 25, 2018

Only if the application itself upgrades from Monolog 1 to Monolog 2. But that's the point where a BC break would be expected.

But I'm quite sure most apps have a dependency on MonologBundle and no direct dependency on Monolog itself (we could argue that they should have it once they start writing their own handlers depending on the library extension point, but that is likely not always happening).

IMO if creating a new interface is the only option it should probably be Symfony\Bridge\Monolog\Handler\MonologConsoleHandler.

In theory we could deprecate it again in 5.0 and move development back to ConsoleHandler

We simply cannot keep BC for people extending our handlers if we want to start supporting Monolog 2 before Symfony 5.0. If we create a new class with the proper return types, code extending the existing class without the return types would still break if Monolog 2 gets installed (as our own code would be incompatible with it). The necessary plan to support such case would be:

  • add checks in Symfony 4.2 triggering a deprecation notice if they extend the class without adding the return types (they can already do it, unless they themselves need to account for child classes, but then it forces them to do the same process than us earlier than us)
  • in Symfony 5.0, add the return types on our own classes. At that point, we become compatible with Monolog 2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment