Use GET method as fallback if HEAD fails

Great suggestion.

This would have to be opt in, so make it configurable at least

Are you able to suggest something in a merge request or s patch?

Switched to 2.0.x branch and added a new branch and merge request for this feature.

I agree this still makes sense as option, as several hosts refuse HEAD calls, for example:

• Google
• Amazon
• Facebook
• Pexels
• ...

Which leads to failing / false-positive broken link checkes especially in the imprint.

@bwaindwain could you change the target branch to 2.1.x please?

Implementation looks nice, we should get this over the finish line! :)

I also left a comment in code.

I tried this and for many links it works and links are marked correct now. But some still are marked failed, for example:

• https://www.pexels.com
• https://pixabay.com/

Yes, EXACTLY this URLs! Give it a try. Any idea for the reasons, if it's already GET, not HEAD?

try 'GET' if 'HEAD' fails

@xfxfca please back off your patch and test the latest MR here:

https://git.drupalcode.org/project/linkchecker/-/merge_requests/120.diff

need to confirm if this works.

updated patch

Rerolled and combined:
#3334240-28: Use GET method as fallback if HEAD fails

with

#3213210-46: provide optional host and port substitution to check links behind a reverse proxy and/or docker environment

in MR 121.

Please review MR 120

The small patch is from MR120.

The larger patch is from the double issue MR121.

There are quite a few proposed solutions here, and it’s not clear which one is meant to move forward. I hid two that looked like accidental MRs (hopefully correctly!).

The issue summary is excellent though! Short, clear, and the title is specific. That really helps. Let’s make it easy for maintainers to commit something.

@joseph.olstad instead of me trying to guess, maybe you could hide any MRs/branches we no longer need? If you’re combining approaches, a patch might be a better fit anyway, since it’s composer-patchable and avoids branch clutter.

I think most of this confusion came from creating a 2.0.x target branch on a fork that didn’t already have it — GitLab makes you create it before you can select it as the MR target, which unintentionally makes it look like it’s part of the issue work. (I still haven’t figured out how to avoid that either.)

I will argue that MR 56 is not needed since linkchecker already offers a domain blacklist to handle the option.

MR 120 has been extensively tested and also passes automated testing.

Let’s make this simple, which MR only does what the issue summary and title asks of it and doesn’t creep on scope?

Answer: MR 120 and it is the only currently visible MR

Thanks @joseph.olstad I will reviewMR 121 later, first glance the test could use something a bit more explicit, but I will look closer as there is probably context lines that make it clearer already in place.

The code looks great I should add, I am mostly concerned about the test

MR 120

This one

@joelpittet looks like you accidentally checked MR121 instead of MR120 as @joseph.olstad pointed out in #44?

@joseph.olstad the MR looks good and clear to me, I just left two notes, maybe you'd like to have a look.

I agree this is very helpful and should get released soon.

On further reflection, I think it makes sense to have a configuration and have it disabled by default, so existing sites don't have a bunch of extra requests being made that weren't previously being made.

Also improved the fidelity of the tests.

edit: Ignore the wrongly named diff file.

It's the latest static version of the current MR.

has tests, passes tests, let's merge that

MR 120 needs to be merged asap. To eliminate false 404s,many people need this and most of them do not even realize what's going on.

Let's just merge this in today and cut a release.

When this gets merged, we'll need to reroll the solutions in #3213210: provide optional host and port substitution to check links behind a reverse proxy and/or docker environment

FYI: The array type breaks PHP 8.2 compatibility

const array HEAD_FALLBACK_STATUS_CODES = [
  400, 401, 403, 404, 405, 406,
];

ParseError: syntax error, unexpected identifier "HEAD_FALLBACK_STATUS_CODES", expecting "=" in Composer\Autoload\{closure}() (line 31 of /web/modules/contrib/linkchecker/src/LinkCheckerService.php).

I was about to merge this, but I felt like the other fix that I just made took priority so this one now needs a re-roll if somebody could take care of that, I will be happy to commit it as well

I attempted to resolve the conflicts and pushed it up, I have no idea if I resolved the conflicts correctly or not. Lets see what the tests say.

MR 21 is hosed since the conflicts. Will work on it later.

Meanwhile, MR 20 seems ok and is passing tests. MR21 is a combined solution patch with another issue (we are using an older version of it). Needs work.

@joseph.olstad sorry not totally understanding the need for this feature in MR 121 to me it feels outside the scope of this module and/or this issue and shouldn't be worked on here.

Can you move that to a different issue please? Or didn't you already in #3213210: provide optional host and port substitution to check links behind a reverse proxy and/or docker environment in comment #52? It seems to be just adding noise here and making it harder to get committed.

@joelpittet

it's because we require the solutions for this issue 3334240 and also 3213210 , we cannot apply both patches since there's conflicts. That's why we have MR 21

and there's conflicts with the recent changes.

I don't have time now to redo these patches. Significant time went in to getting them working the first time.

Would be great to move forward with MR #20 so that we have less bs to deal with.

Every time we upgrade linkchecker, it takes us days and days and days to do manual regression testing with all of the combined fixes. Both issues are serious for us. We won't be able to test this until there's solutions either merged or patchable for both of these issues.

3334240 is very important since many false broken links show up unless this solution is in place

3213210 is very important to us since we use docker with varnish in front and a reverse proxy and linkchecker simply will not function unless we run with a functional patch for this (ugh, rerolls are currently a mess).

For us, we're currently unable to upgrade until these two issues are either patchable or fixed.

Perhaps someone else will sort this out for us both ways before I have to.

MR 20 needs review.

Throw a combined patch up if you’re needing both, exclude the tests to reduce patch conflicts. It’s easier to understand and you can be explicit in the filename.

I'm hoping MR 120 goes in and we'll have one less moving target to deal with.

I should clarify but you mean MR 120, not MR 20 from #3259507: Fragments of internal links are not checked?

Yes I actually meant MR 120 from this issue. lol, I think my brain did some strange concatenation with the ! exclamation keyword. some cache clear needed in my brain.

Yeah I figured, I am not sure why the explanation marks are used but they confuse me too

Rebased for current 2.1.x!

This should go in asap because it's ready and it's on the critical path.

Once this is in, I'll work on my other rerolls since there's conflicts touching the same lines of code elsewhere in my other issues.

Thank you all! In my eyes MR!120 is ready for RTBC. Any objections?

@joelpittet

I had another look, this is clean, has tests, still ready to go.

Please merge.

I need this one in before working on other merge requests.