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

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 📌 [Policy] Dependency evaluation critera Active , but since this particular package already has it, this issue isn't blocked on that discussion.

Updated the merge request; back to review.

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.

Dictionary.

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

Opened 📌 Adopt TestLogger for testing logging Closed: won't fix .

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 ✨ Allow kernel tests to fail or expect logged errors Needs work (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, I think this is ready to go today.

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.

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

Bumped the requirement and updated.

Since no other changes took place, and the requisite reviews have been completed, I'm gonna restore RTBC here on the assumption that tests will pass.

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.

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.

It shouldn't be backported. This is being added to support the Package Manager module going into core as alpha experimental, which is targeted (hopefully) for 10.1.x.

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?

Automatically closed - issue fixed for 2 weeks with no activity.

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 📌 Replace usages of colinodell/testlogger with BufferingLogger from symfony/error-handler, then remove dev dependency on testlogger Closed: won't fix to discuss removal