Link formatter swallows last character if it is not a period

I was able to reproduce it here: https://3v4l.org/lWPio

$api_link = 'https://api.drupal.org/api/drupal/8.9.x/search/';
$error = [
    'message' => 'The "path.alias_manager" service is deprecated. Use "path_alias.manager" instead. See https://drupal.org/node/3092086',
];
$formatted_error = preg_replace('!deprecated function ([^(]+)\(\)!', 'deprecated function <a target="_blank" href="' . $api_link . '\1">\1()</a>', $error['message']);
$formatted_error = str_replace('\\', '\\<wbr>', $formatted_error);
$formatted_error = preg_replace('!See (https://drupal.org(.\S+)).$!', 'See <a href="\1">\1<a>.', $formatted_error);
print $formatted_error;

And regex example: https://www.phpliveregex.com/p/Cwc

Using this regex seems to fix it: See (https://drupal.org(.*)).$

The .\S+ is skipping the last character for some reason.

Hm, that assumes that there is a character (not even a dot lol), after the issue link. Is this the first one where there is none? Wow.

Well, the dot is swallowing the last character in the string, and here it is a significant character :D The .$! should probably be changed to (\.|$)! or somesuch to allow for either a dot or a direct string end.

Oh snap, I see.

  path.alias_whitelist:
    alias: path_alias.whitelist
    deprecated: 'The "%service_id%" service is deprecated. Use "path_alias.whitelist" instead. See https://drupal.org/node/3092086'
  path.alias_manager:
    class: Drupal\Core\Path\AliasManager
    arguments: ['@path_alias.repository', '@path_alias.whitelist', '@language_manager', '@cache.data']
    deprecated: 'The "%service_id%" service is deprecated. Use "path_alias.manager" instead. See https://drupal.org/node/3092086'

The regex assumes punctuation. There isn't any. So the regex is broken. So it's a core bug, technically.

Tests pass! Looks good to me, but maybe we should add a test sample that proves that deprecation message is properly reported.

@mglaman Are there plans to add a test or can this be RTBCed as is?

I think we probably need a test to verify the regex properly

Moving back for tests per #12.

Updated a tested message and the expectation that should prove this I think. Once/if that passes, a test only patch would be useful to prove that it otherwise fails and we are good IMHO.

Here is the test changes only.

Hm that did not prove it...

Maybe I'm not following, but my understanding is:

1) Regex was eating the last character of the deprecation message when there wasn't a period at the end.

2) We need a test that will show the link extracted by the regex is broken before the code is fixed.

3) If this is correct, you need to test the extracted link and not the full text. i.e. the old regex should cause the link to be like:

https://www.drupal.org/project/upgrade_statu

so if you check that link is valid, you should get a 404.

I think the last letter would be lost not just missing from the link. As shown in the screenshot in the issue summary too.

$formatted_error = preg_replace('!See (https://drupal.org(.\S+)).$!', 'See <a href="\1">\1<a>.', $formatted_error);

This will eat the last character, whatever it is (unescaped period at end of string matches whatever character there is). Then it will be replaced with a literal period. Because it matches to the end of the string, the character matched by the unescaped period would be lost. So I was expecting this simple test change should show it failing. It should not be lost from the link only but from the entire string.

Ok the problem with the test is that the bug is in the result formatter which is only used on the UI, so the analyzer test will not find it. We should update the UI test.

Ok now this should fail without the fix. #famouslastwords

Hm, the MR branch is not up to date. Rerolled the test only.

Meh, I give up. I think the fix is correct regardless of whether we can make the test coverage fit :D

So committed the fix as-is. Thanks all!

Ah ha! Glad you sorted it… good work everyone:)