Problem/Motivation

The annotation in core/modules/block_content/src/Entity/BlockContentType.php looks like this:

 * @ConfigEntityType(
 *   id = "block_content_type",
 *   label = @Translation("Custom block type"),
 *   label_collection = @Translation("Custom block types"),
 *   label_singular = @Translation("custom block type"),
 *   label_plural = @Translation("custom block types"),
 *   label_count = @PluralTranslation(
 *     singular = "@count custom block type",
 *     plural = "@count custom block types",
 *   ),
 *   label_collection = @Translation("Custom block library"),

Proposed resolution

As you can see, label_collection is defined TWICE.

  1. The first label_collection was added as part of an older issue to add entity labels: #2702683: Add plural labels to entity types
  2. The second label_collection was committed two months earlier than that, by: #2919890: Make BlockContentType use a route provider instead of hard-coded routes

I checked all content and config entities defined by core, for duplicate use of label, label_collection, label_singular, label_plural, or label_count, and this is the only instance of duplicate labels I found.

This problem exists in D8.8, D8.9, D9.0, and D9.1.

The patch removes the second use of label_collection because the text is not consistent with the other labels.

Remaining tasks

None

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TR created an issue. See original summary.

TR’s picture

Status: Active » Needs review
FileSize
587 bytes
TR’s picture

Issue summary: View changes
TR’s picture

Issue tags: +Quick fix, +Novice
Sivaji_Ganesh_Jojodae’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

+1 to commit this patch. RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I'm not sure about removing the second one. It changes the title on admin/structure/block/block-content/types and the result of calling \Drupal::entityTypeManager()->getDefinition('block_content_type')->getCollectionLabel();

The no change version of this change is to remove the first one.

TR’s picture

Status: Needs work » Needs review
FileSize
973 bytes

Yes, it does change the title, but why is that bad?

This is not a breaking string change - BOTH of the strings are currently defined and exist in the translation table. We're removing one of the strings with this patch but we're not changing any remaining strings.

Right now the other labels on the entity are:

 *   label_singular = @Translation("custom block type"),
 *   label_plural = @Translation("custom block types"),
 *   label_count = @PluralTranslation(
 *     singular = "@count custom block type",
 *     plural = "@count custom block types",
 *   ),

So to me it makes sense to use:
* label_collection = @Translation("Custom block types"),

But if it concerns you, here's an alternate patch that instead uses:
* label_collection = @Translation("Custom block library"),

jungle’s picture

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

Using Custom block types is better than Custom block library to me. But from my understanding, for history reasons, keeping it as Custom block library is better, or end users might get confused and try to find Custom block library still.

Just like the well-known typo HTTP referer (a misspelling of referrer).

So #7 is RTBC.

jungle’s picture

#3116502: Wrong field_ui_base_route or duplicate field_ui_base_route in badge entity is a similar one which filed by me. I am pondering that is it worthy to write a test for this to avoid duplicate keys in annotations of config entities, annotation-based plugins, as this is a quick fix. I won't tag "Needs test", so let's do it in a separate issue if we agree on this.

alexpott’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 2dbb3e450a to 9.1.x and ad66016aac to 9.0.x and 48945fd26c to 8.9.x. Thanks!

Now we're not changing any UI strings this can be backported all the way to 8.9.x

  • alexpott committed 2dbb3e4 on 9.1.x
    Issue #3137430 by TR, jungle, alexpott: label_collection is defined...

  • alexpott committed ad66016 on 9.0.x
    Issue #3137430 by TR, jungle, alexpott: label_collection is defined...

  • alexpott committed 48945fd on 8.9.x
    Issue #3137430 by TR, jungle, alexpott: label_collection is defined...
alexpott’s picture

@jungle +1 to opening a coder issue to check for duplicate annotation keys... it might be worth looking to see and any third party sniffs cover annotations.

jungle’s picture

@alexpott, thanks for committing! An issue opened to coder.

Status: Fixed » Closed (fixed)

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