Add colinodell/psr-test-logger to core's dev dependencies

We still need to add a dependency evaluation. I suggest using @xjm's template from #2843147-117: Add JSON:API to core as a stable module, and @bnjmnm's stellar example of a dependency evaluation at #2550717: [JS] Replace jQuery.cookie with JS-cookie and provide a BC layer.

It seems like all dependencies are eventually collected at https://www.drupal.org/about/core/policies/core-dependency-policies/depe... — so that's got a lot of examples :)

Big +1 to this. @joachim and I had actually rolled our solution very similar to this in #2903456: Allow kernel tests to fail or expect logged errors, which I'm now postponing on this.

Dependency evaluation

Repository: https://github.com/colinodell/psr-testlogger

This is a very simple package. Fundamentally it is single class:
https://github.com/colinodell/psr-testlogger/blob/main/src/TestLogger.php

Code quality

The project has unit tests and automated coding standards.

Maintainership of the package

Maintained by a single maintainer Colin O'Dell. He is an experienced php open source maintainer and a member of thephpleague leadership team. See https://github.com/colinodell. This package should required very little maintenance.

Security policies of the package

This is a dev dependency, should only be used in test code.

Expected release and support cycles

None stated. Likely would need to adapt only to new php versions.

Other dependencies it'd add, if any

None.

Per #6.

@jonathanshaw So I've got some bad news actually … turns out that Drupal's LoggerChannel does this:

  /**
   * Map of PSR3 log constants to RFC 5424 log constants.
   *
   * @var array
   */
  protected $levelTranslation = [
    LogLevel::EMERGENCY => RfcLogLevel::EMERGENCY,
    LogLevel::ALERT => RfcLogLevel::ALERT,
    LogLevel::CRITICAL => RfcLogLevel::CRITICAL,
    LogLevel::ERROR => RfcLogLevel::ERROR,
    LogLevel::WARNING => RfcLogLevel::WARNING,
    LogLevel::NOTICE => RfcLogLevel::NOTICE,
    LogLevel::INFO => RfcLogLevel::INFO,
    LogLevel::DEBUG => RfcLogLevel::DEBUG,
  ];

…

    if (is_string($level)) {
      // Convert to integer equivalent for consistency with RFC 5424.
      $level = $this->levelTranslation[$level];
    }
    // Call all available loggers.
    foreach ($this->sortLoggers() as $logger) {
      $logger->log($level, $message, $context);
    }

… which means it actually does not log using string levels, but int levels. Which is why in https://www.drupal.org/project/automatic_updates, we've had to resort to code like:

$this->assertTrue($logger->hasRecordThatContains('[name] => AAA 8.x-5.x', (string) RfcLogLevel::ERROR));

instead of

$this->assertTrue($logger->hasRecordThatContains('[name] => AAA 8.x-5.x', LogLevel::ERROR));

Which explains why we cannot use any of the colinodell/psr-test-logger methods that look like this:

$this->assertTrue($logger->hasErrorThatContains('[name] => AAA 8.x-5.x'));

And this has been true also when we were using \Psr\Log\Test\TestLogger.

Note that Drupal's LoggerChannel is compliant, because PSR's logging infrastructure dictates:

    /**
     * Logs with an arbitrary level.
     *
     * @param mixed   $level
     * @param string|\Stringable $message
     * @param mixed[] $context
     *
     * @return void
     *
     * @throws \Psr\Log\InvalidArgumentException
     */
    public function log($level, string|\Stringable $message, array $context = []): void;

Fortunately it requires only a very simple change to support this mapping, which I proposed as a PR to https://github.com/colinodell/psr-testlogger: https://github.com/colinodell/psr-testlogger/issues/2.

The linked issue to allow integer/mixed log levels is proceeding nicely with an MR submitted a few days ago. https://github.com/colinodell/psr-testlogger/issues/2

Security policies of the package
This is a dev dependency, should only be used in test code.

Even for dev dependencies, we have a strong preference for there to at a minimum be a security policy to report security issues privately, which means including contact information for how to do that. https://github.com/colinodell/psr-testlogger#reporting-security-issues includes that. Given that and the other information in the issue summary and in #7, and my own review of the repo, I'm removing the "Needs framework manager review" tag, but this still needs release manager review as well.

