Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Gábor Hojtsy in #2235457-131: Use link field for shortcut entity:
+++ b/core/modules/shortcut/src/Entity/Shortcut.php @@ -205,13 +146,22 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) { + $fields['link'] = BaseFieldDefinition::create('link') + ->setLabel(t('Path')) + ->setDescription(t('The location this shortcut points to.')) + ->setRequired(TRUE) + ->setTranslatable(FALSE)It was discussed above that even though the link field has a title (and a description following #2413029: Change LinkItem schema to also store a description), that is not being used because the entity needs a label. Why is the field kept translatable then? Do we expect to support links different per language in a shortcut? The similar menu link patch at #2406749: Use a link field for custom menu link does not make the link field translatable.
Wim Leers in #2235457-135: Use link field for shortcut entity:
Why is the field kept translatable then? Do we expect to support links different per language in a shortcut?
But the code already says
setTranslatable(FALSE)— so the link field isn't translatable.Discussed with Gábor in chat.
setTranslatable(FALSE)is the default. Hence it's opt-in, and he misread it precisely because it's opt-in: presence of asetTranslatable()call at all typically implies it's translatable.So, rectifying that detail in #2415129: Small clean-up for Shortcut entity's usage of link field.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | 2415129-1.patch | 587 bytes | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
hussainwebMarking it as RTBC tentatively, after the discussion in #2235457: Use link field for shortcut entity as indicated in the Issue Summary.
Comment #3
plachA non translatable base field definition will never be enabled for translation: is this the intended behavior? If we want to leave the possibility to have different paths per language it should be marked as translatable.
Comment #4
Gábor Hojtsy@plach: The route name it replaced was also not translatable, so this is not much of a change.
Comment #5
amateescu CreditAttribution: amateescu commentedThis cleanup could be a bit bigger based on the outcome of the title debate, so let's postpone it on whatever gets decided in #2406749: Use a link field for custom menu link.
Comment #6
Gábor HojtsyIf the link field is to be used to store translations, that would be a MUCH bigger scale change, may have performance implications, etc. so I don't think removing this one line will hurt any such effort later. It will help readability of the code in the meantime.
Comment #7
plachWorks for me, I wasn't sure this solution was meant, after reading the previous comments.
Comment #8
amateescu CreditAttribution: amateescu commentedI didn't want to imply that this patch will hurt anything, just that we might want to re-purpose it for something bigger :)
Comment #10
webchickI think there's definitely a conversation to be had about how to handle translatability of these fields, but for now this seems like a simple clean-up.
Committed and pushed to 8.0.x. Thanks!