StringFormatter shows link for entities that might not have a canonical URL

@mxh I tried to follow your recommendations.
Can you take a look and tell me if any adjustments are needed?
Thank you in advance.

Read the patch now and I guess the issues aren't really strictly related. Sorry for the noise.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fix Needs Review Queue Bot

Ran testLinkEntitiesWithCanonical without the fix and it still passes. So not sure if it's covering the bug. Leaving tests tag for that.

@Thanks for the review @smustgrave.
The goal of the issue is to replace the check on the entity_type ($entity_type->hasLinkTemplate('canonical')) inside StringFormatter::viewElements by a check on the entity itself ($entity->hasLinkTemplate('canonical')).
The test "testLinkEntitiesWithCanonical" passes because the return of StringFormatter::viewElements stilling to be the same with and without the fix.
Do you think it is necessary to add a new test to illustrate the check replacement on the entity_type by on the entity itself ?

Typically for bugs we have a test case to show the issue. There are a few times where that’s not needed. This instance I can’t say, but leaning towards a test

@smustgrave the test case to show the issue is testLinkEntitiesWithoutCanonical.
The following lines check render don't contains link when an entity without canonical is displayed with link to entity :

// Test a link is not being generated for entities without canonical URLs
// when link_to_entity is set to TRUE.
$this->renderEntityFields($entity_without_canonical, $display_with_link);
$this->assertNoLink($value);
$this->assertNoLinkByHref($entity_without_canonical->toUrl()->toString());

The 2 asserts failed when we keeping original core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php.
The 2 asserts succeed when we apply patch into core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php.

The testLinkEntitiesWithCanonical which passes with or without the fix can be removed, @mxh proposed to write it (#12).
Say me if we keep or remove it. Thank you in advance.

Removing tests tag as they appear to be added

Can steps to reproduce be added to the issue summary. Put NA on sections I believe may not apply to this ticket.

Recommend turning patch to MR for reviews and hiding all patches

New functions should be typehinted example testLinkEntitiesWithCanonical(): void

Is the MR good to review? See it's marked draft.

Yes, the MR is good to review.

smustgrave → changed the visibility of the branch 9.3.x to hidden.

smustgrave → changed the visibility of the branch 3269889-stringformatter-shows-link to hidden.

Hiding old MRs for clarity.

Ran the test-only feature https://git.drupalcode.org/issue/drupal-3269889/-/jobs/815018 unfortunately it passes when it should of failed.

I'm not sure Reverting core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php works, I read the following error :
fatal: invalid reference: refs/heads/11.x

I add the branch 11.x to the fork but now the 4 tests fails with the same error, even the tests elready present which have not been modified.

Drupal\Tests\field\Kernel\KernelString\StringFormatterTest::testStringFormatter
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "string" plugin does not exist. Valid plugin IDs for Drupal\Core\Field\FormatterPluginManager are: text_trimmed, text_summary_or_trimmed, text_default, entity_reference_custom_cache_tag, user_name, author, uri_link, timestamp, timestamp_ago, number_unformatted, email_mailto, language, number_integer, entity_reference_label, entity_reference_entity_id, entity_reference_entity_view, number_decimal, boolean, basic_string

https://git.drupalcode.org/issue/drupal-3269889/-/jobs/1013107

I confirm, tests are ok with phpUnit on my locale.

Some feedback on the MR.

Did not re-review yet but MR appears to have failures.

Not just canonical url support is problematic, but the lack of url_callback support as well, see 🐛 StringFormatter does not support entity uri_callback Active .