Discussed with @catch on IRC to close #23298: Entity bundles should declare a plural form of their label in favour of this.

Problem/Motivation

Entity types can define now plurals for their labels starting with #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed. It's possible now to have the variants "content item" (singular) and "content items" (plural) for nodes. We want the same for bundles. We want to be able to have "article" (singular) and "articles" (plural).

It's not the same as for entity types. Entity types can define the label plural variants in the annotation while for bundles the site builder should be able to add such variants. Thus we need to store the variants in the bundle config entity and provide UI for site builders.

Proposed resolution

  1. Make \Drupal\Core\Config\Entity\ConfigEntityBundleBase implement a new interface ConfigEntityBundleInterface.
  2. Add next methods to the new ConfigEntityBundleInterface interface: getSingularLabel(), getPluralLabel() and getCountLabel(). These are similar to those implemented to entity types.
  3. Create a 'bundle_config_entity' schema that contains the new storage keys: label_singular, label_plural, label_count.
  4. Implement the UI and storage for NodeType entity type.

Remaining tasks

Open issues for each bundle type that requires label plurals.

User interface changes

User is able to add plural variants in NodeTypeForm UI.

The site is installed in English (2 variants)

The site is installed in Romanian (3 variants)

API changes

New interface \Drupal\Core\Config\Entity\ConfigEntityBundleInterface:

  • getSingularLabel()
  • getPluralLabel()
  • getCountLabel()

Data model changes

The bundle config entity has new keys/values: label_singular, label_plural, label_count.

Files: 

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

This is a first try, a PoC

claudiu.cristea’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: allow_plurals_on_bundle-2765065-2.patch, failed testing.

Gábor Hojtsy’s picture

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.');
    }
Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config, +sprint
claudiu.cristea’s picture

@Gábor Hojtsy,

I agree but there's only one form for label_plural. This is also true for #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed.

When it comes to count labels, the patch allows multiple (>2) variants.

+++ 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);
+    }
+    else {
+      // We assume 2 plurals if Locale's services are not available.
+      $plurals = 2;
+    }
...
+    for ($i = 0; $i < $plurals; $i++) {
...
+    }

.

Gábor Hojtsy’s picture

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.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
12.11 KB
5.22 KB

@Gábor Hojtsy, thank you for review.

#9.1: True. As is extending config_entity I rename it bundle_config_entity.

#9.2: Right about type: string vs. type: label. I changed that. But sequence vs. plural_label the problem is the same in terms of overriding on translation. In terms of storage I like more sequence (an indexed array) because: (1) it's more readable when you inspect the config as YAML and (2) it removes the need of imploding/exploding a string. In fact plural_label is only storing an array/sequence as a concatenated string. But in terms of override is the same. Let's see what is happing when translating an English node-type in Polish. The label_count is a sequence with 2 items and in Polish will have only 2 items. When you visit the Polish node-type edit form, the form will still expose 3 text fields. The first 2 copied from English and the 3rd one filled with the fallback value. After the user make changes and submits the form, in the Polish version of the node-type config entity all 3 variants will be stored. The same happens if we use plural_label.

#9.3: It defaults to stored value but falls back to a default value.

#9.4: Correct. Changed.

#9.5: Changed.

Still needs tests.

claudiu.cristea’s picture

Sorry, you are right on #9.2. Only now I enabled the config_translation module and saw that i cannot add the 3rd variant there. Will change that in the next patch.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ 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.

The last submitted patch, 10: allow_plurals_on_bundle-2765065-10.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
11.18 KB
15.37 KB

@Gábor Hojtsy, fixed that & many others.

claudiu.cristea’s picture

Issue summary: View changes
claudiu.cristea’s picture

Issue summary: View changes
FileSize
23.19 KB
9.3 KB
271.4 KB
287.61 KB
claudiu.cristea’s picture

Issue tags: -Needs tests

Tests were added in #16.

claudiu.cristea’s picture

Do not add label_singular, label_plural, label_count to config_export if were already added in annotation.

Gábor Hojtsy’s picture

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.

claudiu.cristea’s picture

Thank you for review, @Gábor Hojtsy!

#19.1: Done.

#19.2

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

Hm. You mean by implementing JS stuff?

#19.3: I've opened #2766857: Provide a PluralTranslatableMarkup::getPluralVariantIndex() factory method and changed the @todo.

#19.4: OK. I completely removed the "missed" state message to avoid cluttering the vertical tab label.

