[PP-1] Allow plurals on bundle labels

I don't think the proposed resolution works because bundles are user created and may therefore be in any language (unlike entity types defined in code). So there may be a number of plural variants. See Drupal\views\Plugin\views\field\NumericField for example, especially:

    $form['format_plural'] = array(
      '#type' => 'checkbox',
      '#title' => $this->t('Format plural'),
      '#description' => $this->t('If checked, special handling will be used for plurality.'),
      '#default_value' => $this->options['format_plural'],
    );
    $form['format_plural_string'] = array(
      '#type' => 'value',
      '#default_value' => $this->options['format_plural_string'],
    );

    $plural_array = explode(LOCALE_PLURAL_DELIMITER, $this->options['format_plural_string']);
    $plurals = $this->getNumberOfPlurals($this->view->storage->get('langcode'));
    for ($i = 0; $i < $plurals; $i++) {
      $form['format_plural_values'][$i] = array(
        '#type' => 'textfield',
        // @todo Should use better labels https://www.drupal.org/node/2499639
        '#title' => ($i == 0 ? $this->t('Singular form') : $this->formatPlural($i, 'First plural form', '@count. plural form')),
        '#default_value' => isset($plural_array[$i]) ? $plural_array[$i] : '',
        '#description' => $this->t('Text to use for this variant, @count will be replaced with the value.'),
        '#states' => array(
          'visible' => array(
            ':input[name="options[format_plural]"]' => array('checked' => TRUE),
          ),
        ),
      );
    }
    if ($plurals == 2) {
      // Simplify interface text for the most common case.
      $form['format_plural_values'][0]['#description'] = $this->t('Text to use for the singular form, @count will be replaced with the value.');
      $form['format_plural_values'][1]['#title'] = $this->t('Plural form');
      $form['format_plural_values'][1]['#description'] = $this->t('Text to use for the plural form, @count will be replaced with the value.');
    }

That is, #5 for the counted singular/plural case. The general singular/plural case that is above is simpler and would the same I think regardless of language of the bundle configuration.

Right, sorry the screenshot mislead me by showing the single plural case, which is all too common ;) Looking at the implementation it would be nice to harmonize the form elements and descriptions with the ones in views, config translation, etc. (same patterns used there). Other than that there are two main problems that I found while reviewing and two not so big ones:

1. +++ b/core/config/schema/core.entity.schema.yml
   @@ -356,3 +356,19 @@ field.formatter.settings.entity_reference_label:
   +bundle:
   

   Minor: This would be nicer to call entity_bundle or somesuch for clarity?

2. +++ b/core/config/schema/core.entity.schema.yml
   @@ -356,3 +356,19 @@ field.formatter.settings.entity_reference_label:
   +    label_count:
   +      type: sequence
   +      label: 'Count label'
   +      sequence:
   +        type: string
   +        label: 'Plural variant'
   

   Config translation stores and manages translations as overrides (think deep array merge). Therefore storing the count labels as a sequence does not work because if the original was English and the translation was Polish (more plural forms), there would be new keys appearing in the config, which is not supposed to happen with overrides. The config is supposed to have the same keys regardless of overrides.

   Also this way the plural variants are not translatable (strings are not, labels are). And if there would be 2 English source strings, you could only have 2 Polish translations. See and use the plural_label config schema type.

3. +++ b/core/modules/node/src/NodeTypeForm.php
   @@ -182,6 +183,66 @@ public function form(array $form, FormStateInterface $form_state) {
   +      '#default_value' => $type->getSingularLabel(),
   ...
   +      '#default_value' => $type->getPluralLabel(),
   ...
   +        $default_value = $i == 0 ? $this->t('1 @label_singular', $arguments) : $this->t('@count @label_plural', $arguments);
   

   Should the default value not be whatever was stored? :)

