[GH-ISSUE #197] 2.0 Code cleanups / tasks

kerem commented

2026-03-04 02:11:50 +03:00

Owner

Originally created by @Seldaek on GitHub (May 21, 2013).
Original GitHub issue: https://github.com/Seldaek/monolog/issues/197

Clean up arg orders of handlers that have had added args pushed at the end (after bubbling etc), maybe group them in an $options array or parameter objects? Needs RFC. Check #801 too about fluent setters
add scalar hints and return type hints where relevant ("only" handlers left to go through)
~~figure out how to handle network timeouts globally somehow, see #733~~
Add a close() method on the Logger to close all its handlers? What about resetting FingersCrossedHandler?
Define which php version should be supported (=> 7.1)
Add a timezone arg to Logger instead of the static timezone?
Turn static::getLevelName into static::
Set Monolog\Logger::API = 2 to allow users to adjust
Remove non PSR-3 methods to add record
Remove processor/formatter setters and getters from the HandlerInterface
Remove logstash V0 support or at least make V1 default
Remove legacy GELF handler support, c.f. #326
Remove datetime TODO in loggly handler
Move Monolog\TestCase to src/, see https://github.com/Seldaek/monolog/pull/356#issuecomment-40899938
~~Split handlers w/ dependencies into packages i.e. http://php-and-symfony.matthiasnoback.nl/2014/04/theres-no-such-thing-as-an-optional-dependency/~~
~~Change Logger constants to have RFC values (https://github.com/php-fig/log/issues/8)~~
Remove substr TODO in AmqpHandler
Remove default handler ($this->pushHandler(new StreamHandler('php://stderr', static::DEBUG));) from Monolog\Logger
~~Consider adding a public method (send or write or so) on handlers to make reusing them out of monolog easier, c.f.~~ github.com/Seldaek/monolog@f3b61cf48a (commitcomment-8237858)
Consider using MongoDBFormatter as default in MongoDBHandler
Drop support for outdated mongo exts in MongoDBHandler
Remove BC __get() in SwiftMailerHandler
Update HipChat to API v2 using 01549ec4d0 and room_id and from in buildContent (+ nickname everywhere) should be removed. See if we can't detect the token format and warn users using a v1 token.
Check all PHP_VERSION and version_compare checks for relevance
LineFormatter when showing stack traces should show new lines on new lines, and not the exception as part of the context that gets inlined into json => #752
Consider removing useless PHP docs if the scalar + return type hints give enough info
Disable microseconds timestamps by default?

php7 ideas:

treat Error-class exceptions in a special way as they are replacing E_COMPILE_ERROR and such from defaultErrorLevelMap
~~possibly remove ErrorHandler::registerFatalHandler if this is not needed anymore~~
enable strict mode everywhere?

~~those items have been abandoned~~

Originally created by @Seldaek on GitHub (May 21, 2013). Original GitHub issue: https://github.com/Seldaek/monolog/issues/197 - [x] ~~Clean up arg orders of handlers that have had added args pushed at the end (after bubbling etc), maybe group them in an $options array or parameter objects? Needs RFC. Check #801 too about fluent setters~~ - [x] add scalar hints and return type hints where relevant ("only" handlers left to go through) - [x] ~~figure out how to handle network timeouts globally somehow, see #733~~ - [x] Add a close() method on the Logger to close all its handlers? What about resetting FingersCrossedHandler? - [x] Define which php version should be supported (=> 7.1) - [x] Add a timezone arg to Logger instead of the static timezone? - [x] Turn static::getLevelName into static:: - [x] Set Monolog\Logger::API = 2 to allow users to adjust - [x] Remove non PSR-3 methods to add record - [x] Remove processor/formatter setters and getters from the HandlerInterface - [x] Remove logstash V0 support or at least make V1 default - [x] Remove legacy GELF handler support, c.f. #326 - [x] Remove datetime TODO in loggly handler - [x] Move `Monolog\TestCase` to src/, see https://github.com/Seldaek/monolog/pull/356#issuecomment-40899938 - [x] ~~Split handlers w/ dependencies into packages i.e. http://php-and-symfony.matthiasnoback.nl/2014/04/theres-no-such-thing-as-an-optional-dependency/~~ - [x] ~~Change Logger constants to have RFC values (https://github.com/php-fig/log/issues/8)~~ - [x] Remove substr TODO in AmqpHandler - [x] Remove default handler (`$this->pushHandler(new StreamHandler('php://stderr', static::DEBUG));`) from Monolog\Logger - [x] ~~Consider adding a public method (send or write or so) on handlers to make reusing them out of monolog easier, c.f.~~ https://github.com/Seldaek/monolog/commit/f3b61cf48af2c0730f149ae48f930e12a3bec1f2#commitcomment-8237858 - [x] Consider using MongoDBFormatter as default in MongoDBHandler - [x] Drop support for outdated mongo exts in MongoDBHandler - [x] Remove BC __get() in SwiftMailerHandler - [x] Update HipChat to API v2 using 01549ec4d05e2d3f33227531d9fc9454d5ef6049 and `room_id` and `from` in buildContent (+ `nickname` everywhere) should be removed. See if we can't detect the token format and warn users using a v1 token. - [x] Check all PHP_VERSION and version_compare checks for relevance - [x] LineFormatter when showing stack traces should show new lines on new lines, and not the exception as part of the context that gets inlined into json => #752 - [x] Consider removing useless PHP docs if the scalar + return type hints give enough info - [x] Disable microseconds timestamps by default? php7 ideas: - [x] treat Error-class exceptions in a special way as they are replacing E_COMPILE_ERROR and such from defaultErrorLevelMap - [x] ~~possibly remove ErrorHandler::registerFatalHandler if this is not needed anymore~~ - [x] enable strict mode everywhere? ~~those items have been abandoned~~

kerem closed this issue

2026-03-04 02:11:50 +03:00

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@stof commented on GitHub (May 21, 2013):

you should define Monolog\Logger::API = 1 now if you intend to bump it for 2.0, otherwise adjusting will require checking both if it is defined and equal to 2.

@stof commented on GitHub (May 21, 2013): you should define `Monolog\Logger::API = 1` now if you intend to bump it for 2.0, otherwise adjusting will require checking both if it is defined and equal to 2.

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@Seldaek commented on GitHub (May 21, 2013):

yup for sure, though that won't entirely prevent the need for a check,
but it should help

@Seldaek commented on GitHub (May 21, 2013): yup for sure, though that won't entirely prevent the need for a check, but it should help

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@rpkamp commented on GitHub (Aug 25, 2013):

Since we're talking about breaking BC anyway ...

I've always wondered why in Logger::addRecord you first check ::isHandling for each of the registered handlers to find the first one that will be handling the message, and then iterate through all handlers from that point forward. Wouldn't a more logical implementation be:

$handlingHandlers = [];
foreach ($this->handlers as $key => $handler) {
    if ($handler->isHandling($record)) {
        $handlingHandlers[] = $key;
    }
}

if (0 === count($handlingHandlers) {
    return false;
}

// processing here

foreach ($handlingHandlers as $handlerKey) {
    if (true === $this->handlers[$handlerKey]->handle($record)) {
        break;
    }
}

That way you only check once if a hander will handle a message, and then and only then give it the record. With the current version of the code all handlers need to call isHandling on themselves to see if they are actually handling the message. This to me seems the wrong place to check this.

@rpkamp commented on GitHub (Aug 25, 2013): Since we're talking about breaking BC anyway ... I've always wondered why in `Logger::addRecord` you first check `::isHandling` for each of the registered handlers to find the first one that will be handling the message, and then iterate through all handlers from that point forward. Wouldn't a more logical implementation be: ``` $handlingHandlers = []; foreach ($this->handlers as $key => $handler) { if ($handler->isHandling($record)) { $handlingHandlers[] = $key; } } if (0 === count($handlingHandlers) { return false; } // processing here foreach ($handlingHandlers as $handlerKey) { if (true === $this->handlers[$handlerKey]->handle($record)) { break; } } ``` That way you only check once if a hander will handle a message, and then and only then give it the record. With the current version of the code all handlers need to call `isHandling` on themselves to see if they are actually handling the message. This to me seems the wrong place to check this.

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@Seldaek commented on GitHub (Aug 25, 2013):

There are a few things that influenced the existing code:

we don't want to pre-process records if no handler will handle it (that's the only reason isHandling is called externally at all).
if a given handler has bubbling disabled and handles a record, the chain is broken and we stop looping through them.

Since bubbling is rarely disabled and processors are also not very much used it seems, maybe one thing we could do at least is avoid calling isHandling if no processors are present. That would be BC and might make things a wee bit more efficient.

@Seldaek commented on GitHub (Aug 25, 2013): There are a few things that influenced the existing code: - we don't want to pre-process records if no handler will handle it (that's the only reason isHandling is called externally at all). - if a given handler has bubbling disabled and handles a record, the chain is broken and we stop looping through them. Since bubbling is rarely disabled and processors are also not very much used it seems, maybe one thing we could do at least is avoid calling isHandling if no processors are present. That would be BC and might make things a wee bit more efficient.

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@DennisBecker commented on GitHub (Sep 16, 2013):

I think the "FingersCrossedHandler" has a bad name. I was looking something like "Backtrace" logger and just wanted to start to implement that for myself and then I have found the FindersCrossedHandler" in the docs.

I would appreciate to rename this handler to a more obvious name for those who first look into the source code and file names.

@DennisBecker commented on GitHub (Sep 16, 2013): I think the "FingersCrossedHandler" has a bad name. I was looking something like "Backtrace" logger and just wanted to start to implement that for myself and then I have found the FindersCrossedHandler" in the docs. I would appreciate to rename this handler to a more obvious name for those who first look into the source code and file names.

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@Seldaek commented on GitHub (Sep 16, 2013):

@DennisBecker I feel your pain but frankly I doubt there is a short name that can convey clearly what it does to everyone. It's just a concept and as such Backtrace wouldn't be more clear to me and less fun so I'd say no. It's not strictly a backtrace since it also sends logs following an error for example.

@Seldaek commented on GitHub (Sep 16, 2013): @DennisBecker I feel your pain but frankly I doubt there is a short name that can convey clearly what it does to everyone. It's just a concept and as such Backtrace wouldn't be more clear to me and less fun so I'd say no. It's not strictly a backtrace since it also sends logs following an error for example.

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@DennisBecker commented on GitHub (Sep 16, 2013):

@Seldaek I agree, backtrace doesn't fit better at all. It is more a kind of log message aggregation. That would better describe it from my point of view.

@DennisBecker commented on GitHub (Sep 16, 2013): @Seldaek I agree, backtrace doesn't fit better at all. It is more a kind of log message aggregation. That would better describe it from my point of view.

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@adlawson commented on GitHub (Oct 21, 2013):

One thing that may be worth considering is the potential to limit the handlers contributed to the core library, and instead encourage contributions as a separate library.
The benefit of not including every handler under the sun is a reduced library size, and consumers can depend on the specific handlers they need.
I think, though, that it's a good idea to have a good selection of handlers in the core.

Just something to consider.

@adlawson commented on GitHub (Oct 21, 2013): One thing that may be worth considering is the potential to limit the handlers contributed to the core library, and instead encourage contributions as a separate library. The benefit of not including every handler under the sun is a reduced library size, and consumers can depend on the specific handlers they need. I think, though, that it's a good idea to have a good selection of handlers in the core. Just something to consider.

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@Seldaek commented on GitHub (Oct 21, 2013):

@adlawson I'm generally all for pushing code into plugins, because I don't particularly enjoy having to maintain code I am not using and did not write. That said, in this case since most handlers have very little code it's not so bad on the maintenance side, and having them "blessed" here means higher code quality and consistency between handlers. At least I try to keep things that way, probably it's not 100%, but better than if there was a wild-west of plugin packages to choose from I think. So that's unlikely to change unless the amount of log backends becomes unmanageable.

@Seldaek commented on GitHub (Oct 21, 2013): @adlawson I'm generally all for pushing code into plugins, because I don't particularly enjoy having to maintain code I am not using and did not write. That said, in this case since most handlers have very little code it's not so bad on the maintenance side, and having them "blessed" here means higher code quality and consistency between handlers. At least I try to keep things that way, probably it's not 100%, but better than if there was a wild-west of plugin packages to choose from I think. So that's unlikely to change unless the amount of log backends becomes unmanageable.

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@rpkamp commented on GitHub (Mar 19, 2014):

Maybe a better name for FingersCrossedHandler would be AllOrNothingHandler? It does convey what the handler does a little bit better. Although I personally still think FingersCrossedHandler is the best class name ever and does not need changing I can see that it can be confusing to newcomers.

@rpkamp commented on GitHub (Mar 19, 2014): Maybe a better name for FingersCrossedHandler would be AllOrNothingHandler? It does convey what the handler does a little bit better. Although I personally still think FingersCrossedHandler is the best class name ever and does not need changing I can see that it can be confusing to newcomers.

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@staabm commented on GitHub (Oct 14, 2014):

@Seldaek some LogHandlers have a plethora of parameters which make them really hard to use and read.
What do you think of introducing parameter objects with a builter pattern, to ease the config of handlers?
This would also ease the addition of even more parameters in the future, since you don't need to clutter the constructor anymore..

@staabm commented on GitHub (Oct 14, 2014): @Seldaek some LogHandlers have a plethora of parameters which make them really hard to use and read. What do you think of introducing parameter objects with a builter pattern, to ease the config of handlers? This would also ease the addition of even more parameters in the future, since you don't need to clutter the constructor anymore..

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@Seldaek commented on GitHub (Oct 21, 2014):

@staabm might be enough to have a simple $options array as standard constructor param.. Not as self-documenting as the args but I agree in some cases it's getting a bit absurd.

@Seldaek commented on GitHub (Oct 21, 2014): @staabm might be enough to have a simple $options array as standard constructor param.. Not as self-documenting as the args but I agree in some cases it's getting a bit absurd.

kerem commented

2026-03-04 02:11:51 +03:00

Author

Owner

@nackjicholson commented on GitHub (Oct 29, 2014):

@DennisBecker +1 For "FingersCrossedHandler" having a bad name. I wasted a couple hours building and testing my own handler which had the same functionality. I don't like my name for it either, but I named mine "TailingBufferHandler" because what I wanted to see from it was kind of like a linux tail of contextual past logs from the point when the log of an appropriate severity was handled.

"FingersCrossed" sounds like a goofy ass database name, or the name of some quirky log service you've never heard of -- and that makes it extremely easy to overlook. Which is a shame, because it's awesome! Quite possibly the most useful handler.

@nackjicholson commented on GitHub (Oct 29, 2014): @DennisBecker +1 For "FingersCrossedHandler" having a bad name. I wasted a couple hours building and testing my own handler which had the same functionality. I don't like my name for it either, but I named mine "TailingBufferHandler" because what I wanted to see from it was kind of like a linux `tail` of contextual past logs from the point when the log of an appropriate severity was handled. "FingersCrossed" sounds like a goofy ass database name, or the name of some quirky log service you've never heard of -- and that makes it extremely easy to overlook. Which is a shame, because it's awesome! Quite possibly the most useful handler.

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@nackjicholson commented on GitHub (Oct 29, 2014):

Do you have a branch where you're targeting these changes to? I'd be willing to lend a hand on some of the low complexity ones like removing todos, and moving the TestCase to the src or a tests-src/ directory.

@nackjicholson commented on GitHub (Oct 29, 2014): Do you have a branch where you're targeting these changes to? I'd be willing to lend a hand on some of the low complexity ones like removing todos, and moving the `TestCase` to the `src` or a `tests-src/` directory.

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@ChristianRiesen commented on GitHub (Jan 7, 2016):

Just a small side note on the naming FingersCrossed: The concept is close to tripping a fuse, a common pattern in flood avoidance code. BufferedFuse? FuseTripping? I'm not saying these are good, but maybe will inspire someone for a better one. Good naming will save a lot of time for many I think, just judging from how many already said here that they essentially started implementing it on their own as well.

@ChristianRiesen commented on GitHub (Jan 7, 2016): Just a small side note on the naming FingersCrossed: The concept is close to tripping a fuse, a common pattern in flood avoidance code. BufferedFuse? FuseTripping? I'm not saying these are good, but maybe will inspire someone for a better one. Good naming will save a lot of time for many I think, just judging from how many already said here that they essentially started implementing it on their own as well.

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@ZergMark commented on GitHub (Mar 3, 2016):

"Drop support for outdated mongo exts in MongoDBHandler" can be marked as done via #307.

Very Minor PHP 7 Ideas:

Return type declarations
Group use declarations

@ZergMark commented on GitHub (Mar 3, 2016): "Drop support for outdated mongo exts in MongoDBHandler" can be marked as done via #307. Very Minor PHP 7 Ideas: 1. Return type declarations 2. Group use declarations

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@rpkamp commented on GitHub (Dec 2, 2016):

Why was "Split handlers w/ dependencies into packages" crossed out? It seems like a good idea to me. At the moment if you want to develop for monolog and run all the tests to make sure you haven't broken anything you need to install a lot of dependencies, one of which I don't even have access to at all (Zend Server). If parts that need additional dependencies were to be split off of the main repository it would make developing a lot easier imo.

@rpkamp commented on GitHub (Dec 2, 2016): Why was "Split handlers w/ dependencies into packages" crossed out? It seems like a good idea to me. At the moment if you want to develop for monolog and run all the tests to make sure you haven't broken anything you need to install a _lot_ of dependencies, one of which I don't even have access to at all (Zend Server). If parts that need additional dependencies were to be split off of the main repository it would make developing a lot easier imo.

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@stof commented on GitHub (Dec 2, 2016):

@ZergMark return types are used in several places already.

@stof commented on GitHub (Dec 2, 2016): @ZergMark return types are used in several places already.

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@moderndeveloperllc commented on GitHub (Dec 2, 2016):

@stof In my defense, there were quite a bit fewer uses back in March (@ZergMark is an older business account). :-)

