Skip to content
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.

Sign up for GitHub

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

Monolog 2.x compatabiliy #24

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Monolog 2.x compatabiliy #24

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
879d35c
Monolog 2.x availability.
zack6849 May 4, 2020
c0ba40c
It's actually probably not appropriate for this PR to bump the version
zack6849 May 4, 2020
bd46edf
Don't push any handlers by default
zack6849 Jan 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,10 @@
"wiki": "https://github.com/koraktor/steam-condenser/wiki"
},
"require": {
"php": ">=5.4.0",
"monolog/monolog": "~1.10"
"php": "^7.2",
"monolog/monolog": "^2.0"
},
"require-dev": {
"phpdocumentor/phpdocumentor": "~2.6.1",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated and should go into a separate PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies, that was removed oin my fork and I forgot to re-add that change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think updating to 3.0.0-rc should be fine.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually did do that, though not via composer, let me see if that's a viable option, thanks.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I‘m totally fine with having these changes in a single „update dependencies“ PR.

Looks good so far.

"phpunit/phpunit": "~4.1.3"
},
"autoload": {
Expand Down
7 changes: 5 additions & 2 deletions lib/SteamCondenser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@

namespace SteamCondenser;

use Monolog\Handler\StreamHandler;
use Monolog\Logger;

const VERSION = '1.3.9';
const VERSION = '1.3.10';

/**
* Returns a Monolog logger with the given name
Expand All @@ -23,5 +24,7 @@
* @return Logger The requested Monolog logger
*/
function getLogger($name) {
return new Logger($name);
$logger = new Logger($name);
$logger->pushHandler(new StreamHandler("php://stdout"));
return $logger;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is @mavprolucky you or something? I'm very confused about these comments being left on this PR.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is new behavior in Monolog 2?

Mavprolucky is not me, just reported as spam.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The desired behavior would be „let the user decide and don’t spam whatever logging method by default“.

Copy link
Author

@zack6849 zack6849 May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About 2.0 behavior: Seldaek/monolog@ad37b7b

}