itsgoingd / clockwork

Clockwork - php dev tools in your browser - server-side component
https://underground.works/clockwork
MIT License
5.7k stars 320 forks source link

Monolog handler doesn't work in Monolog 2 #566

Closed mahagr closed 2 years ago

mahagr commented 2 years ago

Declaration of Clockwork\Support\Monolog\Handler\ClockworkHandler::write(array $record) must be compatible with Monolog\Handler\AbstractProcessingHandler::write(array $record): void

So there are two options: either create another class for Monolog 2 or use \Monolog\Logger::API constant to choose between two versions of the class.

mahagr commented 2 years ago

I can make a pull request and I'm in the favor of the second option as it requires no work in the application. I just need to know which approach do you prefer @itsgoingd

itsgoingd commented 2 years ago

Hmm, I'm not sure about returning different version of the class based on the constant. Do you know of anyone using similar approach? Seems like something somewhere would break, opcache, preloading or whatever.

We can of course add a separate handler, but now I remember why I've never added it, I couldn't decide on the name. :)

mahagr commented 2 years ago

I do it all the time. There are multiple ways to do it, depending on what you want. Opcache won't break if you name the classes properly instead of attempting to override them through autoloader.

You could for example use the decoration pattern for these methods (write() etc) and either pass the proper monolog handler adapter into the constructor or handle it internally by selecting one based on the constant (or preferably check if the adapter is passed and if not, fall back to detection).

itsgoingd commented 2 years ago

Ah, I thought you were suggesting declaring different version of the class based on that constant at first. :)

Well, for this to not be a breaking change we pretty much can't touch the existing ClockworkHandler as it extends the Monolog abstract class which implements their interfaces.

I guess I will just add a separate handler for Monolog 2 for now, there's already an RFC for Monolog 3, so we might need even more implementations in the future anyway. We can revisit and do something better in the next major.

mahagr commented 2 years ago

A separate handler sounds good to me. It shouldn't even be a large change. Do you need help or do you have time to implement it yourself?

itsgoingd commented 2 years ago

Added in Clockwork 5.1.6.