[Regression] pre-10.2 field type categories as translatable strings don't work any more

Created on 12 March 2024, 6 months ago
Updated 26 April 2024, 4 months ago

Problem/Motivation

The new field type category system appears to not support pre-10.2 field type categories that were translatable strings.

E.g. plugin module does this:

 * @FieldType(
 *   default_widget = "plugin_selector:plugin_select_list",
 *   default_formatter = "plugin_label",
 *   id = "plugin",
 *   label = @Translation("Plugin collection"),
 *   category = @Translation("Plugin"),
 *   deriver = "\Drupal\plugin\Plugin\Field\FieldType\PluginCollectionItemDeriver",
 *   list_class = "\Drupal\plugin\Plugin\Field\FieldType\PluginCollectionItemList"
 * )

and ALL of its field types end up as top-level 'general' category. With Plugin module, this makes a HUGE MESS as there are LOTS of derived field types.

This happens because of:

    if ($definition['category'] instanceof TranslatableMarkup) {
      @trigger_error('Using a translatable string as a category for field type is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3375748', E_USER_DEPRECATED);
      $definition['category'] = FieldTypeCategoryManagerInterface::FALLBACK_CATEGORY;
    }

Treating the translatable category label as just 'general' is not backwards-compatible behaviour IMO. The category label should be stringified and treated as that string.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Needs work

Version

11.0 πŸ”₯

Component
FieldΒ  β†’

Last updated 1 day ago

Created by

πŸ‡¬πŸ‡§United Kingdom joachim

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @joachim
  • πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

    Adding issue that made the change

  • Assigned to joachim
  • πŸ‡¬πŸ‡§United Kingdom joachim

    I'm working on this.

    I really think this needs to get into 10.3 as it's a regression.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    Urgh, I was hoping to add a quick and dirty hack to FieldTypePluginManager, but FieldStorageAddForm calls $this->fieldTypeCategoryManager->getDefinitions() as well, so it still finds non-existent categories.

  • πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

    This is by design, they are not meant to be supported. categories are their own definition now, it's not just a string.

    Several modules, including paragraphs and simplenews have examples on how to be compatible with both 10.2 and earlier versions. Plugin needs to be updated too.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    Until contrib modules do this update, they make the Field add form look broken. It's particularly bad with Plugin, because that module adds 20+ field types:

    Do you mean this code in Paragraphs: https://git.drupalcode.org/project/paragraphs/-/blob/8.x-1.x/paragraphs....

    That's a LOT of code to have to add to be compatible with both 10.1 and 10.2.

    When you consider that for changing a test trait we add BC handling with a deprecation message, the BC handling around the change to field type categories is REALLY bad. We should be fixing this.

  • πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

    plugin is a special case, I don't think any other module add more. For most it's 1-3 or so and you really need to rethink the category to fit into the new UI.

    Paragraphs is a special case, it's not not a field type, it's a predefined option that is being promoted up, like media. The basic example is this: https://git.drupalcode.org/project/dynamic_entity_reference/-/blob/3.x/d....

    We have BC handling, it doesn't break and it adds it to general. I don't think we should spend time in core to do more, because there is no straight forward conversion. It's a completely different UI and most modules that use a category will need to rethink that and instead consider to use an existing category. Plugin could also be seen as a reference, only the fact that there are 20 of them (which probably really shouldn't work like that, it should most likely be a setting, like entity references but there might be a technical reason why that's not possible, I don't remember) is a reason to have its own category.

  • Status changed to Needs review 5 months ago
  • πŸ‡¬πŸ‡§United Kingdom joachim

    > We have BC handling, it doesn't break and it adds it to general.

    But it looks like craaaaaaaaaaaaaaaaaaap.

    And like I say, we normally nitpick BC to the TINIEST detail, so why not here?

    I've got a fix that works. It's hacky but simple, and I don't think that it matters that it's hacky as it gets removed for 11.x anyway.

    Pushing the fix to an MR, since you seem dead set against it I'm not bothering with cleaning up the code :/

  • Pipeline finished with Failed
    5 months ago
    Total: 160s
    #153353
  • Pipeline finished with Success
    5 months ago
    Total: 1021s
    #153360
  • πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

    I'm not "dead set" against it, I just think it's not worth your time and is unlikely to get committed. This isn't a 10.3 regression, it's already in 10.2.

    Yes, plugin looks bad, but it can be improve to look better in both 10.1 and 10.2 fairly easy and again, plugin is an exceptional case.

  • Status changed to Needs work 4 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    If we do change this seems like we should have test coverage for it.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    Do we really need tests for something that's going to be removed from 11.x?

Production build 0.71.5 2024