Entity type labels lack plurality, cannot generate UI text based on label if plural is needed

It will be helpful also in the generic views integration.

We should probably run this by the D8MI folks first?

Discussed in IRC and #1830822-43: Split the Views UI listing into two tables for enabled and disabled. I'm not actually sure this is a good idea because:

1. The dichotomy of "singular" and "plural" is language-specific. Some languages have no plural. Some have a dual and a plural. Etc. In most cases, we should be using format_plural() instead.
2. It also seems odd to privilege entity type with a special extra label.

@tim.plunkett mentioned that Views handler types might have a special plural format as well; we might want to look into that to see if there's a better way.

So at least, this needs more discussion.

So what we might should do is the following:

• Just add the pure string to the entity info, so
  

    *   label = "Breakpoint",
    *   plural_label = "Breakpoints",
  

• In the actual code which uses these labels we should use t() for the single case and format_plural for the N > 1 case,
  with the known count. If the count is not known we might just apply 3 or something like that? Seems to be pretty difficult to give a context of unknown many.

I'm not sure we should do it at all, TBH.

plural_entity_labels.patch queued for re-testing.

Crossreferencing the Entity API issue: #1664350: Revise the custom label keys.

Pasting what I said into this issue.

Background

Core is pretty limited in the way it does entity type labels. You get one label (Node, Commerce Product, OG membership) and that's it.
If you want to refer to the entity type generically through the UI (permissions, forms, buttons), you need more than that.

So, in parallel Commerce and Entity API tried to address this.

Commerce did this:

