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
Comment | File | Size | Author |
---|
Issue fork drupal-3269889
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:
Comments
Comment #2
mxhI think this might be a good one for first-time contributors.
Comment #3
cilefen CreditAttribution: cilefen commentedComment #6
Shashwat Purav CreditAttribution: Shashwat Purav at Portage CyberTech for Drupal Association commentedCreated patch to replace the check on entity type level within
StringFormatter::viewElements
by a check on the entity itself.Comment #7
Libbna CreditAttribution: Libbna as a volunteer and at QED42 for Drupal India Association commentedapplied #6 patch and it LGTM.
Added screenshot.
Comment #8
cilefen CreditAttribution: cilefen commentedThank you all. Core requires regression test coverage, so that is the next step. Usually you can enhance an existing test.
Comment #9
renatog@cilefen which one?
Comment #10
cilefen CreditAttribution: cilefen commentedWhat 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.
Comment #11
franck_lorancy CreditAttribution: franck_lorancy commentedI am starting with phpUnit, can the test be added like this way?
Maybe someone can help me.
Comment #12
mxh@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 thatlink_to_entity
is set toTRUE
link_to_entity
is set toFALSE
link_to_entity
is set toTRUE
link_to_entity
is set toFALSE
Hope this helps.
Comment #14
andregp CreditAttribution: andregp at CI&T commentedI'm going to work on these tests. I'll send the updated patch here soon.
Comment #15
mxh@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.
Comment #16
andregp CreditAttribution: andregp at CI&T commentedI 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.
Comment #17
mxhOk no problem, but thanks for providing feedback. Removing tags.
Comment #18
franck_lorancy CreditAttribution: franck_lorancy commented@mxh I tried to follow your explanation, maybe we can do something like that.
Comment #19
mxh@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 returntrue
), and the other one has it not enabled (calling its method::hasLinkTemplate('canonical')
would returnfalse
). For a starting point, there is an already existing bundle classDrupal\entity_test_bundle_class\Entity\EntityTestBundleClass
which extends theEntityTest
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.Comment #20
Ankit.Gupta CreditAttribution: Ankit.Gupta at Dotsquares Ltd. commentedReroll the patch #18 with Drupal 9.5.x and Fixed custom commands.
Comment #21
mxhSee #19
Comment #24
franck_lorancy CreditAttribution: franck_lorancy commented@mxh I tried to follow your recommendations.
Can you take a look and tell me if any adjustments are needed?
Thank you in advance.
Comment #25
tstoecklerComment #26
tstoecklerRead the patch now and I guess the issues aren't really strictly related. Sorry for the noise.
Comment #27
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #28
franck_lorancy CreditAttribution: franck_lorancy commentedFix Needs Review Queue Bot
Comment #29
franck_lorancy CreditAttribution: franck_lorancy commentedComment #30
smustgrave CreditAttribution: smustgrave at Mobomo commentedRan testLinkEntitiesWithCanonical without the fix and it still passes. So not sure if it's covering the bug. Leaving tests tag for that.
Comment #31
franck_lorancy CreditAttribution: franck_lorancy commented@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 ?
Comment #32
smustgrave CreditAttribution: smustgrave at Mobomo commentedTypically 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
Comment #33
franck_lorancy CreditAttribution: franck_lorancy commented@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 :
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.
Comment #34
smustgrave CreditAttribution: smustgrave at Mobomo commentedRemoving 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
Comment #36
franck_lorancy CreditAttribution: franck_lorancy commentedComment #37
smustgrave CreditAttribution: smustgrave at Mobomo commentedIs the MR good to review? See it's marked draft.
Comment #38
franck_lorancy CreditAttribution: franck_lorancy commentedYes, the MR is good to review.
Comment #41
smustgrave CreditAttribution: smustgrave at Mobomo commentedHiding 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.
Comment #42
franck_lorancy CreditAttribution: franck_lorancy commentedI'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
Comment #43
franck_lorancy CreditAttribution: franck_lorancy commentedI 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.
https://git.drupalcode.org/issue/drupal-3269889/-/jobs/1013107
I confirm, tests are ok with phpUnit on my locale.
Comment #44
franck_lorancy CreditAttribution: franck_lorancy commentedComment #45
smustgrave CreditAttribution: smustgrave at Mobomo commentedSome feedback on the MR.