Certain pages return 404 when making a HEAD request, but they return 200 if GET is used; in this case the broken links report would be inaccurate and report correct pages as incorrect. For many sites with many links it would require a lot of work to change each individual link to GET.
Create a page with a link to page that behaves like described in 'Motivation' (e.g. https://www.bible.com/bible/3290/MAT.1.MTHB). Then run checking the links and see the report.
Create a GET fallback for HEAD requests. If a HEAD request fails, a GET request to the target link would be called.
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | linkchecker21x-3213210-and-3334240-combined-33.patch | 6.09 KB | joseph.olstad |
| #33 | linkchecker21x-use_get_as_fallback-3334240-32.patch | 2.99 KB | joseph.olstad |
Show commandsStart 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:
changes, plain diff MR !120changes, plain diff MR !121changes, plain diff MR !56compare compare changes, plain diff MR !43 / changes, plain diff MR !37 / changes, plain diff MR !36changes, plain diff MR !42
Comments
Comment #2
eiriksmGreat 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?
Comment #4
Livecan commentedHere's the suggested change to get the idea: https://git.drupalcode.org/issue/linkchecker-3334240/-/commit/294a184e04....
We could have it configurable - have a checkbox in the config form? And maybe use default setting as fallback enabled for new installation but for existing installation leave the current behavior?
I could include the changes to the form and create the config tomorrow and put it here.
Comment #7
joelpittetComment #12
Livecan commentedSorry, had some problems with figuring out submitting my changes to the project. I hope now it should be really ready to merge.
Comment #13
eiriksmComment #16
bwaindwain commentedSwitched to 2.0.x branch and added a new branch and merge request for this feature.
Comment #17
anybodyI agree this still makes sense as option, as several hosts refuse HEAD calls, for example:
Which leads to failing / false-positive broken link checkes especially in the imprint.
Comment #18
anybody@bwaindwain could you change the target branch to 2.1.x please?
Implementation looks nice, we should get this over the finish line! :)
Comment #19
anybodyI 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:
Yes, EXACTLY this URLs! Give it a try. Any idea for the reasons, if it's already GET, not HEAD?
Comment #22
xfxfca commentedtry 'GET' if 'HEAD' fails
Comment #26
joseph.olstadComment #27
joseph.olstad@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.
Comment #28
xfxfca commentedComment #29
xfxfca commentedupdated patch
Comment #31
joseph.olstadRerolled 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.
Comment #32
joseph.olstadPlease review MR 120
Comment #33
joseph.olstadThe small patch is from MR120.
The larger patch is from the double issue MR121.
Comment #36
joelpittetThere 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.)
Comment #38
joseph.olstadI 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.
Comment #40
joelpittetLet’s make this simple, which MR only does what the issue summary and title asks of it and doesn’t creep on scope?
Comment #41
joseph.olstadAnswer: MR 120 and it is the only currently visible MR
Comment #42
joelpittetThanks @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.
Comment #43
joelpittetThe code looks great I should add, I am mostly concerned about the test
Comment #44
joseph.olstadMR 120
This one
Comment #45
anybody@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.