@moderndeveloperllc commented on GitHub (Dec 2, 2016): @stof In my defense, there were quite a bit fewer uses back in March (@ZergMark is an older business account). :-)

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@elazar commented on GitHub (May 23, 2017):

Potentially related to this and #903: a number of handlers are using the curl extension directly in conjunction with Monolog\Handler\Curl\Util for interacting with third-party services:

Loggly
IFTTT
Slackbot
SlackWebhook
Mandrill
Cube
SendGrid

Support for environments without the curl extension could be possible by refactoring these handlers to use Guzzle instead.

@elazar commented on GitHub (May 23, 2017): Potentially related to this and #903: a number of handlers are using the curl extension directly in conjunction with `Monolog\Handler\Curl\Util` for interacting with third-party services: * Loggly * IFTTT * Slackbot * SlackWebhook * Mandrill * Cube * SendGrid Support for environments without the curl extension could be possible by refactoring these handlers to use Guzzle instead.

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@rpkamp commented on GitHub (May 23, 2017):

Instead of going for Guzzle I would consider HTTPlug instead so people can provide their own implementation.

@rpkamp commented on GitHub (May 23, 2017): Instead of going for Guzzle I would consider [HTTPlug][httplug] instead so people can provide their own implementation. [httplug]: https://github.com/php-http/httplug

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@hkdobrev commented on GitHub (Mar 10, 2018):

