Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
I've enabled Link checker, set several fields to check for links. It's been enabled on production for a couple of months now. However, the status shows "31123 out of 20727 items have been processed." saying 150%.
It seems that this count does not update after the initial scan? One thing that we wanted to use though is the "Check link status" setting, disabling, and filtering out links based on this option. But re-analyzing content for links will reset this status, if I'm not mistaken.
| Comment | File | Size | Author |
|---|---|---|---|
| #31 | 3184613-test-only.patch | 10.29 KB | eiriksm |
| #30 | 3184613-linkchecker-extraction-status-30.patch | 12.55 KB | codebymikey |
| #24 | 3184613-test-only.patch | 5.21 KB | eiriksm |
| #5 | 3184613-5.png | 90.94 KB | Balu Ertl |
| #2 | 3184613-extraction-status.patch | 1.26 KB | parasite |
| |||
Issue fork linkchecker-3184613
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:
changes
Comments
Comment #2
parasiteThe linkchecker_entity_insert and linkchecker_entity_update puts every entites into linkchecker_index table without checking if it has links or not.
Comment #3
parasiteOh, and besides the patch a re-analyzing is required indeed.
Comment #4
parasiteComment #5
Balu ErtlNow tested on our project and hereby I confirm that the patch shared in comment #2 does resolve the issue of displaying unreal number on progress bar via
/admin/config/content/linkcheckerpage:Therefore RTBCʼing this issue.
Comment #6
C-Logemann@parasite Thanks for providing a patch. If you want to push the review process you can create an issue fork and open a merge request.
Comment #7
C-LogemannComment #8
Balu ErtlComment #9
eiriksmThe change makes sense, and the patch works well. However, would be great to have a test for this. Setting to needs review, so in case you good people agree with me, let's move this to needs work. If everyone disagree with that, we can set it back to needs work 😃️
Are you able to add a test as well, @parasite? I would be glad to help out if you need help 😃️
Comment #10
parasiteMaybe in my free time, but I'd rather say no. My effort was done in official working hours.
Comment #14
parasiteSince there is no progress here for almost 2 months ago and no one was volunteering to make tests I think there no point of holding this back anymore. Community tested already and the applied changes are simple and straightforward. If there are no urls extracted on node updates/inserts there's no need to save anything and we don't need to update linkchecker index table.
Comment #15
C-LogemannThanks @parasite for creating a merge request which is sadly not being reviewed by someone else. At first the CI system is currently reporting "Not currently mergeable" on MR !11. So this needs to be checked etc. And please do not mark your own code contribution as "RTBC".
Also please do not remove the tag "Needs tests" if currently no one has currently the time to write one. Even when the code change above will be committed this issue should be open until there is an decision or an additional issue to discuss this.
Comment #16
C-LogemannComment #17
C-LogemannIt's not easy to see why there was reported an error so this can eventually be ignored. The test situation is a nice to have but I think it's not a blocker to first add the fix and add the tests later.
Comment #18
parasiteThanks for putting the tag back it was a mistake, sorry. And regarding RTBC, it was already RTBC 2 months ago, nothing has changed since then.
Comment #19
parasiteI am not familiar enough with this gitlab based vcs yet, @C_Logemann is there something that I can do to move this forward?
Comment #20
parasiteThe first MR !11 was by mistake as it was basically empty without commits, and the 2nd one is passed so it should be ok.
Comment #21
parasite@C_Logemann could you please also make sure that this ticket was RTBC'd in comment #5? Thank you.
Comment #22
C-Logemann"RTBC" is just a status which is often be changed to "needs work" etc. in many drupal projects. In this case the community was only one reviewer. As a responsible maintainer I don't accept code changes even small ones just because of this status. So personally I need more time to test and decide or another maintainer find some time to do.
With using the new Gitlab way you already pushed the review process. It was easier for me to take a first look and it will be easier to implement in a test system.
But Everybody who need this code you contributed can use this e.g. via composer patch management.
Comment #23
parasiteJust as we did. Thanks for explanation.
Comment #24
eiriksmAdded a test for the bug. Test-only patch attached.
Comment #26
eiriksmThe test in the pr passed, and the test only patch failed, proving we are testing what the patch fixes
Comment #28
codebymikey CreditAttribution: codebymikey at Zodiac Media commentedAttached a patch which also applies the same check during batch runs.
edit: My assumption was wrong. Will try and address this when I get some time.
Comment #30
codebymikey CreditAttribution: codebymikey at Zodiac Media commentedComment #31
eiriksmLet's do a test only patch with the latest test changes from @codebymikey
Comment #33
eiriksmGreat.
So if we can just get someone who has this problem to test the current patch (that would be #30), and of course a review would also be great.
Thank you to everyone who worked on this, let's get this done! 🙌️
Comment #34
eiriksmAgain, great if someone could either confirm this fixes their issue, or at least post a review? 🤓
Comment #35
smustgrave CreditAttribution: smustgrave at Mobomo commentedWorked like a charm
Comment #36
eiriksmGreat.
I am going to merge this and tag a new release. I think this is going to avoid several of the support and bug tickets we get in this issue queue 🤞🚀
Comment #38
eiriksm