Problem/Motivation

As it has been introduced in #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed now we have a way to specify plural label for any entity type.

Proposed resolution

Add plural labels to all entity types defined in core. More information can be found in the change record: https://www.drupal.org/node/2689949

Remaining tasks

  • List all entity types and determine which still need labels.
  • Determine the correct capitalization patterns in English and make it consistent.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

CommentFileSizeAuthor
#70 reroll-diff-2702683-64-69.txt844 bytesManuel Garcia
#69 2702683-69.patch29.42 KBpguillard
#64 2702683-64.patch29.45 KBManuel Garcia
#64 interdiff-2702683-59-64.txt16.21 KBManuel Garcia
#59 2702683-59.patch27.82 KBManuel Garcia
#59 interdiff-2702683-57-59.txt2.87 KBManuel Garcia
#57 2702683-57.patch24.95 KBManuel Garcia
#57 interdiff-2702683-55-57.txt2.84 KBManuel Garcia
#55 2702683-55.patch24.3 KBManuel Garcia
#55 interdiff-2702683-53-55.txt4.33 KBManuel Garcia
#53 2702683-53.patch21.28 KBjofitz
#53 interdiff-51-53.txt4.4 KBjofitz
#51 2702683-51.patch16.88 KBjofitz
#42 interdiff.txt2.27 KBManuel Garcia
#42 2702683-42.patch17.52 KBManuel Garcia
#40 plural-translations-2702683-40.patch17.42 KBrosinegrean
#28 interdiff-27-28.patch2.48 KBvg3095
#28 plural-translations-2702683-28.patch18.75 KBvg3095
#27 plural-translations-2702683-27.patch16.27 KBrosinegrean
#27 interdiff-26-27.txt594 bytesrosinegrean
#26 interdiff-22-26.txt2.74 KBztl8702
#26 plural-translations-2702683-26.patch15.69 KBztl8702
#22 interdiff-2702683-11-22.txt2.22 KBpguillard
#22 plural-translations-2702683-22.patch15.69 KBpguillard
#12 interdiff-11.patch618 bytesxjm
#12 plural-translations-2702683-11.patch13.06 KBxjm
#7 plural-translations.patch12.46 KBxjm
#3 2702683-3.patch1.48 KBlucian.cazaciuc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lucian.cazaciuc created an issue. See original summary.

lucian.cazaciuc’s picture

Issue tags: +DCTransylvania
lucian.cazaciuc’s picture

Assigned: lucian.cazaciuc » Unassigned
Status: Active » Needs review
FileSize
1.48 KB

I fixed it with the help of amateescu and the great DrupalHeroes at DrupalTransilvania Camp

jibran’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Quickfix
xjm’s picture

Title: Add plural labels to Entity Types in Contact module » Add plural labels to content entity types
Status: Reviewed & tested by the community » Needs work

I don't think it makes sense to scope these issues on a per-module basis. Please see:
https://www.drupal.org/core/scope#incomplete
(Edit: more applicable fragment link)

Retitling to what I think the correct scope should be. Let's close the other issues as duplicated and add them to this patch.

xjm’s picture

Title: Add plural labels to content entity types » Add plural labels to entity types

Guess it applies to both content and config. Does someone want to do a quick count of how many there are that should have plural labels, and add that to the summary?

xjm’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
12.46 KB

Here is the combined patch from the RTBC issues that still applied. Search and aggregator did not. (Actually they are in the patch, must have been my mistake.) Thanks to everyone who helped on the other issues -- I'll try to list all the contributors on here shortly.

Also, 8.1.x is in string freeze, so moving to 8.2.x.

xjm’s picture

Component: contact.module » entity system
Status: Needs review » Needs work
Issue tags: -Quickfix +Needs issue summary update

NW for a list of which entity types need fixing and have been fixed per #6.

xjm’s picture

dpopdan, bojanz, rszrama, vasike, vdanielpop, rteijeiro, pguillard, jmikii, svjuly, Drupa1ish, jhodgdon, ciprian.stavovei, Wim Leers, Gabr1el all contributed to the other issues I closed as a duplicate so far. (Edit: updated). amateescu is also mentioned as a mentor and should also be credited.

