Problem/Motivation

Hi

Originally saw this on slack but now i have experienced in my dev setup

In #3372097, the file core/lib/Drupal/Core/Field/FieldTypeCategory.php was introduced for 10.2.x.

After the update to that version, if there is any custom FieldItem located in any module (custom or contrib) that does not have description = @Translation("..."), in its annotation (as it happened on my case), when going to any Content Type and adding a new field to it, the site will return the following error:

TypeError: Drupal\Core\Field\FieldTypeCategory::getDescription(): Return value must be of type Drupal\Core\StringTranslation\TranslatableMarkup, string returned in Drupal\Core\Field\FieldTypeCategory->getDescription() (line 26 of core/lib/Drupal/Core/Field/FieldTypeCategory.php).

Steps to reproduce

1. Have any custom field in Plugin/Field/FieldItem without description in the annotation
2. CR
3. Go to admin/structure/types/manage/<bundle>/fields
4. Site crashes

Proposed resolution

There should be a warning to indicate the description is missing in the annotation, so in case you are in a contrib module, the developer/maintainer adds it.
Meanwhile, in order to avoid the error, if the function is changed to something like: 

  /**
   * {@inheritdoc}
   */
  public function getDescription(): TranslatableMarkup {
    return ($this->pluginDefinition['description'] instanceof TranslatableMarkup ? $this->pluginDefinition['description'] : new TranslatableMarkup($this->pluginDefinition['description']));
  }

it will avoid the error, as if the description is missing in the annotation, $this->pluginDefinition['description'] will return '' while the function expects a TranslatableMarkup object

Best

Issue fork drupal-3409413

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:

About issue forks

Comments

gorkagr created an issue. See original summary.

gorkagr’s picture

gorkagr commented
Title: Error TypeError: Drupal\Core\Field\FieldTypeCategory::getDescription() if a FeildType has description missing in the annotation » Error TypeError: Drupal\Core\Field\FieldTypeCategory::getDescription() if a FieldType has 'description' missing in its annotation

gorkagr opened merge request !5858

gorkagr closed merge request !5858

gorkagr opened merge request !5861

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

gorkagr’s picture

gorkagr commented

Thnks @lauriii
That one also works

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs work

Coding standards checker doesn't like it though:

 19 | WARNING | Do not pass empty strings to t()

I think there is also no guarantee that someone has used @Translation in an annotation, I think we should add a test with:

 * @FieldType(
 *   id = "test_field",
 *   label ="Untranslated test field",
 *   description = "Dummy untranslated field type used for tests.",
 *   default_widget = "test_field_widget",
 *   default_formatter = "field_test_default"
 * )

or similar

gorkagr’s picture

gorkagr commented

Hi @longwave

Having only description = "text" in the annotation causes:

TypeError: Drupal\Core\Field\FieldTypeCategory::getDescription(): Return value must be of type Drupal\Core\StringTranslation\TranslatableMarkup, string returned in Drupal\Core\Field\FieldTypeCategory->getDescription() (line 26 of core/lib/Drupal/Core/Field/FieldTypeCategory.php).

so yes, there is an issue with that part too

gorkagr’s picture

gorkagr commented

Something such as


  /**
   * {@inheritdoc}
   */
  public function __construct(array $configuration, string $plugin_id, array $plugin_definition) {
    $plugin_id = $configuration['unique_identifier'];
    $plugin_definition = [
      'label' => $configuration['label'],
      'description' => (isset($configuration['description'])
        ? ($configuration['description'] instanceof TranslatableMarkup ? $configuration['description'] : new TranslatableMarkup($configuration['description']))
        : new TranslatableMarkup('')),
      'weight' => $configuration['weight'] ?? 0,
    ] + $plugin_definition;
    parent::__construct($configuration, $plugin_id, $plugin_definition);
  }

can cover the empty case, the string only and the use of @translation in the annotation.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented 
      'description' => (isset($configuration['description'])
        ? ($configuration['description'] instanceof TranslatableMarkup ? $configuration['description'] : new TranslatableMarkup($configuration['description']))

I don't think this is the right solution either, as we shouldn't pass arbitrary strings to TranslatableMarkup (coding standards checker may also complain about this). I think a better solution might be to relax the types in places so we accept either TranslatableMarkup or string.

chebbro’s picture

chebbro commented

I have this problem when i enable commerce_tax module

chebbro’s picture

chebbro commented

Oh, thanks @lauriii! It's work

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review

Changed the approach so that it doesn't require an empty translatable string

gorkagr’s picture

gorkagr commented

Hi @lauriii

Applied the last patch. It works with and without the @traslation in the description and also with plugins without the description

Thnks for the patch :)

gorkagr’s picture

gorkagr commented
Status: Needs review » Reviewed & tested by the community
freddy rodriguez’s picture

freddy rodriguez
Location Bogotá
commented

+1 works perfect.

In this case, this bug limits the addition of new fields of any content type in D10.2. Should be considered critical?

  • longwave committed 9dddb680 on 10.2.x 
    Issue #3409413 by gorkagr, lauriii, longwave: Error TypeError: Drupal\...

  • longwave committed 19f5bdd1 on 11.x 
    Issue #3409413 by gorkagr, lauriii, longwave: Error TypeError: Drupal\...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

As per the priority criteria this is still major and not critical, I classify this as "causes a significant admin- or developer-facing bug with no workaround" but there is no data loss and the site is still usable apart from this.

Committed and pushed 19f5bdd1e3 to 11.x and 9dddb68024 to 10.2.x. Thanks!

keiserjb’s picture

keiserjb commented

Thanks for getting this fix up on here. There are lots of modules that don't include a description in the annotations. This patch does fix things. I couldn't add fields on multiple sites and now I can.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Thank you for this fix!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.