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.

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

[FEATURE] Add support for PSR/Log #596

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ziegenberg
Copy link
Contributor

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

Helps with #461

@ziegenberg
Copy link
Contributor Author

@oliverklee would this be the direction you thought of?

@ziegenberg ziegenberg marked this pull request as ready for review June 20, 2024 22:53
Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

The motivation for adding this was to allow removal of the lenient/strict mode parsing switch. See #463.

All CSS should be parsed as best as possible, with invalid constructs dropped and logged. This is what browsers do.

The strict mode setting allowed parsing exceptions to propagate such that parsing failed, or caused exceptions to be thrown. Instead, we want them to be logged.

Searching for bLenientParsing will indicate where the behaviour deviates. I think initially we would want logging in only those situations, where strict parsing would cause an error, with a view to replacing the strict parsing option with logging.

I haven't looked through these changes in detail, but am aware some exceptions are intentional and should not be logged.

@JakeQZ
Copy link
Contributor

JakeQZ commented Jun 21, 2024

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 ziegenberg force-pushed the add-psr-logger branch 2 times, most recently from 41b62e5 to 256d09b Compare June 24, 2024 12:52
- Add PSR/Log as dependency
- Add a new SimpleLogger
- Use the NullLogger per default

Helps with MyIntervals#461

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
@ziegenberg
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Contributor

JakeQZ commented Jun 27, 2024

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 :) )

Comment on lines +1 to +34
<?php

namespace Sabberworm\CSS;

use Psr\Log\AbstractLogger;

/**
* This class provides a simple logging facility.
*/
class SimpleLogger extends AbstractLogger
{
public function log($level, $message, array $context = []): void
{
echo self::interpolate($message, $context) . PHP_EOL;
}

/**
* Interpolates context values into the message placeholders.
*/
private function interpolate(string $message, array $context = []): string
{
// build a replacement array with braces around the context keys
$replace = [];
foreach ($context as $key => $val) {
// check that the value can be cast to string
if (!is_array($val) && (!is_object($val) || method_exists($val, '__toString'))) {
$replace['{' . $key . '}'] = $val;
}
}

// interpolate replacement values into the message and return
return strtr($message, $replace);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to provide this? Shouldn't it be up to the users of the package to provide their own logger, or one from a library, should they want logging?

@JakeQZ
Copy link
Contributor

JakeQZ commented Jun 27, 2024

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
Copy link
Contributor

JakeQZ commented Jun 27, 2024

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
Copy link
Contributor

JakeQZ commented Jun 27, 2024

I ... am out of ideas right now.

implementsLogger might be one such test.

@oliverklee
Copy link
Contributor

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. ❤️

@@ -24,7 +24,8 @@
],
"require": {
"php": ">=7.2.0",
"ext-iconv": "*"
"ext-iconv": "*",
"psr/log": "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

For dependencies that are actual packages (instead of PHP extensions), please always use specific versions:

Suggested change
"psr/log": "*"
"psr/log": "^1.0.0 || ^2.0.0 || ^3.0.0"

@oliverklee
Copy link
Contributor

oliverklee commented Jun 29, 2024

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
Copy link
Contributor

JakeQZ commented Jun 29, 2024

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
Copy link
Contributor

JakeQZ commented Jun 29, 2024

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
Copy link
Contributor

oliverklee commented Jun 30, 2024

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
Copy link
Contributor

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
Copy link
Contributor

JakeQZ commented Jun 30, 2024

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 JakeQZ requested a review from sabberworm July 1, 2024 00:47
@JakeQZ
Copy link
Contributor

JakeQZ commented Jul 1, 2024

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

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

Successfully merging this pull request may close these issues.

3 participants