I'd like to chime in on the old topic of FingersCrossedHandler. Sentry has a concept exactly for that and they call it "breadcrumbs" - what notable events you went through prior to the exception. I'd suggest BreadcrumbsHandler as I think most people would get it right away and it fits the prior art of Sentry.

@hkdobrev commented on GitHub (Mar 10, 2018): I'd like to chime in on the old topic of `FingersCrossedHandler`. Sentry has a concept exactly for that and they call it "breadcrumbs" - what notable events you went through prior to the exception. I'd suggest `BreadcrumbsHandler` as I think most people would get it right away and it fits the prior art of Sentry.

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@filips123 commented on GitHub (Jun 19, 2018):

What will be different between Monolog 1 and 2? Will existing code still work?

@filips123 commented on GitHub (Jun 19, 2018): What will be different between Monolog 1 and 2? Will existing code still work?

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@staabm commented on GitHub (Jun 20, 2018):

The point of a new major version is mostly that it will break „some“ things, hopefully not too much

@staabm commented on GitHub (Jun 20, 2018): The point of a new major version is mostly that it will break „some“ things, hopefully not too much

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@Seldaek commented on GitHub (Jun 20, 2018):

Yeah the goal is to keep things working for end users apart from the configuration of the logger (which is mostly abstracted in other places/frameworks anyway). You can see https://github.com/Seldaek/monolog/blob/master/UPGRADE.md for the current listed BC breaks, the first two kinda affect regular usage.

@Seldaek commented on GitHub (Jun 20, 2018): Yeah the goal is to keep things working for end users apart from the configuration of the logger (which is mostly abstracted in other places/frameworks anyway). You can see https://github.com/Seldaek/monolog/blob/master/UPGRADE.md for the current listed BC breaks, the first two kinda affect regular usage.

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@filips123 commented on GitHub (Jun 20, 2018):

What will be minimal PHP version? Bacause some systems like Ubuntu 14.04 and 16.04 don't have latest PHP version (7.2) in repositories. Also, many hosting are still using PHP 5.5.

@filips123 commented on GitHub (Jun 20, 2018): What will be minimal PHP version? Bacause some systems like Ubuntu 14.04 and 16.04 don't have latest PHP version (7.2) in repositories. Also, many hosting are still using PHP 5.5.

kerem commented

2026-03-04 02:11:52 +03:00

Author

Owner

@stof commented on GitHub (Jun 20, 2018):

@filips123 For now, it requires PHP 7.1+

@stof commented on GitHub (Jun 20, 2018): @filips123 For now, it requires PHP 7.1+

kerem commented

2026-03-04 02:11:53 +03:00

Author

Owner

@stof commented on GitHub (Jun 20, 2018):

and if you still need to support PHP 5.5, the solution is simple: keep using Monolog 1.x. It won't magically stop working the day 2.0 is released.

@stof commented on GitHub (Jun 20, 2018): and if you still need to support PHP 5.5, the solution is simple: keep using Monolog 1.x. It won't magically stop working the day 2.0 is released.

Rows
Columns

[GH-ISSUE #197] 2.0 Code cleanups / tasks #64