Problem/Motivation

https://github.com/php-fig/log/pull/77 introduced a void return type in v3.0.0. Drupal 10 bumps prs/log from 1.x to 3.x. As a result in D10 there's a fatal in LinkyCheckerMemoryLogger::log()

Proposed resolution

Add the return type. As the LoggerInterface previously didn't have a return type, adding it in our overridden LinkyCheckerMemoryLogger shouldn't be an issue for BC with 1.x of psr/log. I.e. it'll be compatible with D9 and D10.

CommentFileSizeAuthor
#4 3346873-4.patch512 bytesfenstrat
#3 3346873-3.patch0 bytesfenstrat

Issue fork linkychecker-3346873

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

fenstrat created an issue. See original summary.

fenstrat opened merge request !2

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
StatusFileSize
new 3346873-3.patch0 bytes

Not sure what's going on that with test run failure. Will look again later, he's the change in patch format for now.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Active » Needs review
StatusFileSize
new 3346873-4.patch512 bytes

This time with an actual patch!

dpi’s picture

dpi
Location Perth, Australia
commented
Status: Needs review » Needs work

I think we're missing coverage here.

A few issues.

Linkychecker presently supports D8 and up. We should explicitely make 9.5 minimum

Linkychecker has minimum of PHP 7.1, \Stringable is a PHP8.0 feature.

We cant introduce the parameter change as it will break with psr/log 1, of which Drupal 9.5 requires.

9.5 core constraints only allow psr/log ^1.1, so we cannot force the psr/log 3 change

I think we have to make a release split between D9 + D10.

dpi’s picture

dpi
Location Perth, Australia
commented

I'm all for a minimum D10+PHP8.0 constraint change + dedicated major or minor versioning to support the changes required to satisfy this issue.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs work » Needs review

Good point about \Stringable being php8.0+

In the MR I've bumped php to 8.1+ (as D10 already requires that) and the core_version_requirement: ^10. So this can go into a new 3.x branch.

As for coverage, yep, we are indeed missing that. There's no existing functional that's doing any checking (or rechecking which is where we hit this fatal), so it'll need that the crawler service swapped out as a minimum. Bit to it, happy to take a look, but might make sense in a follow up?

dpi’s picture

dpi
Location Perth, Australia
commented
Version: 2.x-dev » 2.2.x-dev
dpi’s picture

dpi
Location Perth, Australia
commented

Introduced 2.1 for D9 support, in case we need to go back and fix anything. but id consider it locked for new dev.

Introduced 2.2 for D10 min support, which this PR is now based off.

I couldnt rationalise a v3 yet.

dpi’s picture

dpi
Location Perth, Australia
commented

  • dpi committed 3ebeeae0 on 2.2.x authored by fenstrat 
    Issue #3346873 by fenstrat: Fix LinkyCheckerMemoryLogger::log return...
dpi’s picture

dpi
Location Perth, Australia
commented
Status: Needs review » Fixed

As found in 2.2.0

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented

Thank you kind sir!

Status: Fixed » Closed (fixed)

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