Link extraction is deferred until LinkExtractorService::destruct(). If an entity is saved and deleted in the same request, its parent no longer exists when the extracted link is saved.
The original fatal was fixed by eb10370 and shipped in 2.1.0. Between 2.1.0 and #3622264 this left an orphaned linkcheckerlink behind. Since #3622264 LinkCleanUp::destruct() runs after the extractor and removes it, but the extractor still does the full extraction, saves the link and writes a linkchecker_index row for a deleted entity, and saveLink() is unguarded for direct callers.
Current result: the extractor saves a linkcheckerlink and an index row for the deleted entity, which cleanup then deletes again in the same request.
In LinkExtractorService::destruct(), skip queued entities that no longer load. In saveLink(), only save when the parent entity still exists (patch #6). Kernel test: save and delete a node in the same request, run destruct(), assert no link and no index row.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 3543895-linkchecker-null-parent-entity-fix-6.patch | 532 bytes | enchufe |
| #2 | 3543895-linkchecker-null-parent-entity-fix.patch | 682 bytes | davidburns |
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
Comments
Comment #2
davidburnsComment #3
eiriksmThanks for the patch, that looks very correct. 👌🌈
However, the steps to reproduce seem inaccurate? And the suggested fix does not align with what is the proposed solution? Based on the content of the text it seems to be generated by some LLM, so not surprised about this inaccuracy 🤓
For sure we have tests that covers exactly the steps to reproduce. So I feel pretty confident that is not the case. Which makes also the patch and issue hard to QA and verify.
In addition. Once we have actual accurate steps to reproduce it should be easy to add a test for this as well.
So the status is "needs work" based on
- needs steps to reproduce
- needs test
- needs issue summary update
Thanks for the contribution! 🚀
Comment #4
davidburnsHi @eiriksm,
You are correct that I used AI to generate the ticket description.
The only piece that was really missing from the description was that linkchecker was working fine up until I ran the committed codebase through PHPCS and PHPSTAN then fixed a massive amount of coding standards in custom themes and modules. I committed the code to the git provider where all our checks and tests run. The test that failed was when it ran cron on our Tugboat environment. I was able to confirm this error when I ran cron locally with the latest database. The site that the test ran on did have A LOT of broken links across many different entity types and I didn't capture exactly where it failed.
After creating and applying this patch all tests and checks passed.
Comment #5
davidburnsComment #6
enchufeVersion
2.1.0solves the issue, but I've reworked the patch #2 to improve the code and prevent other related errors.Comment #7
enchufeComment #9
joelpittetI reproduced the remaining issue. The original fatal was already fixed by eb10370 and shipped in 2.1.0.
However, if an entity is saved and deleted in the same request, deferred extraction still saves a
linkcheckerlinkwith no parent entity. Patch #6 fixes that by only saving when the parent still exists.I added a test covering this case: it reproduces the original fatal before eb10370 and the orphaned link on current 2.1.x.
Updated the title and summary accordingly. Keeping at Needs review for patch #6 plus the test.
Comment #11
joelpittet#3622264: Avoid expensive operations while client waits for response actually does clean it up as a destructible... hmm
Comment #12
joelpittetComment #14
joelpittetMerged into 2.1.x, thanks all!
Even though the original fatal turned out to be already fixed in 2.1.0, this issue still led us to a real gap: deferred extraction was doing full work on entities deleted in the same request. Thanks @davidburns for the report, @eiriksm for pushing for accurate steps to reproduce, and @enchufe for the patch in #6 that became the core of the fix.
The plan is to roll this into a 2.2.0 release, see the roadmap if you can help get us there #3624551: [meta] 2.2.0 stable release roadmap