The MR needs updating, so "Needs work" for that.

Even for dev dependencies, we have a strong preference for there to at a minimum be a security policy to report security issues privately

I found out that that's actually up for discussion in #3201547: [Policy] Dependency evaluation critera, but since this particular package already has it, this issue isn't blocked on that discussion.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Added the dependency evaluation to the issue summary, including a link to the security policy https://github.com/colinodell/psr-testlogger#reporting-security-issues

I think we should have a follow-up to see where we can convert existing test coverage to use this, so it becomes a general pattern as opposed to specific to the automatic updates code base.

Given this code was already in core for a while implicitly (probably still is in Drupal 9?) and we're just adding it back explicitly, and the dependency evaluation looks good (now that it's in the issue summary), happy to sign off on this.

Leaving tagged for needs issue summary update because we should link the follow-up once it's created.

In https://github.com/colinodell/psr-testlogger/pull/3 the maintainer is about to merge a PR of mine to mostly fix the issue raised by @wimleers in #9.

The solution the maintainer wants to use will alow us to make calls like
$this->assertTrue($logger->hasRecordThatContains('[name] => AAA 8.x-5.x', LogLevel::ERROR));

but not calls like
$this->assertTrue($logger->hasErrorThatContains('[name] => AAA 8.x-5.x'));

I think that's acceptable. In #2903456: Allow kernel tests to fail or expect logged errors (postponed on this) we explored adding a test trait with enhanced assertion DX anyway so the niceties of the syntax don't seem crucial.

https://github.com/colinodell/psr-testlogger/pull/3 is not yet in but I +1'd it.

Given the tiny size and tight scope of this library, and a trivial future update for it after that upstream issue lands & ships, I think this is ready to go today. That upstream PR is not essential but will make the DX better.

I realized something obvious but simple last night (and that wasn't obvious to me before): we could easily write a single class similar to what https://github.com/colinodell/psr-testlogger/blob/main/src/TestLogger.php does for ourselves, and then that avoids adding one extra dependency… 🤓😅

Curious what core committers think!

The biggest risk from adding third party PHP dev dependencies is having to wait for compatibility with new versions of its own dependencies or PHP.

If we exclude dev dependencies, then those are psr/log itself and PHP for this library.
https://github.com/colinodell/psr-testlogger/blob/main/composer.json

So there is a non-zero risk of that, probably a lot closer to zero than many of our other dependencies though.

For this one I think there is probably not much between the two options.

For this one as @catch says I don't think there's much between the two options. If it turns out that the maintainer abandons this package and we need to modify it for a newer version of PHP or psr/log then we can always fork it and bring it into core; we have precedent for this with doctrine/reflection and symfony-cmf/routing, for example. In the meantime, we might as well benefit from an external maintainer looking after this code instead of forking it now.

https://github.com/colinodell/psr-testlogger/pull/3 landed, and a 1.2 was released. Probably we should update the pinned dependency in the MR to use that?

#26: Great news, and yes, agreed we should bump the pinned version.

Thanks,
-Derek

Heh, naughty-naughty. 😉

But I reviewed the bump commit, and it all looks fine to me. The one thing that caught my eye is this:

            "require": {
-                "php": "^7.4 || ^8.0",
+               "php": "^8.0",

But if this going to stay in 10.1.x branch and not be backported, that's fine, since 10.1.x already requires PHP 8.1.

So yeah, RTBC++.

Thanks,
-Derek

p.s. added a link to #3347306 in the summary under remaining tasks. The summary no longer needs any updates.

Agreed. It's a new dev dependency for new features. Not backportable. That's what I was trying to acknowledge/say. Sorry I wasn't clear.

Committed 1b96488 and pushed to 10.1.x. Thanks!

No CR?

I wonder why we didn't consider using \Symfony\Component\Debug\BufferingLogger instead

Something from the symfony namespace feels nicer than a random github username.

Added #3357046: Replace usages of colinodell/testlogger with BufferingLogger from symfony/error-handler, then remove dev dependency on testlogger to discuss removal