[FEATURE] Add support for PSR/Log

[ziegenberg]

• Add PSR/Log as dependency
• Add a new SimpleLogger
• Use the NullLogger per default

Helps with #461

[ziegenberg]

@oliverklee would this be the direction you thought of?

[JakeQZ]

So I am thinking that the logging should be taking place where the exceptions are handled (if the conditional bLenientParsing comes into play), rather than where they are thrown. Though in cases where !bLenientParsing causes an exception to be thrown, logging should take place there (and we can later remove the throw as part of #462).

[ziegenberg]

@oliverklee Any chance I could a quick 👍🏻 or 👎🏻 for this? Yes, this is a work in progress, but before I invest more time, I'd like to know if this is going in the right direction or if I'm completely off track.

[oliverklee]

@ziegenberg I regret that you didn't get feedback from us on the general direction of this PR. I'm sorry for this.

I personally am currently in a conflict in another open source project, which impacts my mental wellbeing, and I try to stick to the smaller/easier reviews during this time.

I'll do my best to write something on the architecture and direction of this PR in a few days.

Until then, please bear with me/us.

[JakeQZ]

In the meantime, I've had another brief look.

I think the use of LoggerAwareInferface, LoggerAwareTriat, etc., looks perfect: exact;ly what we want. (I just noticed you had a bunch of static methods to deal with too. I like the solution there too.)

My main concern is the duplication where exceptions are thrown and something identical is logged. Can the logging be done downstream, using the data from the exception where it is caught? This would have to be reviewed on a case-by-case basis; I appreceiate, @ziegenberg, that you probably don't have 100% familiarity with the code base (neither do I, FWIW - you're probaby now as familiar with it as me; we really appreciate your top-notch contributions so far; besides the actual improvements, your involvment has, I think, helped motivate the rest of us to move this project forwards; much kudos).

HTH <3

@oliverklee, hope you resolved the disupute/conflict (hope it wasn't with me - if it was, I was unaware :) )

[JakeQZ]

Can the logging be done downstream, using the data from the exception where it is caught? This would have to be reviewed on a case-by-case basis.

Sorry, I haven't had time to review the specific cases, though I suspect in most it can be (logged where the exception is caught).

[JakeQZ]

Can wie have the introduction of the logger interfaces in solo, without any actual logging, as a separate initial pre-PR?

I feel there should be some unit testing for that, but am out of ideas right now.

[JakeQZ]

I ... am out of ideas right now.

implementsLogger might be one such test.

[oliverklee]

hope you resolved the disupute/conflict (hope it wasn't with me - if it was, I was unaware :) )

@JakeQZ Well, it's mostly over (though not resolved). And no, it's not with you - if there was something to resolve with you, I would do my best to let you know so we can work together to resolve it. :heart:

[oliverklee]

I regret that it took me so long to give some general feedback on this. So here we go. :-)

All classes should use the same logger instance when a document is parsed. This will allow users to get all logged warnings/errors in one place. For this, we need a class that gets the original logger instance and then passes it on to the other classes. Maybe the parser?

Also, the instance should be provided when the corresponding classes are instantiated so the desired logger will be always available. (I think for the parser, it's fine to start with the NullLogger by default and allow setting a different logger later.)

Also, maybe it would help if we split this up into smaller PRs to get this reviewed and merged faster:

1. introduce the NullLogger and provide it to the classes
2. add the non-null logger
3. actually log stuff

We also need to either allow the logger getting set via dependency injection (particularly, Symfony DI), or at least paint us into a corner with our API DI-wise.

[JakeQZ]

All classes should use the same logger instance when a document is parsed ... For this, we need a class that gets the original logger instance and then passes it on to the other classes. Maybe the parser?

I think the Settings class can handle this. The logger instance can be added as a property of that.

Also, the instance should be provided when the corresponding classes are instantiated so the desired logger will be always available.

An instance of ParserState is passed around everywhere during parsing. This has a getSettings method.

(I think for the parser, it's fine to start with the NullLogger by default and allow setting a different logger later.)

If the logger instance is a property of Settings, it can be switched midway through parsing, or later, if the various classes are referencing the Settings (if they need to) rather than the logger directly. (If that's what you meant.)

[JakeQZ]

2. add the non-null logger

Should we be providing a non-null logger in the release package? Shouldn't users be providing a logger of their choice (from a library)? For development/testing, is there some kind of mock/testing logger available that plays well with PHPUnit?

[oliverklee]

Should we be providing a non-null logger in the release package? Shouldn't users be providing a logger of their choice (from a library)? For development/testing, is there some kind of mock/testing logger available that plays well with PHPUnit?

I think packaging the NullLogger should be enough. We can use our non-null logger in the tests (and move it the the testing namespace/folder, e.g., some Fixtures folder). If someone wants to use real logging in their app/library, they'll have they own PSR-compatible logger.

[oliverklee]

I think the Settings class can handle this. The logger instance can be added as a property of that. … An instance of ParserState is passed around everywhere during parsing. This has a getSettings method.

Having the logger attached to the Settings class doesn't feel right to me. Logging is not part of configuration. Having it attached to the ParserState sounds okay to me.

[JakeQZ]

I think packaging the NullLogger should be enough. We can use our non-null logger in the tests (and move it the the testing namespace/folder, e.g., some Fixtures folder).

Agree.

Having the logger attached to the Settings class doesn't feel right to me. Logging is not part of configuration. Having it attached to the ParserState sounds okay to me.

This is debatable (in the literal sense, asking a philosophical question). I'm happy either way.

I the practical sense, I don't think it makes much difference, as ParserState is always available during parsing, and we are primarily, for now, only interested in logging parsing issues. However, the library also provides for manipulation of the CSS objects after parsing (though, offhand, I'm not sure a Settings object is any more readily-available then); this is worth bearing in mind but I don't think anything of particular concern for the time being.

[JakeQZ]

@sabberworm, does what we're saying seem reasonable approach-wise? (Not asking for specific code changes review at this stage, but if you spot a flaw in the plan, that would be good to know.)