function commerce_product_entity_info() {
  $return = array(
    'commerce_product' => array(
      'label' => t('Commerce Product'),
      'permission labels' => array(
        'singular' => t('product'),
        'plural' => t('products'),
      ),

Entity API did this:

function entity_test_entity_info() {
  $return = array(
    'entity_test' => array(
      'label' => t('Test Entity'),
      'plural label' => t('Test Entities'),

The Commerce approach to labels is good, the only problem is that "permission labels" is too narrow, since the labels can have much wider use.
The Entity API approach is not good:
1) The plural label is capitalized by default, which means that for many UI contexts it needs to be lowercased. However, this means it can be semantically incorrect in languages such as German (where nouns need to be capitalized) because the case of the translation is not respected.
A better option is to have it lowercase by default, and uppercased in the few cases where that is needed.
2) It only gives us a plural label, instead of having both singular and plural.
This is important because many contrib entity types have the project name in the label (Commerce Product, OG membership), and the actual UI labels need to be without that prefix (product / products, membership / memberships).

Proposal

Let's standardize both Commerce and Entity API on:

function entity_test_entity_info() {
  $return = array(
    'entity_test' => array(
      'label' => t('Test Entity'),
      'ui labels' => array(
        'singular' => t('entity'),
        'plural' => t('entities'),
      ),

I'd also be fine with "ui strings". And the original plural label key could be kept if it would still have a use somewhere, but I doubt it.

So, applied to D8 core today, it would look something like this:

/**
 * Defines the aggregator item entity class.
 *
 * @EntityType(
 *   id = "aggregator_item",
 *   label = @Translation("Aggregator feed item"),
 *   ui_labels = {
 *     "singular" = @Translation("feed item"),
 *     "plural" = @Translation("feed items"),
 *   }
 *   module = "aggregator",
 *   controllers = {
 *     "storage" = "Drupal\aggregator\ItemStorageController",
 *     "render" = "Drupal\aggregator\ItemRenderController"
 *   },
 *   base_table = "aggregator_item",
 *   fieldable = TRUE,
 *   entity_keys = {
 *     "id" = "iid",
 *     "label" = "title",
 *   }
 * )
 */

Since we have format_plural in core, it makes sense to have labels that are usable there.
From inline entity form to actions ("Deleted $number_of_items $entity_type_in_plural' => "Deleted 10 nodes"), this makes sense in a lot of places.

Perhaps a better title.

Is this a duplicate of #23298: Entity bundles should declare a plural form of their label? And should bundles also have pluralised labels?

It's not really a duplicate, #23298: Entity bundles should declare a plural form of their label deals with *bundle labels*, which are stored in config (and it's pretty much impossible to do with the current config system in D8), while this one deals with entity type labels, which are stored in an annotation, and it is doable in D8. Working on a revised proposal and a patch :)

...we should use t() for the single case and format_plural for the N > 1 case.

Will this suffice for languages like Russian for example that have multiple plural forms (noun quantity that ends in 1 / noun quantities that end in 2,3,4 / noun quantities that end in 5,6,7,8,9,0)?

We should use format_plural() for every case.

Here's how I think this should look like. It's only implemented on the node entity type as a PoC, but should be expanded to all types provided by core if we agree on the direction.

Yes, if there is no count I agree we should not invoke format_plural(). The suggestion that "we should use t() for the single case and format_plural for the N > 1 case" was wrong though. If we do know the number of things, we should call format_plural() at all times.

Fix missing implementation of EntityBCDecorator::entityTypeLabel() in #14.

I believe this is important for UI.
If we really can't avoid using "article content" instead of "articles" (#23298: Entity bundles should declare a plural form of their label), then we should at least avoid "taxonomy term items" and "file items".

What would we do with multiple plural forms though?
If we don't implement a variable number of forms, in these cases we could simply fall back to using " items". That could be left to the translators. It wouldn't be a good solution for all languages but at least for the majority.

What would we do with multiple plural forms though?

That's the whole point of the code above (as opposed to Bojan's proposal), multiple plural forms are handled correctly :)

Last night in IRC he mentioned that we also need a 'generic' plural form for when we don't want to count things, so I added it in this new patch.

Also, thanks for the fix in #17!

Bah!

Added the new @PluralTranslation annotation to existing tests. I think we're ready for some reviews on the approach before adding plural translations for all entity types. Or, even better, this patch could go in as-is and open a followup for the remaining entity types.

While it is great that we have count labels, we also need both singular and plural generic labels, that was the point of my proposal.
This patch adds the generic plural label, but not the generic singular label, so I can't do "Add new $type" in Inline Entity Form for instance.

1. That's true, we'd also need a generic, or indefinite singular.
We might want to define some constants for the first parameter of entityTypeLabel(), instead of the cryptic boolean:
NUMERUS_SINGULAR => indefinite singular
NUMERUS_PLURAL => indefinite plural
NUMERUS_DEFINITE => definite singular or plural depending on the count given in the second argument.
IMHO, we can define these constants in system.module because they are valid beyond language/locale and beyond entity system.

2. In any case these concepts need to be extended even further to really cater for all possible forms.
Apart from the numerus, many languages have regular (and irregular) endings that flect with the case. Translators mostly find a way to work around that, but to solve this generically, we'd need all of those forms. But that's far beyond the scope of D8 now.

Yep, I see now that we need the singular label too. Unfortunately, I'll be away for at least a week so I won't be able to push this further.

@amateescu:
Before you're going: How about the approach proposed in #28.1? Should I give it a try or does something else come in your mind?

I think that's fine, yes :) Although NUMERUS sounds a bit criptic.. we might try to go with something like LABEL_*.

About how the patch should look like, I guess it needs to be a combination of #17 and @bojanz's #8 (I mean separating the count definition for the indefinite one). I started with the count approach only because I'm sure that people would've misused the singular and plural forms for counts..

#31:
I liked numerus because it is the grammatical concept and would at a later point be nicely extendable by CASE_NOMINATIVE etc. But that might be a bit over-engineered for now, andprobably is too much for grammar experts.

So in entityTypeLabel(), we might use:

LABEL_GENERIC = 0 (generic label)
LABEL_SINGULAR = 1 (indefinite singular)
LABEL_PLURAL = 2 (indefinite plural)
LABEL_COUNT = 3 (definite singular/plural depending on the count given)

And the annotation would look like:

 *   label = @Translation("Commerce Product"),
 *   label_singular = @Translation("product"),
 *   label_plural = @Translation("products"),
 *   label_count = @PluralTranslation(
 *     singular = "1 product",
 *     plural = "@count products",
 *   ),

Don't know if we want to rename 'label' to 'label_generic', so it matches the constant in entityTypeLabel()?

#2037763: Set page title in EntityListController::listing() depends on this.

Any thoughts on #32, before I roll it into a patch?

Yeah I think #32 solves the concerns raised in #23298: Entity bundles should declare a plural form of their label as well properly. We do need generic and definite plurals as well if we want to have a complete solution. Not sure if this will be easy on the developer experience, but human language is not easy.

Ok then, let's go with that.

Silly php 5.3 :)

How about this.

+++ b/core/lib/Drupal/Core/Annotation/PluralTranslation.php
@@ -0,0 +1,77 @@
+ * @code
+ *   // Returns: 1 item
+ *   echo $entity->entityTypeLabel(TRUE, 1);
+ *
+ *   // Returns: 5 items
+ *   echo $entity->entityTypeLabel(TRUE, 5);
+ * @endcode

examples are wrong

Thanks for reviewing.

Maybe better provide additional methods?
Lower-cased 'Singular' is confusing me with upper-cased default label

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -15,6 +15,27 @@
+  const ENTITY_TYPE_LABEL_GENERIC = 0;
...
+  const ENTITY_TYPE_LABEL_SINGULAR = 1;
...
+  const ENTITY_TYPE_LABEL_PLURAL = 2;
...
+  const ENTITY_TYPE_LABEL_COUNT = 3;

I think this names too long, suppose 'ENTITY_TYPE_' prefix could be skiped

This is really long:

+ *   echo $entity->entityTypeLabel(\Drupal\Core\Entity\EntityInterface::ENTITY_TYPE_LABEL_COUNT, 1);

These values are just getting passed around in calls, they're never stored in the database. So is there a reason to use integers -- could we use a string like 'plural' instead? That would be easier to read than a namespaced constant.

The 'ENTITY_TYPE_' prefix cannot really be skipped because the constants are on EntityInterface and without the prefix they can be confusing about which label they refer to, entity or entity type..

Re #44:
I was a bit verbose there, it should only be:

+ *   echo $entity->entityTypeLabel(EntityInterface::ENTITY_TYPE_LABEL_COUNT, 1);

entity or entity type

so 'ENTITY_' could be skipped

Well, TYPE_LABEL_COUNT sounds even more confusing..

42: entity_type_count_labels-1850080-42.patch queued for re-testing.

Re-roll.

+++ b/core/lib/Drupal/Core/Annotation/PluralTranslation.php
@@ -0,0 +1,77 @@
+ *   echo $entity->entityTypeLabel(\Drupal\Core\Entity\EntityInterface::ENTITY_TYPE_LABEL_COUNT, 1);

Could someone explain why we have to have these hideously long namespaced constants?

What would be the problem with using strings, like this:

$entity->entityTypeLabel('count', 1);

That was already explained in #45.

Re-roll because of the extension of AnnotationInterface.

> The 'ENTITY_TYPE_' prefix cannot really be skipped because the constants are on EntityInterface and without the prefix they can be confusing about which label they refer to, entity or entity type..

That explains why the constants are so verbose.

It doesn't seem to say why we need constants at all.

Fixed the test failures, and added a PHPUnit test and context support for the @PluralTranslation annotation.

Minor code comment fix.

61: drupal_1850080_61.patch queued for re-testing.

I'm here in Szeged, and I'm going to reroll this.

After the reroll it was a simple automerge.

Here is a fresh reroll.

Triggering testbot.

Reformulate as bug report as per the summary. Needs issue summary update for more detail, suggested resolution, etc. as well as beta evaluation if this is proposed before RC/release :)

@bojanz asked me to look at this. The implementation looks to make sense to me. I would not invoke format_plural() globally but inject / use the string translation service (eg. use the string translation trait). Also you would need a potx module issue to parse the new annotation properly as plural source strings but that is not blocking for core.

Rerolled and fixed the usage of format_plural().

Fixed that.

Fixed the new test class.

+++ b/core/lib/Drupal/Core/Annotation/PluralTranslation.php
@@ -0,0 +1,90 @@
+/**

+++ b/core/modules/system/src/Tests/Plugin/Discovery/AnnotatedClassDiscoveryTest.php
@@ -32,6 +32,11 @@ protected function setUp() {
+            'plural' => '@count loafs',

+++ b/core/modules/system/src/Tests/Plugin/Discovery/CustomDirectoryAnnotatedClassDiscoveryTest.php
@@ -46,6 +46,11 @@ protected function setUp() {
+            'plural' => '@count loafs',

+++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/fruit/Banana.php
@@ -13,7 +13,11 @@
+ *       plural = "@count loafs"

+++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
@@ -49,6 +49,10 @@ protected function setUp() {
+            'plural' => '@count loafs',

s/loafs/loaves? Unless I have been taught wrong.

Good catch :)

I'm wondering if this 8.1.x material now..

Removing from D8MI sprint due to lack of attention.

Yeah this is totally 8.1.x feature to be fair.

I like this patch a lot, nice work! One question I was thinking about is whether we should be doing any fallback for entity types that do not provide this. I.e. just fallback to the label annotation which everyone will have or something like that.

Some more specific notes:

   /**
+   * Identifies the generic entity type label.
+   */
+  const ENTITY_TYPE_LABEL_GENERIC = 0;
+
+  /**
+   * Identifies the indefinite singular entity type label.
+   */
+  const ENTITY_TYPE_LABEL_SINGULAR = 1;
+
+  /**
+   * Identifies the indefinite plural entity type label.
+   */
+  const ENTITY_TYPE_LABEL_PLURAL = 2;
+
+  /**
+   * Identifies the definite singular/plural (depending on the count given)
+   * entity type label.
+   */
+  const ENTITY_TYPE_LABEL_COUNT = 3;

I think these should move to EntityTypeInterface allowing them to be less verbose, i.e. EntityTypeInterface::LABEL_SINGULAR.

A more general question: Why use constants instead of just having separate methods (which could then be on the entity type directly)? I.e. why not have $entity->getEntityType->singularLabel() instead of

$entity->entityTypeLabel(EntityInterface::ENTITY_TYPE_LABEL_SINGULAR). I think that would be more intuitive.

<code>
 abstract class Entity implements EntityInterface {
 
   use RefinableCacheableDependencyTrait;
+  use StringTranslationTrait;

Seems unused.

+  public function entityTypeLabel($label_form = self::ENTITY_TYPE_LABEL_GENERIC, $count = NULL) {

If we're going to leave this, it should be called getEntityTypeLabel().

+ *   label_count = @PluralTranslation(
+ *     singular = "1 content item",
+ *     plural = "@count content items"
+ *   ),

It is done this way all over core, but it is in fact better to use @count in the singular version as well, for languages where the singular formula is more complex than @count === 1.

All in all warrants a needs work I guess.

This mostly addresses some points of the review of @tstoeckler

In general it would be really nice to have this done.

If we're going to leave this, it should be called getEntityTypeLabel().

Well, this is not necessarily a getter, it actually calculates something.

The new code feels misplaced: I would expect the EntityType to have getters for these labels, not the Entity.
I see tim.plunkett had the same comment in #62, but it wasn't addressed.

(tstoeckler in #86 also wanted the constants moved to EntityTypeInterface, which would match the above, not touching Entity at all)

Nitpicks of the current patch:

+ * @code
+ *   label_count = @ PluralTranslation(
+ *     singular = "1 item",
+ *     plural = "@count items",
+ *     context = "cart_items",
+ *   ),
+ * @endcode
+ * Remove spaces after @ in your actual plugin - these are put into this sample
+ * code so that it is not recognized as annotation.
+ *

Should this singular and the Banana one be also updated to use @count? If I saw 1 in some places and @count in others I wouldn't be sure what the best practise is.

+ * Code samples that make use of this annotation class and the definition sample
+ * above:
+ * @code
+ *   // Returns: 1 item
+ *   echo $entity->entityTypeLabel(\Drupal\Core\Entity\EntityInterface::ENTITY_TYPE_LABEL_COUNT, 1);
+ *
+ *   // Returns: 5 items
+ *   echo $entity->entityTypeLabel(\Drupal\Core\Entity\EntityInterface::ENTITY_TYPE_LABEL_COUNT, 5);
+ * @endcode

The constants weren't updated here.

Rerolling

Here's what I had in mind.

Changes:
- Moved the constants to EntityTypeInterface
- Moved the method to EntityType (but didn't rename it for now)
- Cleaned up all references to the constants, usage of array() vs [], PluralTranslation
- Made @count used in all singular labels instead of hardcoding 1.

entityTypeLabels() needs a new name. Any suggestions? getPluralLabel() I guess?

We'll also need EntityType test coverage, and updated example code for entities all over the place..

1. +++ b/core/lib/Drupal/Core/Annotation/PluralTranslation.php
   @@ -0,0 +1,95 @@
   + *   // Returns: 1 item
   + *   echo $entity_type->entityTypeLabel(\Drupal\Core\Entity\EntityTypeInterface::LABEL_COUNT, 1);
   + *
   ...
   + *   echo $entity_type->entityTypeLabel(\Drupal\Core\Entity\EntityTypeInterface::LABEL_COUNT, 5);
   

   Let's skip the echo bit, IMHO its not needed

2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -723,6 +761,26 @@ public function getLowercaseLabel() {
   +  public function entityTypeLabel($label_form = self::LABEL_GENERIC, $count = NULL) {
   

   Here is a question, why do we not have 4 different methods, one for each type? This would make the naming much easier.

Needs work based upon #92

I can live with the separate methods. Any second opinions?

We'll want to update this whole piece then, and most likely remove the constants completely:

diff --git a/core/lib/Drupal/Core/Annotation/PluralTranslation.php b/core/lib/Drupal/Core/Annotation/PluralTranslation.php
index b9420c0..47074c4 100644
--- a/core/lib/Drupal/Core/Annotation/PluralTranslation.php
+++ b/core/lib/Drupal/Core/Annotation/PluralTranslation.php
@@ -29,10 +29,10 @@
  * above:
  * @code
  *   // Returns: 1 item
- *   echo $entity_type->entityTypeLabel(\Drupal\Core\Entity\EntityTypeInterface::LABEL_COUNT, 1);
+ *   $entity_type->entityTypeLabel(\Drupal\Core\Entity\EntityTypeInterface::LABEL_COUNT, 1);
  *
  *   // Returns: 5 items
- *   echo $entity_type->entityTypeLabel(\Drupal\Core\Entity\EntityTypeInterface::LABEL_COUNT, 5);
+ *   $entity_type->entityTypeLabel(\Drupal\Core\Entity\EntityTypeInterface::LABEL_COUNT, 5);
  * @endcode
  *
  * @Annotation

I totally like the separate methods. I already suggested something along those lines (albeit a little bit shyly (wow, that's a cool word)) in #86.

shyly

Agreed

Plan for BC:

+    // Ensure defaults for the different label types.
+    if (empty($this->label_singular)) {
+      $this->label_singular = $this->getLabel();
+    }
+    if (empty($this->label_plural)) {
+      $this->label_plural = new TranslatableMarkup('Multiple @label', ['@label' => $this->getLabel()]);
+    }
+    if (empty($this->label_count)) {
+      $this->label_count = [
+        'singular' => new TranslatableMarkup('@count @label', ['@label' => $this->getLabel()]),
+        'plural' => new TranslatableMarkup('@count @label', ['@label' => $this->getLabel()]),
+      ];
+    }

Initial discussion:

bojanz> catch: hard to qualify breakage. I mean, here we're adding 3 new methods to EntityTypeInterface
bojanz> so if a potential martian implements its own EntityType without extending a base class, there's breakage
catch> bojanz: right but 1. is anyone doing that 2. does that individual person mind the breakage 3. do they find out before a beta/rc release so we can mitigate.
catch> bojanz: 4. do we say 'if there's a base class and you're not extending it then tough luck anyway'
bojanz> catch: considering how many assumptions config and content entities have, I doubt many people are doing it (defining a third kind)

Yes, this is technically a BC break. Potentially impacting half a theoretical person.

Right, EntityType is a value object, the base interface maybe was a bad decision in the first place.

Here is the interdiff

Right, EntityType is a value object, the base interface maybe was a bad decision in the first place.

Actually I think having an interface is fine, the poblem is that we have not really declared in most places what we consider our API. Just because we have an interface doesn't mean we consider it part of the API.

As @tim.plunkett is eluding to in #100 we have marked some places in core that are explicitly not part of the API as @internal, but that's not very widespread, and we have changed classes before where we have assumed people will not reasonably subclass them even though were not marked as @internal.

So really suggesting anything either way, we've just but ourselves in a less than optimal situation... :-/

Added a really related discussion.

Updated the default labels to actually be usable.

Ready for RTBC from my side.

Btw, update from catch on the BC issue: https://www.drupal.org/node/2550249#comment-10870382
I am of course inclined to agree.

And the interdiff.

I really like how this looks like now

In #2507235: EntityType::getLowercaseLabel() breaks translation, it was pointed out that the proposed solution there would get really complicated in combination with this. Looking at the patch, it seems like we only use lowercase labels for this now as we expect it will only be used as part of sentences. Which is fine I think and means that we do *not* need special lowercase versions for this in the other issue.

I'll update there as well.

As it turns out, we are not allowed to pass translatable markup objects to PluralTranslatableMarkup

Note: I also changed the code to do less on every initialization.

Nothing but minor changes

1) For example Translation::__construct which also extends AnnonationBase restricts the input parameter to be "array".
2) Moved the comments so the first line is on is own. in a minor thing pointed out by phpcs
3) From https://www.drupal.org/coding-standards/docs#Plugin @Annonation should be at the end of the comment section.

Thank you @martin107
Those changes are looking great!

@dawehner
Can you provide an interdiff for #112?

martin107's #113 doesn't include your changes, we need to reapply them.

Ah crap patch clash I posted my changes 3 mins after dawehner's and did not check for updates

... my changes are also based on #107 therefore ignoring the changes from #112 .. I will sort this out tonight...my bad.

OH yeah sorry

#116 is another comment clash it was written with outsight of #115

Who knew that giving my lucky charm to Wile E Coyote was going to turn out like this!

https://www.youtube.com/watch?v=i6e-KrNCe_E&list=PLL3uNn7f3npgkXGEU71mZS...

If I step outside tonight, I should look up as there might just be a grand piano about to fall on my head...

What follows is a merge of the last 2 changes with and interdiff from #107

This comment is sponsored by the acme corporation.

Okay, we're definitely good to go.

Um.... Does this work with translation, really? I mean, is the POTX extractor getting these? I doubt it.

@jhodgdon Currently it doesn't this is for sure, but I don't see why it should not be able to parse that as well, much like it parses @Translation at the moment.

Well the problem is that it is not going to store it like a normal Plural translation string. This is not going to work.

Plural strings are stored in the translation database as something like this:
single_formWEIRD_DELIMITERplural_form

This will store single_form and plural_form as separate strings. So when someone calls formatPlural() or makes a PluralTranslatableMarkup or whatever, it will not get translated properly. The test that you have in here didn't detect this because it doesn't actually try to translate to a different language.

This patch needs some work.

I mean @jhodgdon what else should we be able to do? This calls out to format_plural at the end of the day, which is kinda what we want, don't we?

Really, the entire premise of this issue is flawed. This whole thing will only work for some languages. English, sort of. Spanish, sort of. French, sort of. Russian, not at all.

So. Let me explain.

There is a reason that we translate entire phrases at once. For instance, we might translate:

1 content item has been updated. / @count content items have been updated.

Note: these are plural/singular forms -- but not only the noun "content item" changes, but also the verb. In other languages, also adjectives might need to be changed to agree with the noun, or even the entire sentence order might need to change between singular and plural, and in some languages there are more than 2 plural forms for verbs, and in some there is only one.

The proposal here is just to have the strings "content item" and "content items", as the singular and plural forms, translate into other languages. So this would be OK for French or Spanish, aside from the verbs and adjectives, but not at all OK for Russian, which besides having 4 plural forms (which are not going to be translated right because this is not a proper formatPlural), also have different forms for nouns based on whether they're subjects or objects of verbs or objects of prepositions.

The other problem is formatPlural(). You MUST call formatPlural by passing in the literal strings that are to be translated, because that is how they are picked up by the POTX generator. They are translated as a group on localize.drupal.org into different languages, so that the 2 source strings in English become 4 translations in Russian. This is not going to happen with the code in this patch. They are going to be stored as 2 strings and translated as 2 separate strings, and like I said, in Russian the loner strings like "content items" cannot even be translated without knowing that they're going to be subjects or objects or whatever.

So this whole idea is flawed. Whatever the purpose was, it will not work right for anything but English.

See #2545730: Misuse of formatPlural() in Numeric field prefix/suffix for more discussion on how to misuse formatPlural() in a very similar way on another issue that still needs to be resolved. But let's not introduce another misuse here please.

@jhodgdon So let's drop support for format plural as part of the issue and just handle the rest of it?

What else is there? This patch adds methods getSingularLabel(), getPluralLabel(), and getCountLabel(). To me they are all suspect.

Apparently Gabor signed off on this so I'm setting this back to RTBC. I still think the formatPlural stuff at a minimum is wrong. But whatever.

I'm feeling bad how this went :(

As I said on IRC, I'm with with pinging Gabor and other experts again, getting a second opinion.

As a speaker of a Slavic language (Serbian) I confirmed that it's fine to have generic singular / plural labels, and then the various count-specific forms. The problems start when the labels are embedded into a sentence (which brings noun cases into play), but that's a problem with every label in Drupal, including the current singular-only entity type labels. We are not making this problem worse. In fact, we're making translations better, because I'd very much rather see $plural_translated_to_serbian than $singular_translated_to_serbian . 's', etc.'

So first things first the generic singular and generic plural should be fine regardless of language, I would expect translators know that concept and would translate accordingly when seeing a plural/singular without a @count :) Looking at the concrete singular/plural stuff again now.

All right, looking at the definite plural/singular cases, I don't see the problem. Once potx is capable of extracting the three strings (singular/plural/context) from its annotation, the fact that formatPlural() is called on them with those original source strings should both match up the translations properly and apply the plural rules of the language currently in use for the request. Any caching or bubbling that data without consideration for language may be a problem, but otherwise it looks like it considers the interface language and uses proper source values to me. Please correct me if I am missing something.

On review found one doc problem and a bigger problem with how getCountLabel's fallback code and lack of consideration for string context is shaping up. So those need fixing (and additional tests for context support). Leaving at needs work for these.

1. +++ b/core/lib/Drupal/Core/Annotation/PluralTranslation.php
   @@ -0,0 +1,98 @@
   + * passed to format_plural().
   

   format_plural() as a global function does not exist.

2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -723,6 +746,42 @@ public function getLowercaseLabel() {
   +  public function getCountLabel($count) {
   +    if (empty($this->label_count)) {
   +      $lowercase_label = $this->getLowercaseLabel();
   +      $this->label_count = [
   +        'singular' => str_replace('@label', $lowercase_label, '@count @label'),
   +        'plural' => str_replace('@label', $lowercase_label, '@count @label entities'),
   +      ];
   +    }
   +    return $this->formatPlural($count, $this->label_count['singular'], $this->label_count['plural']);
   +  }
   

   This would of course not be able to be pretranslated. This is fallback code. A slightly better way to fix this is to do the @label replacement after the format plural, so the string translated is static and is not dynamic.

   Also to have an inline formatPlural() with the two hardcoded strings so potx finds it properly. This way the source strings will not be found.

   (Also this fallback probably needs context, since '@count @label' will not be easy to understand for translators).

   Also wrong that label_count's context is never passed into format plural, that should be. Otherwise what does it help? This apparently does not have test coverage either or it would have failed.

Additionally @dawehner points out that the 2 snippet I posted is also incorrect because getLowercaseLabel is already a translation object, so replacing the translation into a string and then format plural translating the translation would double translate it. One more reason to do the replacement in formatPlural()'s args.

If I understood #135 correctly, something like this should do it?

I'm glad we have this kind of test coverage

Gabor's feedback has been addressed. Third time's the charm?

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -723,6 +746,39 @@ public function getLowercaseLabel() {
+    if (empty($this->label_count)) {

Should this be if (empty($this->label_count['singular']) || empty($this->label_count['plural'])) { - or should we check this below and throw an exception if the keys are present and it's not empty?

Should this be if (empty($this->label_count['singular']) || empty($this->label_count['plural'])) { - or should we check this below and throw an exception if the keys are present and it's not empty?

I think the annotation class should do some kind of validation ...

There we go.

Nothing major - Just describing what we throw in the annotations.

   public function getCountLabel($count) {
     $context = isset($this->label_count['context']) ? $this->label_count['context'] : 'Entity type label';
-    if (empty($this->label_count)) {
+    if (empty($this->label_count['singular']) || empty($this->label_count['plural'])) {
       return $this->formatPlural($count, '@count @label', '@count @label entities', ['@label' => $this->getLowercaseLabel()], ['context' => $context]);
     }

Didn't we just ensure that label_count can be empty, but the individual keys can't? In which case this change is not needed?

Confirmed #147 with dawehner. His exceptions in PluralTranslation ensure that the singular and plural keys are always set. The point of the check in getCountLabel() is to provide a default when the entire count label is missing. Made the change to reflect that.

Thank you @bojanz

@tim.plunkett
Probably no reason, though note that the PHPUnit maintainer no longer recommends using the annotation: https://thephp.cc/news/2016/02/questioning-phpunit-best-practices

Given the additions to EntityTypeInterface I think we have to wait for #2661926: [policy no-patch] Document that interfaces without an @api tag can have API additions during a minor release to become fixed before committing this. Also if this is committed to 8.1.x during beta it should have CR and be mentioned in the release notes.

Here is one: https://www.drupal.org/node/2689949

#2661926: [policy no-patch] Document that interfaces without an @api tag can have API additions during a minor release has landed, and we have a change record now (made some tweaks too).

Saving issue credit.

Last time I reviewed this patch I get stuck on the fact that EntityType::getLowerCaseLabel() is not going to work for languages like German... but that is not the fault of this issue - that's #2507235: EntityType::getLowercaseLabel() breaks translation

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -186,6 +187,29 @@ class EntityType implements EntityTypeInterface {
   /**
+   * The indefinite singular name of the type.
+   *
+   * @var string
+   */
+  protected $label_singular = '';
+
+  /**
+   * The indefinite plural name of the type.
+   *
+   * @var string
+   */
+  protected $label_plural = '';
+
+  /**
+   * A definite singular/plural name of the type.
+   *
+   * Needed keys: "singular" and "plural".
+   *
+   * @var string[]
+   */
+  protected $label_count = [];

+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -631,6 +631,33 @@ public function getLabel();
   /**
+   * Gets the singular label of the entity type.
+   *
+   * @return string
+   *   The singular label.
+   */
+  public function getSingularLabel();
+
+  /**
+   * Gets the plural label of the entity type.
+   *
+   * @return string
+   *   The plural label.
+   */
+  public function getPluralLabel();
+
+  /**
+   * Gets the count label of the entity type
+   *
+   * @param int $count
+   *   The item count to display if the plural form was requested.
+   *
+   * @return string
+   *   The count label.
+   */
+  public function getCountLabel($count);

I think we need to improve the documentation because atm we now have 4 labels in the annotation and 4 methods and I'm confused about when to use getSingularLabel() over label(). Working out how Gets the human-readable name of the entity type. is different Gets the singular label of the the entity type. is not so simple.

I'm sorry it has taken a while to review this patch I've looked at it many times over the past week and discussed with @catch and @xjm. The general feeling is that this patch in its present state can land during the 8.1.x beta but none of us are "yay" this look fantastic - #35 sums this up nicely

but human language is not easy.

And given that we are adding quite a few new methods which are similar to an existing method I think we need to make it super obvious which method to use.

I hope this improves the documentation a bit.

Discussed with @xjm, @effulgentsia and @cottser. We decided that the docs changes need to be improved but we'd like this patch to be in the 8.1.0-rc1 release. So we should go back to the patch in #148 and commit that and file a docs followup.

Pushed #159 to 8.2.x and 8.1.x. The confusing sequence of #161 and #162 is due to me having initially cherry picked into 8.1.x without the -x option, so I reverted, and then repushed with that option. Sorry for that.

Needs work for the docs follow-up per #159.

Prefixing title to clarify that the main patch here has landed.

Created #2701005: Improve documentation of EntityTypeInterface label functions for the followup.

Follow-up: #2767025: Add entity type label for a collection of entities