#19.5: "Placeholder" sounds better?

claudiu.cristea’s picture

@Gábor Hojtsy, I added a patch also in #2766857-3: Provide a PluralTranslatableMarkup::getPluralVariantIndex() factory method. If that is accepted we change this accordingly.

claudiu.cristea’s picture

Addressed #19.2

Gábor Hojtsy’s picture

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

claudiu.cristea’s picture

Thank you for review, @Gábor Hojtsy. Do you think you can take a look also at #2766857: Provide a PluralTranslatableMarkup::getPluralVariantIndex() factory method? That would help here to avoid the code duplication (ConfigEntityBundleBase::getPluralIndex() is a copy/paste from PluralTranslatableMarkup).

Bojhan’s picture

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?

claudiu.cristea’s picture

@Bojhan, In fact these are forms of the label. As we let the site builder to enter the bundle label why wouldn't we allow the same with the label plural form?

Gábor Hojtsy’s picture

@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.

Bojhan’s picture

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.

claudiu.cristea’s picture

@Bojhan, this is the reason I hide it in an non-active vertical tab. Or you mean the tab label?

Bojhan’s picture

Yup, the label.

claudiu.cristea’s picture

@Bojhan, OK, I removed that. However, don't you think it would be helpful as UX if we just show a message there if plurals are missed, like "Needs plural labels", or similar?

I also converted the new NodeTypePluralLabelsTranslationTest test into a BrowserTestBase.

Status: Needs review » Needs work

The last submitted patch, 31: allow_plurals_on_bundle-2765065-31.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

Failures seems unrelated

Status: Needs review » Needs work

The last submitted patch, 31: allow_plurals_on_bundle-2765065-31.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

Ran Drupal\Tests\dblog\Kernel\Migrate\d6\MigrateDblogConfigsTest locally and passes. Strange

Retesting

Bojhan’s picture

Issue tags: -Needs usability review
penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

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.

claudiu.cristea’s picture

I think this needs a change record and possibly, being a new feature, should be included in the release notes.

claudiu.cristea’s picture

claudiu.cristea’s picture

Issue summary: View changes
FileSize
280.25 KB
228.7 KB

Updating screenshots to match the RTBC patch.

catch’s picture

Issue tags: +beta deadline

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.

claudiu.cristea’s picture

@catch, yes it was very fast. But this was possible thanks to @Gábor Hojtsy and @Bojhan on reviewing the issue.

Only a small fix: We need to preserve the count label plural variants deltas.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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?

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
22.87 KB
2.78 KB

@alexpott, tested that change.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Remarks at #43 have been covered.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
  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.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
22.88 KB
3.42 KB

@tim.plunkett, thank you for review.

#46.1: OK.

#46.2: The first builds the labels and, reading the code, it's obvious why. That one is producing the same labels as in \Drupal\views\Plugin\views\field\NumericField. The 2nd, specifically that return, is a fallback default value, when we have no labels stored in the backend but we're still able to build a decent fallback. Why we're not using formatPlural? Simply because we cannot predict any plural variant. We use t() only for words ordering inside the string (in other languages the order might be reversed). So, we provide the same plural default value fallback for all @count > 1 plural variants.

#46.3, 4: OK, even I disagree. That was supposed to help in IDE. Now $node_type->getSingularLabel() is highlighted in PhpStorm and the IDE is yelling "Method 'getSingularLabel' not found in \Drupal\node\NodeTypeInterface" because the method is in other NodeType interface. I think we should re-consider that kind of commenting because that is NOT type-hinting, but only a DX helper.

#46.5: Fixed.

#46.6: That special char is NOT a newline, it's PluralTranslatableMarkup::DELIMITER.

I'm moving back to RTBC as changes are only nits to documentation.

tim.plunkett’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
131.92 KB

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?
claudiu.cristea’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
139.66 KB
153.65 KB

@alexpott, thank you for your comments.

#49.1: I don't think we should provide any upgrade path. The labels are site builder decisions, we cannot predict that. Neither on labels of standard profile. If such a site is updated then the site builder will have to visit the page and manually enter his desired labels. Imagine that on a site installed prior this patch the site builder has set the 'article' main label to 'Super-mega article'. The site updates. Who give us the right to set the plural labels to 'articles'? This should be entirely a manual process.

#49.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.

No, please re-read the IS. In this issue we are providing the foundation for all bundle types to define such plural label variants. Some of them might provide UI to allow site builder to set them. Some of them will decide to set such labels through the code and, probably, some will ignore this feature. But we should provide all of them with the chance to declare such label variants. This issue is not about the NodeType specifically but about all bundle types. I choose to implement here the UI for NodeType because is the most popular and also to provide an example for other core or contrib implementations. After committing this we ca decide to open a new follow-up issue for taxonomy vocabularies. But if we don't, probably someone will create a contrib for that. The foundation is there. In custom modules a lot will provide the labels directly in the code (in default config YAMLs) but will use the same foundation.

#49.3: I cannot reproduce that. If I'm adding a new content type all the fields are empty (expected). If I save the form without entering any of them, next time I visit the form I see them filled with the fallback values because now there is a main label and the form is able to build some decent fallbacks.

admin/structure/types/add

Saving without entering any value.

admin/structure/types/manage/mouse

claudiu.cristea’s picture

xjm’s picture

Issue tags: -8.2.0 release notes

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

claudiu.cristea’s picture

Discussed with @alexpott on IRC abut this feature. @alexpott suggested that we should make the interface & trait more generic and use them on both, on entity type and on bundles. But this seems a headache and would not bring too much benefit because:

Problem on EntityType on bundle Effect
$label_count has different structure associative array with 3 keys: 'singular', 'plural', 'context' a string with plural delimiters - can be transformed into indexed array with an arbitrary number of items (>=2) need to override $label_count and ::getCountLabel()
The label method is different as name ::getLabel() ::label() need to override ::getLowercaseLabel()
Different fallback template in ::getPluralLabel() '@label entities' '@label items' need to override ::getPluralLabel()
Different fallback template in ::getCountLabel() '@count @label' and '@count @label entities' (@label is getLowercaseLabel) '1 @singular', '@count @plural' (@singular is ::getSingularLabel, @plural is :: getPluralLabel) need to override ::getCountLabel()

Having all this differences, I don't think it makes too much sense add this level of abstraction. The only benefit would be the interface but the methods will have to be overwritten in implementations. Should I go with only a common interface or we remain on #51?

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.

claudiu.cristea’s picture

The tests for 8.3.x are passing. We should unblock this issue.

claudiu.cristea’s picture

Status: Needs review » Needs work

The last submitted patch, 51: allow_plurals_on_bundle-2765065-51.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea’s picture

Status: Needs work » Needs review
claudiu.cristea’s picture

Removed t() also from WTB.

pfrenssen’s picture

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?

pfrenssen’s picture

Status: Needs review » Needs work
Issue tags: -beta deadline

Updating status and removing outdated tag.

claudiu.cristea’s picture

FileSize
23.17 KB
761 bytes

@pfrenssen, thank you for review. Rerolled and fixed #61.

#61.2: Double quotes are required when using control characters like \x03. Quoting from https://symfony.com/doc/current/components/yaml/yaml_format.html#strings:

If the string contains any of the following control characters, it must be escaped with double quotes:

\0, \x01, \x02, \x03, \x04, \x05, \x06, \a, \b, \t, \n, \v, \f, \r, \x0e, \x0f, \x10, \x11, \x12, \x13, \x14, \x15, \x16, \x17, \x18, \x19, \x1a, \e, \x1c, \x1d, \x1e, \x1f, \N, \_, \L, \P

#61.3, 4:

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?

No. The correct plural variant is determined by the number of items (count) to be represented. That formula is how gettext is telling how many variants of plurals has a specific language. This is the nplurals=3; part. That means the Romanian language has 1 variant for singular and 2 variants for plural. The 2nd part is telling how to find what variant of plural to use depending of the count. In this case (Romanian):

plural=((n==1)?(0):(((n==0)||(((n%100)>0)&&((n%100)<20)))?(1):2));

If the count (n) is 1 use the 1st variant (delta 0 = the singular). If count is 0 or inside one of the next intervals 2..19, 101..119, 201..219, 301..319, etc., use the 2nd plural variant (delta 1). Otherwise use the 3rd one. This looks strange for a native English speaker as in English there's only singular and 1 variant of plural. In English the gettext plural formula would be nplurals=2; plural=(n != 1);. Here's a list with plural variants per language: http://docs.translatehouse.org/projects/localization-guide/en/latest/l10...

claudiu.cristea’s picture

Status: Needs work » Needs review

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.

ckaotik’s picture

Status: Needs review » Needs work

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?