Problem/Motivation

The Drupal\Core\Field\Plugin\Field\FieldFormatter\StringFormatter checks in its ::settingsForm method on the entity type, whether it has a canonical link template. That is correct.

However, it also checks for canonical link template existence within ::viewElements on the entity type. Since single entity instance are allowed to define whether they have a canonical link template on their own via EntityInterface::hasLinkTemplate, the StringFormatter should respect that accordingly.

Steps to reproduce

1. Generate new entity:content
drush generate entity:content

Module machine name:
 ➤ custom_entity
 Module name [Custom entity]:
 ➤ 
 Entity type label [Custom entity]:
 ➤ 
 Entity type ID [custom_entity]:
 ➤ 
 Entity class [CustomEntity]:
 ➤ 
 Entity base path [/custom-entity]:
 ➤ 
 Make the entity type fieldable? [Yes]:
 ➤ 
 Make the entity type revisionable? [No]:
 ➤ 
 Make the entity type translatable? [No]:
 ➤ 
 The entity type has bundle? [No]:
 ➤ 
 Create canonical page? [Yes]:
 ➤ 
 Create entity template? [Yes]:
 ➤ 
 Create CRUD permissions? [No]:
 ➤ 
 Add "label" base field? [Yes]:
 ➤ 
 Add "status" base field? [Yes]:
 ➤ 
 Add "created" base field? [Yes]:
 ➤ 
 Add "changed" base field? [Yes]:
 ➤ 
 Add "author" base field? [Yes]:
 ➤ 
 Add "description" base field? [Yes]:
 ➤ 
 Create REST configuration for the entity? [No]:
 ➤ 
 The following directories and files have been created or updated:
–––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
 • /var/www/html/web/modules/custom_entity/custom_entity.info.yml
 • /var/www/html/web/modules/custom_entity/custom_entity.links.action.yml
 • /var/www/html/web/modules/custom_entity/custom_entity.links.contextual.yml
 • /var/www/html/web/modules/custom_entity/custom_entity.links.menu.yml
 • /var/www/html/web/modules/custom_entity/custom_entity.links.task.yml
 • /var/www/html/web/modules/custom_entity/custom_entity.module
 • /var/www/html/web/modules/custom_entity/custom_entity.permissions.yml
 • /var/www/html/web/modules/custom_entity/custom_entity.routing.yml
 • /var/www/html/web/modules/custom_entity/config/install/system.action.custom_entity_delete_action.yml
 • /var/www/html/web/modules/custom_entity/config/install/system.action.custom_entity_save_action.yml
 • /var/www/html/web/modules/custom_entity/src/CustomEntityInterface.php
 • /var/www/html/web/modules/custom_entity/src/CustomEntityListBuilder.php
 • /var/www/html/web/modules/custom_entity/src/Entity/CustomEntity.php
 • /var/www/html/web/modules/custom_entity/src/Form/CustomEntityForm.php
 • /var/www/html/web/modules/custom_entity/src/Form/CustomEntitySettingsForm.php
 • /var/www/html/web/modules/custom_entity/templates/custom-entity.html.twig

2. Edit custom_entity/src/Entity/CustomEntity.php file adding inside the CustomEntity class the following function

/**
* {@inheritdoc}
*/
public function hasLinkTemplate($rel): bool {
  if ($rel === 'canonical') {
    return FALSE;
  }
  return parent::hasLinkTemplate($rel);
}

3. Edit custom_entity.info.yml to change `core_version_requirement: ^10 || ^11`

4. Enable module custom_entity
drush en custom_entity

5. Manage custom_entity display (/admin/structure/custom-entity/display) define "Label" field format to "Plain text" with Link to the Custom entity

6. Add new content custom_entity (/custom-entity/add) and view the content /custom-entity/1
The field label is displayed has a link whereas class CustomEntity define hasLinkTemplate('canonical') to return FALSE.
The patch correcting this comportment to display field label without link.

Proposed resolution

Replace the check on entity type level within StringFormatter::viewElements by a check on the entity itself.
Meaning changing this line

if ($this->getSetting('link_to_entity') && !$entity->isNew() && $entity_type->hasLinkTemplate('canonical')) {
to
if ($this->getSetting('link_to_entity') && !$entity->isNew() && $entity->hasLinkTemplate('canonical')) {

Remaining tasks

Add steps to reproduce
Turn to MR

User interface changes

NA

API changes

NA

Data model changes

NA

Release notes snippet

NA

Issue fork drupal-3269889

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mxh created an issue. See original summary.

mxh’s picture

Issue tags: +good-first-issue

I think this might be a good one for first-time contributors.

cilefen’s picture

Issue tags: +Novice

franck_lorancy made their first commit to this issue’s fork.

Shashwat Purav’s picture

Status: Active » Needs review
FileSize
818 bytes

Created patch to replace the check on entity type level within StringFormatter::viewElements by a check on the entity itself.

Libbna’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
26.26 KB

applied #6 patch and it LGTM.
Added screenshot.

cilefen’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thank you all. Core requires regression test coverage, so that is the next step. Usually you can enhance an existing test.

renatog’s picture

Usually you can enhance an existing test.

@cilefen which one?

cilefen’s picture

What I wrote in #8 isn't a riddle. Often there is already a test of a component or feature. If there isn't one, then add a new test.

franck_lorancy’s picture

FileSize
1.47 KB

I am starting with phpUnit, can the test be added like this way?
Maybe someone can help me.

mxh’s picture

@franck_lorancy PHPUnit is the right framework to use, and you're extending already the right test method. The test should assert regarding the generated output of the string formatter. For doing so I think you can make use of $this->renderEntityFields similar to the other existing calls in that test method. The output should assert that

  • A link is being generated for entities having a canonical URL when link_to_entity is set to TRUE
  • A link is not being generated for entities having a canonical URL when link_to_entity is set to FALSE
  • A link is not being generated for entities not having a canonical URL when link_to_entity is set to TRUE
  • A link is not being generated for entities not having a canonical URL when link_to_entity is set to FALSE

Hope this helps.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andregp’s picture

I'm going to work on these tests. I'll send the updated patch here soon.

mxh’s picture

@andregp that's great news. If you work on this one, you can assign yourself to this issue so that others don't accidentally work at the same time on this one too.

andregp’s picture

I wasn't able to create functional tests (that would fail without the fix, and work with the fix), neither was I able to understand and manipulate the entities to not have a canonical URL.

mxh’s picture

Issue tags: -good-first-issue, -Novice

Ok no problem, but thanks for providing feedback. Removing tags.

franck_lorancy’s picture

Status: Needs work » Needs review
FileSize
2.44 KB

@mxh I tried to follow your explanation, maybe we can do something like that.

mxh’s picture

Status: Needs review » Needs work

@franck_lorancy Thank you for creating a patch with tests.

The tests are not covering the change, as the display configuration is not affecting this change. The change affects the entity itself, and therefore we need to test regards the entity.

One way could be to extend the already existing Drupal\entity_test\Entity\EntityTest class with two bundle classes. One bundle class may have canonical URLs enabled (calling its method ::hasLinkTemplate('canonical') would return true), and the other one has it not enabled (calling its method ::hasLinkTemplate('canonical') would return false). For a starting point, there is an already existing bundle class Drupal\entity_test_bundle_class\Entity\EntityTestBundleClass which extends the EntityTest class. The tests would then assert that an instance of the first bundle would render a link, whereas an instance of the other bundle would not.

Ankit.Gupta’s picture

Version: 9.4.x-dev » 9.5.x-dev
Status: Needs work » Needs review
FileSize
2.47 KB
1.1 KB

Reroll the patch #18 with Drupal 9.5.x and Fixed custom commands.

mxh’s picture

Status: Needs review » Needs work

See #19

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

franck_lorancy’s picture

Status: Needs work » Needs review
FileSize
8.95 KB

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

tstoeckler’s picture

tstoeckler’s picture

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

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
2.84 KB

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.

franck_lorancy’s picture

Fix Needs Review Queue Bot

franck_lorancy’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

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

franck_lorancy’s picture

@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 ?

smustgrave’s picture

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

franck_lorancy’s picture

Status: Needs work » Needs review

@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.

smustgrave’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs tests

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

franck_lorancy’s picture

Issue summary: View changes
Status: Needs work » Needs review
smustgrave’s picture

franck_lorancy’s picture

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.

smustgrave’s picture

Issue summary: View changes
Status: Needs review » Needs work

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.

franck_lorancy’s picture

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

franck_lorancy’s picture

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.

franck_lorancy’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Some feedback on the MR.