Monolog 2.x compatabiliy #24
Conversation
New Requirement: PHP 7.2+
I'll leave that to the repo owner, if / when he decides to tag a release.
|
The above build errors seem to be from mostly upstream errors (5.4 and 5.5 binaries don't exist and 404) the 5.6 fails because we require php 7.2 now, I don't know if telling travis to run on 7.2 is something that can be done via a PR or not.. |
|
I'm probably going to run php 7.3 in the CI stuff, since 7.2 is actually unsupported. https://www.php.net/supported-versions.php I'd feel silly going through all this trouble to update only for it to be updated to a non-supported php version. Thoughts? |
|
@koraktor how do you feel about upgrading the tests / some other changes? PHPUnit removed the assertAttribute method a while back, I started upgrading the tests, but haven't poked at those changes with the assertAttribute changes yet, as I didn't want to go modifying the source files to add accessors before you said so https://github.com/zack6849/steam-condenser-php/tree/travis-php7 I've gotten it more or less even with master, build status wise, aside from the risky tests because of no assertions (i just commented out the assertAttribute lines to be filled in later) |
| }, | ||
| "require-dev": { | ||
| "phpdocumentor/phpdocumentor": "~2.6.1", |
There was a problem hiding this comment.
This is unrelated and should go into a separate PR.
There was a problem hiding this comment.
My apologies, that was removed oin my fork and I forgot to re-add that change.
There was a problem hiding this comment.
So, i'm happy to make another PR for phpdoc, but what are we even using it for?
I don't see any phpdocs live anywhere, there's no docs written to the repo, what are we doing with it?
I can't install it via composer because it requires monolog 1.6, but reverting all these changes to satisfy a dev dependency just for the convenience of installing it via composer seems silly.
Thoughts?
There was a problem hiding this comment.
I think updating to 3.0.0-rc should be fine.
There was a problem hiding this comment.
I actually did do that, though not via composer, let me see if that's a viable option, thanks.
There was a problem hiding this comment.
I was able to achieve this, but only by setting the minimum stability to dev and telling it to prefer stable packages.
See here: https://github.com/zack6849/steam-condenser-php/tree/travis-php7
I tested it in my branch to bring all the testing code up to 7.x, I can make it a separate PR, but let's tackle that after we figure out this whole monolog thing, too many fixes in the air right now IMO
There was a problem hiding this comment.
I‘m totally fine with having these changes in a single „update dependencies“ PR.
Looks good so far.
lib/SteamCondenser.php
Outdated
| return new Logger($name); | ||
| $logger = new Logger($name); | ||
| $logger->pushHandler(new StreamHandler("php://stdout")); | ||
| return $logger; |
There was a problem hiding this comment.
Not adding a handler to the Monolog logger was a deliberate decision. That way nobody needs to know how to disable logging. Which is requested more often than vice-versa.
There was a problem hiding this comment.
I'm a tad bit confused on this, then.
Unit tests with the master branch printed log lines, and before I pushed an STDOUT handler, the tests didn't.
How is the log handling being accomplished?
There was a problem hiding this comment.
Also, is @mavprolucky you or something? I'm very confused about these comments being left on this PR.
There was a problem hiding this comment.
Maybe this is new behavior in Monolog 2?
Mavprolucky is not me, just reported as spam.
There was a problem hiding this comment.
Gotcha, thanks for clarifying, disregard my email then :)
So, what is the desired behavior here? I'm not super sure how to proceed with this.
There was a problem hiding this comment.
The desired behavior would be „let the user decide and don’t spam whatever logging method by default“.
There was a problem hiding this comment.
Well, that's doable, do we want to just expose the logger via a getter, and leave it to the user to push whatever handlers they like to it if they want output?
There was a problem hiding this comment.
About 2.0 behavior: Seldaek/monolog@ad37b7b
Don't push any handlers to the logger, leave it to the user.
|
@koraktor i've removed the handler, this has been sitting for a while, is there anything else needed testing wise for this to get merged? |
New Requirement: PHP 7.2+
This should fix koraktor/steam-condenser#329
Testing output seemed to be identical to the current branch
One caveat, the default handler right now only prints to STDOUT, i'm not entirely sure if that was the configuration on the original, but it's what it seemed to be, if you want to log to STDERR, or a log file, or something else, just let me know, or of course feel free to edit the PR
Thanks!