2.0 Code cleanups / tasks #197

Open
Seldaek opened this Issue May 21, 2013 · 19 comments

Projects

None yet

10 participants

@Seldaek
Owner
Seldaek commented May 21, 2013 edited
  • 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
  • Define which php version should be supported (=> 7.0)
  • 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 #356 (comment)
  • 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. f3b61cf#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 01549ec 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

@stof
Contributor
stof commented 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.

@Seldaek
Owner
Seldaek commented May 21, 2013

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

@rpkamp
Contributor
rpkamp commented 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.

@Seldaek
Owner
Seldaek commented 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.

@DennisBecker

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.

@Seldaek
Owner
Seldaek commented 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.

@DennisBecker

@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.

@adlawson
Contributor

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.

@Seldaek
Owner
Seldaek commented 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.

@rpkamp
Contributor
rpkamp commented 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.

@Seldaek Seldaek changed the title from 2.0 Code cleanups to 2.0 Code cleanups / tasks Apr 20, 2014
@staabm
Contributor
staabm commented 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..

@Seldaek
Owner
Seldaek commented 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.

@nackjicholson

@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

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.

@ChristianRiesen

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.

@ZergMark
Contributor
ZergMark commented 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
@rpkamp
Contributor
rpkamp commented 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.

@stof
Contributor
stof commented Dec 2, 2016

@ZergMark return types are used in several places already.

@moderndeveloperllc
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment