Problem/Motivation

Links that start with //, like //www.drupal.org, are never extracted, so they are never checked. Nothing is logged.

There are two causes in LinkExtractorService::getLinks():

  1. The scheme is added twice: the URL becomes https://://www.drupal.org, which is not valid.
  2. The returned value is still the original //www.drupal.org, and isUrlInBlocklist() drops it because it does not start with http(s)://.

This worked in 7.x (#1811908: Allow extracting protocol relative url's).
It broke when the code moved to the extractor service (#2993580: Provide a link extractor service).

Steps to reproduce

  1. Enable link scanning on a formatted text field, for example the body of Basic page.
  2. Create a page with a link to //www.drupal.org/project/linkchecker and a link to https://www.drupal.org/project/drupal.
  3. Only the second link is extracted.

Proposed resolution

Prefix both the key and the value with the scheme:

$urlDecoded = $url = rtrim($httpProtocol, '/') . $urlDecoded;

Remaining tasks

Review

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork linkchecker-3629345

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

herved created an issue. See original summary.

herved opened merge request !184

herved’s picture

herved commented
Issue summary: View changes
Status: Active » Needs review
manishmittal’s picture

manishmittal commented
Status: Needs review » Reviewed & tested by the community

Tested MR !184 locally.

Environment: Drupal 11.4.8, PHP 8.3.29, SQLite, linkchecker 2.1.x (fresh drupal/recommended-project install, paragraphs and redirect added as test dependencies). The MR merges cleanly into the current 2.1.x (git merge-tree, no conflicts).

Reproduced the bug on 2.1.x. I ran the new assertion from this MR against the unpatched src/ and it fails: getLinks(['//www.drupal.org/project/linkchecker']) returns an empty array. The cause is clear in src/LinkExtractorService.php:267. $httpProtocol already ends in :// (from $request->getScheme() . '://' or the default_url_scheme setting, which defaults to https://), so the old code built http://://www.drupal.org/..., which fails UrlHelper::isValid() and the link is silently dropped.

With the MR: rtrim($httpProtocol, '/') . $urlDecoded gives http://www.drupal.org/... and the link is extracted. Assigning the result to $url as well is a good call, since otherwise the stored link would keep the scheme-less value. The cron path (no request, default_url_scheme = https://) is covered by the same rtrim() logic; I verified that by reading the code, not with a separate test.

Test suite, base vs MR (FunctionalJavascript not run):

Suite 2.1.x MR !184
Unit 67 tests, OK 67 tests, OK
Kernel 75 tests / 1154 assertions, OK 75 tests / 1155 assertions, OK
Functional (php -S) 9 tests, OK 9 tests, OK

Deprecation counts are identical to the base branch.

Coding standards: phpcs --standard=Drupal,DrupalPractice on the changed files reports no new issues (the only warning, an unused $node2 in LinkcheckerCleanUpTest.php, already exists on 2.1.x).

Minor, non-blocking: the new test only covers the request-based scheme. A second assertion for the no-request case (default_url_scheme) would protect the cron path, but I do not think it should hold this up.

Recommendation: RTBC. One-line fix, regression test fails without it and passes with it, no side effects in the full suite.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@manishmittal RE #4 thanks for the AI assisted code review. You can see that it likes to explain things that are self-evident by gitlab-ci like PHPCS and the stats, please add a skill/memory/prompt to have it not do that. I don't need a count on the test runs.