4. +++ b/core/modules/node/src/NodeTypeForm.php
   @@ -182,6 +183,66 @@ public function form(array $form, FormStateInterface $form_state) {
   +    if (\Drupal::hasService('locale.plural.formula')) {
   +      $langcode = \Drupal::languageManager()->getCurrentLanguage()->getId();
   +      $plurals = \Drupal::service('locale.plural.formula')->getNumberOfPlurals($langcode);
   +    }
   

   This assumes the language of the config is the language of the current UI interface language, however there is no relation between the two. You may be editing English configuration in Polish and vice versa. The right number of plural forms based on the *language of the configuration itself* should be exposed. See again the views formatter code above.

5. +++ b/core/modules/node/src/NodeTypeForm.php
   @@ -182,6 +183,66 @@ public function form(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t('Token <code>@count

   is available.'),
   ...
   + $title = $this->t('Singular');
   ...
   + $title = $this->t('Plural');
   ...
   + $title = $this->t('Plural variant #@variant', ['@variant' => $i]);
   

   Use labels in sync with views formatter, config translation, etc.

+++ b/core/config/schema/core.entity.schema.yml
@@ -370,5 +370,5 @@ bundle:
       type: sequence
       label: 'Count label'
       sequence:
-        type: string
+        type: label
         label: 'Plural variant'

This does not solve the problems I outlined. When the original config is in English, you'll have two items in this sequence and therefore the Polish translation can only have two items, even though it needs 3. A translation of one label can only be another one label. If your original configuration is Polish, your English translation would need to have translations for each 3 variants from Polish, even though that is nonsensical for English. Using plural_label, config translation understands the variance in element numbers required AND uses the exact same keys regardless of language.

Using a sequence does not work for multilingual use cases, unless you also reimplement plural handling with sequences too both in config_translation and language module's config override handler. That would mean a dependent issue that blocks this one, and I don't think that is a good idea anyway, since then we'd have two different ways to represent and translate plurals in configuration.

Thanks for the update!

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -11,7 +15,26 @@
   +   * A definite singular/plural count label.
   

   It should say these are separated with PluralTranslatableMarkup::DELIMITER

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -66,6 +89,52 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   +      if ($this->isNew()) {
   +        return '';
   +      }
   +      $this->label_singular = Unicode::strtolower($this->label());
   ...
   +      // Provide a fallback in case label_plural is not set yet.
   +      if ($this->isNew()) {
   +        return '';
   +      }
   

   It may still already have a label even if not yet saved, no?

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -112,4 +181,31 @@ protected function loadDisplays($entity_type_id) {
   +   * @todo Inspired by PluralTranslatableMarkup::getPluralIndex(). Move it to a
   +   *   utility class or make it public static on PluralTranslatableMarkup.
   

   We don't place @todo in core without an issue number opened for it :)

4. +++ b/core/modules/node/content_types.js
   @@ -54,6 +54,19 @@
   +        var singularLabel = $(context).find('#edit-label-singular').val();
   +        vals.push(singularLabel ? Drupal.t("Singular '@label'", {'@label': singularLabel}) : Drupal.t('Requires a singular label'));
   +        var pluralLabel = $(context).find('#edit-label-plural').val();
   +        vals.push(pluralLabel ? Drupal.t("Plural '@label'", {'@label': pluralLabel}) : Drupal.t('Requires a plural label'));
   +        var hasAllCountLabels = true;
   +        $('#edit-label-count input[type="text"]', context).each(function () {
   +          if (!$(this).val()) {
   +            hasAllCountLabels = false;
   +            return false;
   +          }
   +        });
   +        vals.push(hasAllCountLabels ? Drupal.t("Count label variants defined") : Drupal.t('Requires count label variants'));
   

   The elements are not required in the form, so it looks odd to say in the summary that they are.

5. +++ b/core/modules/node/src/NodeTypeForm.php
   @@ -182,6 +183,40 @@ public function form(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t('Count labels are used to build a text representation of a certain number of @plural. Token <code>@count

   is available and will be replaced with the number of @plural.', $arguments),
   

   Do we say "token" elsewhere like this? I don't think so.

Looks good to me now. Sounds like this would be down to the usability review.

I am completely lost why we would want to expose this in the UI. It seems like a very specific thing, cant this be done in code?

@Bojhan: in other words because you create node types on the UI and not in code. When you create a "Fish" content type, that needs plural forms and depending on language, it may have different plurals. Eg. in English the plural would be "Fish" as well :) Not so for Article -> Articles or Mouse -> Mice as in the screenshots. For more interesting languages, eg. Polish or Romanian there are even more plural variants. Then when we display counters, pagers, etc. we print "@count fish", "@count articles", "@count mice" etc, we need to use the right forms if we want to make sense in the given human language.

I understand the why :) It just makes for a lot of clutter. Perhaps we can avoid having it in the vertical tab? Its very non-critical information.

Yup, the label.

Gábor approved this change in #23, and Bohjan performed the Usability review on #28, #30 and #36. I did a cursor review of the patch and looks good, so let's RTBC it.

Tagging as beta deadline for 8.2.x (it could go into 8.3.x if it doesn't make it in time). Also nice one getting this RTBC, feels like only a couple of days since we even talked about it.

+++ b/core/modules/node/src/NodeTypeForm.php
@@ -244,8 +244,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
-    $label_count = array_filter($form_state->getValue('label_count'));
-    $form_state->setValue('label_count', implode(PluralTranslatableMarkup::DELIMITER, $label_count));
+    $form_state->setValue('label_count', implode(PluralTranslatableMarkup::DELIMITER, $form_state->getValue('label_count')));

How come this change is not accompanied by a change in the tests?

Remarks at #43 have been covered.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
   @@ -112,4 +181,31 @@ protected function loadDisplays($entity_type_id) {
   +    // @todo Refactor in https://www.drupal.org/node/2660338 so this code does
   +    // not depend on knowing that the Locale module exists.
   

   Nit: subsequent lines of an @todo should be indented two extra spaces

2. +++ b/core/modules/node/src/NodeTypeForm.php
   @@ -254,4 +292,64 @@ public function save(array $form, FormStateInterface $form_state) {
   +  protected function getPluralVariantLabel($plurals, $delta) {
   ...
   +    return $this->formatPlural($delta, 'First plural form', '@count. plural form');
   ...
   +  protected function getPluralVariantDefaultValue($delta) {
   ...
   +    return $delta == 0 ? $this->t('1 @singular', $arguments) : $this->t('@count @plural', $arguments);
   

   Why does one use formatPlural and the other doesn't?

3. +++ b/core/modules/node/src/NodeTypeForm.php
   @@ -254,4 +292,64 @@ public function save(array $form, FormStateInterface $form_state) {
   +    /** @var \Drupal\node\Entity\NodeType $type */
   

   \Drupal\node\NodeTypeInterface

4. +++ b/core/modules/node/tests/src/Functional/NodeTypePluralLabelsTranslationTest.php
   @@ -0,0 +1,138 @@
   +    /** @var \Drupal\node\Entity\NodeType $node_type */
   ...
   +    /** @var \Drupal\node\Entity\NodeType $node_type */
   

   Also should use the interface

5. +++ b/core/modules/system/system.module
   @@ -1416,6 +1417,33 @@ function system_entity_type_build(array &$entity_types) {
   +  foreach ($entity_types as $entity_type_id => &$entity_type) {
   

   Shouldn't need the & on &$entity_types, remove please

6. +++ b/core/profiles/standard/config/install/node.type.page.yml
   @@ -8,3 +8,6 @@ help: ''
   +label_count: "1 page\x03@count pages"
   

   is the \x03 really how it's saved? AFAIK i've seen other yaml with multiline strings with regular newlines

This is very close, but at least that & should be removed.

6: well it's equivalent to CTRL-C, but yes I got it confused with \x0A. The docs on PluralTranslatableMarkup::DELIMITER are helpful, thanks.

+1 for RTBC

I've just realised we are missing a few things:

1. An upgrade path for existing sites... what do we do with existing custom items? And upgrading the content types provided by standard?
2. Also we change ConfigEntityBundleBase - which gives these methods to lots of things but only node type is getting the form. I think we should only be changing \Drupal\node\Entity\NodeType. The new methods can go into a trait and the NodeType class can implement the interface.
3. If you create a new content type you get default values for the singular and plural labels but you don't get them for the count labels, see screenshot - why?
   

(Removing release notes tag; we can retag for whichever release notes whenever the change is committed.)

Did a quick review, found only a few nitpicks. I agree with #53 that it's better to have separate traits for entities and entity types - if we have to override all 3 methods in the trait then it's not a good abstraction.

1. +++ b/core/lib/Drupal/Core/Config/Entity/EntityBundleWithPluralLabelsTrait.php
   @@ -0,0 +1,106 @@
   +  /**
   +   * The indefinite singular name of the bundle.
   +   */
   +  protected $label_singular;
   

   Missing type hint @var string.

2. +++ b/core/lib/Drupal/Core/Config/Entity/EntityBundleWithPluralLabelsTrait.php
   @@ -0,0 +1,106 @@
   +   * Plural variants are separated by EXT (PluralTranslatableMarkup::DELIMITER).
   

   This is actually ETX ("End of TExt"), not EXT.

3. +++ b/core/modules/book/config/install/node.type.book.yml
   @@ -11,3 +11,6 @@ help: ''
   +label_singular: 'book page'
   +label_plural: 'book pages'
   +label_count: "1 book page\x03@count book pages"
   

   Just out of curiosity, is there a reason the first two values are using single quotes and the last one is using double quotes?

4. +++ b/core/modules/node/tests/src/Functional/NodeTypePluralLabelsTranslationTest.php
   @@ -0,0 +1,136 @@
   +    $plural = $po_header->parsePluralForms('nplurals=3; plural=((n==1)?(0):(((n==0)||(((n%100)>0)&&((n%100)<20)))?(1):2));\n');
   

   Oh wow :D

   I'm not even pretending to know what is going on here :D

5. +++ b/core/modules/node/tests/src/Functional/NodeTypePluralLabelsTranslationTest.php
   @@ -0,0 +1,136 @@
   +    // Check that 3 plural variants are exposed for the Romanian language.
   +    $this->assertSession()->fieldExists('label_count[0]');
   +    $this->assertSession()->fieldExists('label_count[1]');
   +    $this->assertSession()->fieldExists('label_count[2]');
   

   This is really interesting, that languages can have multiple plural forms. I guess some forms are more appropriate in specific contexts like labels on submit buttons or table column headers? Or whether it is mentioned on its own, or part of a running text?

Updating status and removing outdated tag.

What bugs me is that both this and #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed assume that entity type labels should be lowercased. This is simply incorrect for certain languages. For example, in English an image medium would be labeled as "image"/"images", while in German it's "Bild"/"Bilder".

 public function getSingularLabel() {
  // Provide a fallback in case label_singular is not set yet.
  if (empty($this->label_singular)) {
    if ($label = $this->label()) {
      $this->label_singular = Unicode::strtolower($label);
    }
  }
  return $this->label_singular;
}

Can we please not do this? Re-introducing the capitalization on a per use case basis is a PITA and simply cannot create correct results. Why not assume that the label is capitalized correctly for the given language, as seen in the screenshot examples in the summary?

1. +++ b/core/lib/Drupal/Core/Config/Entity/EntityBundleWithPluralLabelsTrait.php
   @@ -0,0 +1,112 @@
   +    if (($index = static::getPluralIndex($count)) === -1) {
   +      // If the index cannot be computed, fallback to a single plural variant.
   +      $index = $count > 1 ? 1 : 0;
   +    }
   

   It would be easier to read if the $index variable is defined on a separate line.

   Also the "fallback to a single plural variant" is a bit confusing, but I don't know how to explain it better.

2. +++ b/core/modules/node/src/Entity/NodeType.php
   @@ -41,10 +43,15 @@
   -class NodeType extends ConfigEntityBundleBase implements NodeTypeInterface {
   +class NodeType extends ConfigEntityBundleBase implements NodeTypeInterface, EntityBundleWithPluralLabelsInterface {
   

   This implements EntityBundleWithPluralLabelsInterface should be moved to NodeTypeInterface.

3. +++ b/core/modules/node/src/NodeTypeForm.php
   @@ -184,6 +185,39 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form['display']['plural']['label_singular'] = [
   +      '#type' => 'textfield',
   +      '#title' => $this->t('Singular label'),
   +      '#description' => $this->t("Enter a lowercase label for the singular case. Examples: 'article', 'page'."),
   +      '#default_value' => $type->getSingularLabel(),
   +    ];
   

   When I was testing this patch in my project I had some trouble because the #default_value is being set here to $type->getSingularLabel().

   These methods that return the singular/plural/count labels fall back to the name of the bundle if these values are not populated yet. But setting these fallbacks as a #default_value is not a good idea, because then if the form is saved these values are stored in the database.

   This happened to me when I edited a pre-existing node type, and I just edited the node type description and saved the form. This caused the fallback values to be written, even though I hadn't even opened the "Display settings" vertical tab.

   A good solution would be to show the labels as a #placeholder, and use the actually stored data (without fallback value) for #default_value.

These are some remarks for brainstorming.

1. +++ b/core/config/schema/core.entity.schema.yml
   @@ -362,3 +362,20 @@ field.formatter.settings.entity_reference_label:
   +    label_singular:
   

   Why do we need a singular label? I can imagine that if you define the "Article" to have a singular label of "Basic Page", then in some cases the "Basic Page" will show and in some cases, the label "Article" will show. Even if this is allowed, then the title of the entity bundle will only be used as "Administrative title" so the whole field name should change.

   Generally, I think this will create some confusion as it is kind of not needed extra information.

2. +++ b/core/config/schema/core.entity.schema.yml
   @@ -362,3 +362,20 @@ field.formatter.settings.entity_reference_label:
   +        type: plural_label
   

   This 'plural_label' is the opposite of 'label_plural' above and while this is a sequence, it can still confuse. maybe something like count_plural, or label_count_plural (which seems like a child of the label_count above).

3. +++ b/core/lib/Drupal/Core/Config/Entity/EntityBundleWithPluralLabelsTrait.php
   @@ -0,0 +1,136 @@
   +    // Provide a fallback in case label_singular is not set yet.
   +    if (empty($this->label_singular)) {
   +      if ($label = $this->label()) {
   +        $this->label_singular = Unicode::strtolower($label);
   

   This provides a fallback in case the property is not set while the interface declares that it returns null if the property is not set.

   In this method, it actually never returns empty as the label is always mandatory. The only case is when the entity is created which I guess, in that case, it does not matter if the return is "";

   Then again, the property will be saved as "" which will return falsely later on and will not collide with the NULL return type that the interface declares. I think this can be reworked a bit.

4. +++ b/core/lib/Drupal/Core/Config/Entity/EntityBundleWithPluralLabelsTrait.php
   @@ -0,0 +1,136 @@
   +    // Provide a fallback in case label_plural is not set yet.
   +    if (empty($this->label_plural)) {
   +      if ($label = $this->label()) {
   +        $arguments = ['@label' => Unicode::strtolower($label)];
   +        $options = ['langcode' => $this->language()->getId()];
   +        $this->label_plural = new TranslatableMarkup('@label items', $arguments, $options);
   

   This is more or less the same as above.

5. +++ b/core/lib/Drupal/Core/Config/Entity/EntityBundleWithPluralLabelsTrait.php
   @@ -0,0 +1,136 @@
   +   * Gets the plural index through the gettext formula.
   

   I think the keyword "Returns" is more appropriate.

6. +++ b/core/modules/node/config/schema/node.schema.yml
   @@ -9,7 +9,7 @@ node.settings:
   +  type: bundle_entity_with_plural_labels
   

   I have a feeling, that if this patch is to be accepted, the community will still find this type machine name too weird :/

Updates seem fine to me. I am leaving this ticket under review though since it is a core patch that brings a lot of upgrades and it should be accepted with more than one (or a couple) reviews.

I discussed this morning with @pfrenssen and @claudiu.cristea, we agreed to have the ticket in RTBC since there is enough test coverage and it is quite new functionality (no regression issues).

1. +++ b/core/lib/Drupal/Core/Config/Entity/EntityBundleWithPluralLabelsInterface.php
   @@ -0,0 +1,44 @@
   +  /**
   +   * Returns the plural label of the bundle.
   +   *
   +   * @return string|null
   +   *   The singular label or NULL if it cannot be computed.
   +   */
   +  public function getPluralLabel();
   

   c/p error `The singular' => 'The plural'

2. +++ b/core/lib/Drupal/Core/Config/Entity/EntityBundleWithPluralLabelsTrait.php
   @@ -0,0 +1,143 @@
   +   * A list definite singular/plural count label versions.
   

   The english here is a bit disjointed.

   `A list defining` perhaps?

3. +++ b/core/lib/Drupal/Core/Config/Entity/EntityBundleWithPluralLabelsTrait.php
   @@ -0,0 +1,143 @@
   +   *   "1 item\x03@count items",
   ...
   +   * Each value is definite singular/plural count label with the plural variants
   +   * separated by ETX (PluralTranslatableMarkup::DELIMITER).
   +   *
   ...
   +    $label_count = empty($label_count_versions[$context]) ? [] : explode(PluralTranslatableMarkup::DELIMITER, $label_count_versions[$context]);
   

   Feels like we're overloading a primitive when a value object would make the API cleaner.

   class CountLabelPair {
   
     public function getSingularLabel();
     public function getPluralLabel();
   }
   

   I'm the first to admit that i18n isn't my area of expertise, but this feels like we're overloading the return values.

   I get that we use that for database storage purposes, but I'm not convinced we should base our APIs around it, it feels like it should be an internal implementation detail of PluralTranslatableMarkup.

   I did some searching around po files (like I said, no expert) and it looks to be a Drupalism? Correct me if I'm wrong.

   Edit: ah but we need to store them in config files.. so just an array might be simpler - instead of a value object

4. +++ b/core/lib/Drupal/Core/Config/Entity/EntityBundleWithPluralLabelsTrait.php
   @@ -0,0 +1,143 @@
   +    if ($index === -1) {
   ...
   +   *   $count combination or -1 if the language was not found or does not have a
   ...
   +    return -1;
   

   I think a constant instead of -1 would be good. But that would make it a public API, so if we don't want that, leave as is

5. +++ b/core/lib/Drupal/Core/Config/Entity/EntityBundleWithPluralLabelsTrait.php
   @@ -0,0 +1,143 @@
   +      $result = new FormattableMarkup($label_count[$index], ['@count' => $count]);
   +    }
   +    elseif (($singular = $this->getSingularLabel()) && ($plural = $this->getPluralLabel())) {
   

   we can return here and avoid the elseif

6. +++ b/core/modules/node/tests/src/Unit/NodeTypePluralLabelTest.php
   @@ -0,0 +1,240 @@
   +          'with markup' => [1 => '<span>1</span> blue eye', 2 => '<span>2</span> blue eyes'],
   

   Can we add a case here that checks that Xss:filterAdmin is being run, e.g. add one with script tags in it

Do we need to update migration templates for d6/d7 to add these keys to the destination mappings?

This ticket is quite important for us. I see that the interdiff contains the changes from the latest remarks and the rest are answered.
I am putting this back in RTBC and I am kindly assuming that the conversation will not result into further issues.

1. +++ b/core/lib/Drupal/Core/Config/Entity/EntityBundleWithPluralLabelsTrait.php
   @@ -0,0 +1,141 @@
   +        $this->label_singular = mb_strtolower($label);
   ...
   +        $arguments = ['@label' => mb_strtolower($label)];
   

   Channeling my inner German tells me that these mb_strtolower() is not going to be correct in many cases. This was pointed out in #66 by @ckaotik. Not sure that this makes the situation any worse but at some point we have to tackle this problem. It would be great to have @Berdir's thoughts.

2. +++ b/core/modules/node/tests/src/Unit/NodeTypePluralLabelTest.php
   @@ -0,0 +1,240 @@
   +      // No singular/plural labels and a fallback cannot be build.
   +      'no labels' => [NULL, NULL, NULL, NULL, NULL],
   +      // No singular/plural labels but a fallback label could  be build.
   +      'entity label only'  => ['Eye', NULL, 'eye', NULL, 'eye items'],
   +      // No singular label but a fallback singular label could be build.
   +      'no singular label'  => ['Eye', NULL, 'eye', 'eyes', 'eyes'],
   +      // No plural label but a fallback plural label could be build.
   +      'no plural label'  => ['Eye', 'eye', 'eye', NULL, 'eye items'],
   +      // Singular and plural labels were provided.
   +      'singular/plural labels'  => ['Eye', 'eye', 'eye', 'eyes', 'eyes'],
   

   be built and there are some extra spaces in one of the comments.

3. +++ b/core/modules/node/tests/src/Unit/NodeTypePluralLabelTest.php
   @@ -0,0 +1,240 @@
   +    $this->language = $this->getMockBuilder(LanguageInterface::class)
   +      ->disableOriginalConstructor()
   +      ->getMock()
   +      ->expects($this->any())
   +      ->method('getId')
   +      ->will($this->returnValue('en'));
   

   the language property is not defined but it also appears to not be used so it can be removed.

4. +++ b/core/modules/node/tests/src/Unit/NodeTypePluralLabelTest.php
   @@ -0,0 +1,240 @@
   +   * @dataProvider providerForTestGetSingularAndPLuralLabel
   ...
   +  public function testGetSingularAndPLuralLabel($entity_label, $singular_label, $expected_singular, $plural_label, $expected_plural) {
   ...
   +  public function providerForTestGetSingularAndPLuralLabel() {
   

   Plural not PLural

5. +++ b/core/modules/node/tests/src/Unit/NodeTypePluralLabelTest.php
   @@ -0,0 +1,240 @@
   +   * @param array[]|null $count_labels
   +   *   The count label array.
   ...
   +  public function testGetCountLabel(array $count_labels = NULL, $entity_label, $singular_label, $plural_label, array $expectation) {
   +    $count_labels = $count_labels ?: [NULL];
   

   I think the NULL case should be an empty array. It's odd to have an optional nullable param before a required param. Or pass in [NULL] instead as that's what we're converting it too.

1. Yes, similar problem as #2507235: EntityType::getLowercaseLabel() breaks translation. The problem is that the idea of word-used-in-a-sentence-means-lowercase does not apply to all languages, I don't know how many other languages are like German, but a lot of user interfaces in German are awful right now due to all those lowercase labels.

I guess this is a bit different as it's only a fallback and could be set explicitly, but I'm not sure how to set/translate that properly in German either way. At least the new interface doesn't really make it clear to me when I should use getSingularLabel() vs the existing bundle label method?

If it's good enough for entity types, it has to be good enough for bundles.

As @claudiu points out these are fallbacks and can be overridden.

Let's commit this improvement, and file a follow-up for "Fallback labels for entity types and bundles do not allow for inter-language variation in capitalisation". There we could conceivably explore ways of improving this so that languages can declare whether nouns should be capitalised, and using that language information in these labels. But that's likely to be a complex subject with usefulness and ramifications far beyond bundle labels.

RTBC per #98.

Whoops :)

Symfony now offers Inflector component. Which seems useful for such things.
https://symfony.com/doc/master/components/inflector.html

Yes, unlike the entity type lowercase issue, this is only a fallback and can be overridden, that's true. So it is better than that.

That it is equally good/bad as the entity type labels and shouldn't block that is a fair argument, but it's also true that adding those entity type labels resulted in a mess on german sites, also because there it's not something you can update in configuration. That should make it easier to deal with it. Although there is no UI yet ;)

One thing I'm wondering about is whether the config schema definition should have a context key so that it can be translated separately. That might help dealing with conflicts where words are both noun and verb and might need to be translated differently in german. See recent improvement in workflow translation labels.

> Open issues for each bundle type that requires label plurals.

We should probably create an issue for this? Also, introducing a new entity API and only having a single implementation for it is a bit atypical these days, usually we at least add a test implementation in a test entity type too, e.g. for \Drupal\entity_test\Entity\EntityTestBundle.

The API also doesn't really support bundles that aren't config entities, usually extra per-bundle information is added through the bundle info service: \Drupal\Core\Entity\EntityTypeBundleInfo. that would mean it would have to be array keys instead of methods, but the methods defined in the trait could be defined on that service.

I think it would be useful to have @plach comment on this as well.

I spoke with @Berdir about this and I'm mostly +1 on #108. The main issue I had with it was the suggestion to move the trait methods to EntityTypeBundleInfo: ideally that's a bundle definition factory and those methods belong to an hypothetical bundle definition class.

After discussing this, we agreed that we are fine with keeping the trait and the config entity definition as-is to populate the bundle info but the main API to retrieve this info should be the EntityTypeBundleInfoInterface provided by the service. It might make sense to add the fallback logic itself to EntityTypeBundleInfo via protected methods though. This would allow to introduce this additions for any entity type, using our native entity bundle API, without having to rely on individual bundle config entity types and traits.

As @Berdir suggested, we should validate this solution by testing it also via a test entity type and we should provide the bundle definition additions via code, so we have proof that both approaches work the same way.

The patch does not apply anymore, btw.

As a side note, we should really open a follow-up to introduce a proper bundle definition class mimicking entity type and field definitions.

+1 also on

One thing I'm wondering about is whether the config schema definition should have a context key so that it can be translated separately.

For reference, there is more information about non-English languages that have more than 2 plural formulas in comment #2765065-63: [PP-1] Allow plurals on bundle labels.