xjm’s picture

BTW, if anyone is wondering "Why would we bother to combine all these when they are already done?" see for example:
#2702695-4: Add plural labels to Entity Types in editor module

If we split the change up into lots of tiny patches, then a reviewer cannot ensure it is applied consistently across core.

vdanielpop’s picture

The purpose of the issues created separately was to introduce the new guys to the workflow, but you're right, having all the changes together makes it clearer for the reviewer. Thanks :)

xjm’s picture

Adding in the patch from the Editor issue, and quoting the full comment:

Just one remark:

+++ b/core/modules/editor/src/Entity/Editor.php
@@ -16,6 +16,12 @@
  *   label = @Translation("Text Editor"),
+ *   label_singular = @Translation("Text Editor"),
+ *   label_plural = @Translation("Text Editors"),
+ *   label_count = @PluralTranslation(
+ *     singular = "@count Text Editor",
+ *     plural = "@count Text Editors"

They're consistently capitalized, but I wonder if they should actually all be lowercase?

Deferring that decision to a committer.

I can actually see the case for either. Capitalization is translation-specific, but with this new API, we will allow each language to specify a (more) grammatical capitalization for that language. (It still runs into trouble if used in a sentence but that's a limitation of the original issue.) In English, I could see cases being made for both ways. We say (e.g.) "the Taxonomy Access Control module" so I think by that reasoning it would be capitalized, but I know core goes to lengths to try to lowercase things in other contexts, so let's get more feedback on that point.

xjm’s picture

Status: Needs work » Needs review

(NR to trigger the test run.)

Thanks @vdanielpop! I can see the benefit in that too. (Also, adding your issue credit from the other issue -- thanks for commenting!)

xjm’s picture

Issue summary: View changes
Status: Needs review » Needs work

Adding a note about making capitalization consistent to the summary. Some instances are lowercase and others are uppercase, so we need to make it consistent one way or the other in English.

xjm’s picture

Issue summary: View changes
bojanz’s picture

+1 from me on merging into 1 patch. I'll review tomorrow.

We definitely want the newly introduced labels to be lowercase btw.

pguillard’s picture

+1 !

therealssj’s picture

+1!
Also aren't the labels automatically converted to lowercase later when retrieving their value?

bojanz’s picture

No, they aren't.

rszrama’s picture

Just pinging in from #2702775: Add plural labels to Entity Types in Taxonomy module; my labels are already in the patch. : )

bojanz’s picture

pguillard’s picture

Assigned: Unassigned » pguillard
Status: Needs work » Needs review
FileSize
15.69 KB
2.22 KB

I merged them, though they are not RTBC at the moment.
I hope I understand @bojanz request !

ztl8702’s picture

As a matter of style, I would like to know which one is more acceptable?

+ *   label_count = @PluralTranslation(
+ *     singular = "@count image style",
+ *     plural = "@count image styles",
+ *   ),

or

+ *   label_count = @PluralTranslation(
+ *     singular = "@count image style",
+ *     plural = "@count image styles"
+ *   ),

The difference is the comma after the last item in the array (or the last argument).
There isn't a comma here: https://www.drupal.org/coding-standards/docs#Plugin

ztl8702’s picture

See line 24 here .
I think in most cases we don't add a comma. But there are exceptions like line 19 here.

bojanz’s picture

We want to always have a trailing comma. That's in line with our regular array coding standards.

ztl8702’s picture

rosinegrean’s picture

As @jhodgdon commented here https://www.drupal.org/node/2703103#comment-11083865

merged in the patch for the Tour entity

vg3095’s picture

Added plural labels for System, Views and Node module.

vg3095’s picture

By mistake , uploaded interdiff as a patch file ,the extension should be txt, sorry for that .

xjm’s picture

(Updating issue credit.)

With regard to the trailing commas in annotations, it's our coding standard for normal PHP arrays. However, originally, the Doctrine annotations did not support the final trailing comma. This was eventually fixed, but there are still a lot of the original style in core for that reason. https://www.drupal.org/node/1354#annotations does not specify one way or the other, so I agree it makes sense to be consistent with the PHP array coding standards in absence of that.

Thanks everyone.

Drupa1ish’s picture

pashupathi nath gajawada’s picture

Assigned: Unassigned » pashupathi nath gajawada

I am looking at it.

pashupathi nath gajawada’s picture

Assigned: pashupathi nath gajawada » Unassigned

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

rosinegrean’s picture

Status: Needs review » Closed (duplicate)
Issue tags: -DCTransylvania, -Needs issue summary update
tim.plunkett’s picture

Status: Closed (duplicate) » Active

This is marked as a duplicate of itself, did you mean #2702799: [meta] Add plural labels to core entity types ?

rosinegrean’s picture

Sorry about this.

I wanted to update all issues with DCTransylvania tag. Last year @amateescu created an issue for each entity type as a novice task during the camp and most of them were marked duplicated of this one along the way.

Since next weekend we will be holding a new sprint during the camp, i wanted to have a clean list.

Thanks again for noticing,

rosinegrean’s picture

Assigned: Unassigned » rosinegrean
Issue tags: +DCTransylvania

Let's see if there is still work to be done on this.

rosinegrean’s picture

Status: Active » Needs review
FileSize
17.42 KB

So, i needed to re-roll the patch and update for Comment, CommentType and Editor.
The rest are as they were before.
@tim.plunkett do you think there is something else to do for this one ?
Thanks,

tstoeckler’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/editor/src/Entity/Editor.php
    @@ -14,6 +14,12 @@
    + *     singular = "@count Text Editor",
    + *     plural = "@count Text Editors",
    

    this should be lowercase.

  2. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
    @@ -14,6 +14,12 @@
      *   label = @Translation("Content Language Settings"),
    + *   label_singular = @Translation("content language setting"),
    + *   label_plural = @Translation("content language settings"),
    + *   label_count = @PluralTranslation(
    + *     singular = "@count content language setting",
    + *     plural = "@count content language settings",
    + *   ),
    

    This is tough, because actually each entity is a set of settings already, so I think this needs more thought.

  3. +++ b/core/modules/views/src/Entity/View.php
    @@ -16,6 +16,12 @@
      *   label = @Translation("View", context = "View entity type"),
    + *   label_singular = @Translation("view"),
    + *   label_plural = @Translation("views"),
    + *   label_count = @PluralTranslation(
    + *     singular = "@count view",
    + *     plural = "@count views",
    + *   ),
    

    We should add the context to all new labels, as well.

Also this is not covering the new label_collection yet. Not sure if we should do that here or in a followup.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
17.52 KB
2.27 KB

Addressing #41

About #41.2 - for the plural of content language settings i went with 'content languages settings' - does that make sense?

Status: Needs review » Needs work

The last submitted patch, 42: 2702683-42.patch, failed testing.

amateescu’s picture

The 'label_collection' issue brought up by @tstoeckler made me realize that we should probably fix/finalize the API for defining all these labels, so we don't end up with different solutions for 8.3.x vs. 8.4x, so I opened #2869039: Group all entity labels under a single 'labels' annotation key and I think we should do this one as a follow-up.

Manuel Garcia’s picture

Status: Needs work » Postponed
Related issues: +#2813895: Combine entity type labels in an array

Postponing on #2813895: Combine entity type labels in an array - Though that is far from ready, there's not even a patch there yet...

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Title: Add plural labels to entity types » [PP-1] Add plural labels to entity types

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

bojanz’s picture

Title: [PP-1] Add plural labels to entity types » Add plural labels to entity types
Status: Postponed » Needs work

Looks like #2813895: Combine entity type labels in an array isn't happening.
#2942588: EntityListBuilder::render() should use the entity type plural label in the empty text shows that using better APIs gives us worse UI text because this issue hasn't landed in a year.

So let's proceed. Once [##2813895: Combine entity type labels in an array happens, the labels can be updated, like with any other change.

Manuel Garcia’s picture

Issue tags: +Needs reroll
jofitz’s picture

Assigned: rosinegrean » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.88 KB

Let's start with a re-roll then see where we stand...

(I think we can safely unassign @prics after 11 months)

Status: Needs review » Needs work

The last submitted patch, 51: 2702683-51.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
4.4 KB
21.28 KB

Adjust tests to match new functionality.

Status: Needs review » Needs work

The last submitted patch, 53: 2702683-53.patch, failed testing. View results

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
4.33 KB
24.3 KB

Thanks @Jo Fitzgerald!

I searched for all entities extending ContentEntityBase, and we were missing File and MenuLinkContent, so added support for those as well. I believe now all are covered.

Also worked on the failing tests. Let's see if I broke others...

tstoeckler’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/editor/src/Entity/Editor.php
    @@ -14,6 +14,12 @@
    + *   label_singular = @Translation("Text Editor"),
    + *   label_plural = @Translation("Text Editors"),
    
    +++ b/core/modules/file/src/Entity/File.php
    @@ -18,6 +18,11 @@
    + *   label_plural = @Translation("Files"),
    
    +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
    @@ -19,6 +19,11 @@
    + *   label_plural = @Translation("Custom menu links"),
    

    These should all be lowercase.

  2. +++ b/core/modules/file/src/Entity/File.php
    @@ -18,6 +18,11 @@
      *   label = @Translation("File"),
    
    +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
    @@ -19,6 +19,11 @@
      *   label = @Translation("Custom menu link"),
    

    Missing the singular label.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
2.84 KB
24.95 KB

Thanks @tstoeckler!

Addressing #56 and fixing a failing test.

hchonov’s picture

Status: Needs review » Needs work

There are entity types that are not yet covered in the patch - Workflow, RestResourceConfig, Block, FilterFormat, Vocabulary.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
2.87 KB
27.82 KB

Good call @hchonov, thanks. Added labels to Workflow, RestResourceConfig, Block and FilterFormat. Vocabulary already has them.

tstoeckler’s picture

Looks great to me, thanks!

The only remaining question is whether we want to handle collection labels (i.e. uppercase plural label) here as well. I don't feel strongly either way. On the one hand it shouldn't be that much work, on the other hand I don't want to hold this up any longer.

So I'm fine with this being RTBC directly, but also fine with needs work.

hchonov’s picture

Status: Needs review » Needs work

On the one hand it shouldn't be that much work, on the other hand I don't want to hold this up any longer.

If it isn't that much work, then we will not be holding up this that much longer, so let's do it here.

Manuel Garcia’s picture

Happy to work on that, what exactly do you mean?

tstoeckler’s picture

So we should add an entry similar to the following to all entity types:

 *   label_collection = @Translation("Workflows"),
Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
16.21 KB
29.45 KB

Simple enough, here it is. Also reordered the changes to Editor so its consistent with the rest.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, thank you!

bojanz’s picture

Looks good, +1 for RTBC.

Gogowitsch’s picture

Tested & looks good, thanks everyone! For my project I really need this in core.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: 2702683-64.patch, failed testing. View results

pguillard’s picture

Status: Needs work » Needs review
FileSize
29.42 KB

A a little reroll, one change not applying anymore on 8.6.x
I guess I should not set it back to RTBC.

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
844 bytes

Thank you @pguillard, reroll looks good.
Setting back to RTBC assuming it comes back green :)

alexpott credited Gabr1el.

alexpott credited jhodgdon.

alexpott credited jmikii.

alexpott credited svjuly.

alexpott credited vasike.

alexpott’s picture

Adding commit credit according to #9 and comments on this issue.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +String change in 8.6.0

Committed 1382290 and pushed to 8.6.x. Thanks!

  • alexpott committed 1382290 on 8.6.x
    Issue #2702683 by Manuel Garcia, xjm, prics, pguillard, Jo Fitzgerald,...

Status: Fixed » Closed (fixed)

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

rosinegrean’s picture

Issue tags: -DCTransylvania