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

Create the foundation that allows bundle config entities to store plural labels and add this ability to the node_type bundle entity as a start.

  1. Create a new interface EntityBundleWithPluralLabelsInterface that defines 3 new methods:
    • getSingularLabel()
    • getPluralLabel()
    • getCountLabel()

    These are similar to those implemented to entity types.

  2. Make NodeTypeInterface extend also EntityBundleWithPluralLabelsInterface.
  3. Add a common implementation for the new 3 methods in a new trait EntityBundleWithPluralLabelsTrait, to be reused in most of the bundle config entities.
  4. Make NodeType use the EntityBundleWithPluralLabelsTrait trait.
  5. Create a bundle_entity_with_plural_labels schema to be used by bundles that want to support plural labels. The new schema contains the new storage keys: label_singular, label_plural, label_count.
  6. Allow more than one version for the count label. This is because, in the real world, a site might need different versions depending on the place these count labels are used. Let's see a hypothetical website scenario:
    • On a page the count label could be a simple label as 1 article\x03@count articles.
    • On a search results page we need a different version: 1 article was found\x03@count articles were found. Note that we cannot build this version by deriving from the first one because of the was/were thing.
    • In other place something like <span>1</span> article\x03<span>@count</span> articles.
    • In a 3rd place, because the count is displayed in other <div ...> or even is rendered in a different theme hook, just Article\x03Articles, without the count part.

    Similar with translated strings, were we are able to store contextualised translations and then retrieve them by passing the context identifier, we allow label_count to store multiple versions, identifiable by a context string identifier.

  7. Make node.type.* schema extend bundle_entity_with_plural_labels.
  8. Add plural labels to all node types shipped with Drupal core.
  9. The UI for editing node type plural labels is added in a follow-up: #2938251: Allow edit of bundle plural labels in the node type form.

Remaining tasks

User interface changes

None. The UI changes are split in #2938251: Allow edit of bundle plural labels in the node type form.

API changes

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

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

Data model changes

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

CommentFileSizeAuthor
#91 2765065-91.patch24.03 KBclaudiu.cristea
#87 2765065-87.interdiff.txt5.55 KBclaudiu.cristea
#87 2765065-87.patch24.03 KBclaudiu.cristea
#83 2765065-83.patch22.13 KBclaudiu.cristea
#83 2765065-83.interdiff.txt23.59 KBclaudiu.cristea
#79 2765065-79.patch26.55 KBclaudiu.cristea
#79 2765065-79.interdiff.txt735 bytesclaudiu.cristea
#77 2765065-77.interdiff.txt13.98 KBclaudiu.cristea
#77 2765065-77.patch26.55 KBclaudiu.cristea
#75 2765065-75.interdiff.txt730 bytesclaudiu.cristea
#75 2765065-75.patch24.22 KBclaudiu.cristea
#73 2765065-73.interdiff.txt951 bytesclaudiu.cristea
#73 2765065-73.patch24.22 KBclaudiu.cristea
#71 2765065-71.interdiff.txt850 bytesclaudiu.cristea
#71 2765065-71.patch24.09 KBclaudiu.cristea
#69 2765065-69.patch23.26 KBclaudiu.cristea
#63 interdiff-60-to-63.txt761 bytesclaudiu.cristea
#63 2765065-63.patch23.17 KBclaudiu.cristea
#60 interdiff.txt957 bytesclaudiu.cristea
#60 allow_plurals_on_bundle-2765065-60.patch23.42 KBclaudiu.cristea
#58 interdiff.txt3.1 KBclaudiu.cristea
#58 allow_plurals_on_bundle-2765065-58.patch23.43 KBclaudiu.cristea
#51 interdiff.txt15.01 KBclaudiu.cristea
#51 allow_plurals_on_bundle-2765065-51.patch23.46 KBclaudiu.cristea
#50 pl.png153.65 KBclaudiu.cristea
#50 pl0.png139.66 KBclaudiu.cristea
#49 Screen Shot 2016-08-02 at 09.04.15.png131.92 KBalexpott
#47 interdiff.txt3.42 KBclaudiu.cristea
#47 allow_plurals_on_bundle-2765065-47.patch22.88 KBclaudiu.cristea
#44 interdiff.txt2.78 KBclaudiu.cristea
#44 allow_plurals_on_bundle-2765065-44.patch22.87 KBclaudiu.cristea
#42 interdiff.txt690 bytesclaudiu.cristea
#42 allow_plurals_on_bundle-2765065-42.patch22.21 KBclaudiu.cristea
#40 plural_en.png228.7 KBclaudiu.cristea
#40 plural_ro.png280.25 KBclaudiu.cristea
#31 interdiff.txt4.89 KBclaudiu.cristea
#31 allow_plurals_on_bundle-2765065-31.patch22.26 KBclaudiu.cristea
#22 interdiff.txt2.37 KBclaudiu.cristea
#22 allow_plurals_on_bundle-2765065-22.patch23.31 KBclaudiu.cristea
#20 allow_plurals_on_bundle-2765065-20.patch23.27 KBclaudiu.cristea
#20 interdiff.txt4.28 KBclaudiu.cristea
#18 allow_plurals_on_bundle-2765065-18.patch23.23 KBclaudiu.cristea
#18 interdiff.txt673 bytesclaudiu.cristea
#16 nt2.png287.61 KBclaudiu.cristea
#16 nt1.png271.4 KBclaudiu.cristea
#16 interdiff.txt9.3 KBclaudiu.cristea
#16 allow_plurals_on_bundle-2765065-16.patch23.19 KBclaudiu.cristea
#14 allow_plurals_on_bundle-2765065-14.patch15.37 KBclaudiu.cristea
#14 interdiff.txt11.18 KBclaudiu.cristea
#10 interdiff.txt5.22 KBclaudiu.cristea
#10 allow_plurals_on_bundle-2765065-10.patch12.11 KBclaudiu.cristea
#2 allow_plurals_on_bundle-2765065-2.patch11.61 KBclaudiu.cristea
node_type_labels.png182.76 KBclaudiu.cristea
Members fund testing for the Drupal project. Drupal Association Learn more

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?

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

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

claudiu.cristea’s picture

@ckaotik, that is only the fallback, the default value. Once, you've entered your value that will be used.

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: -sprint
FileSize
23.26 KB

Rerolled (including converting the update path test to a functional test).

Status: Needs review » Needs work

The last submitted patch, 69: 2765065-69.patch, failed testing. View results

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
24.09 KB
850 bytes

Fixing the hal/rest tests.

The last submitted patch, 69: 2765065-69.patch, failed testing. View results

claudiu.cristea’s picture

FileSize
24.22 KB
951 bytes

Ensure a fallback for the case the plural variant index cannot be computed (i.e. the 'locale' module is off).

Status: Needs review » Needs work

The last submitted patch, 73: 2765065-73.patch, failed testing. View results

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
24.22 KB
730 bytes

Better test that a plural variant is available.

pfrenssen’s picture

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

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
26.55 KB
13.98 KB

Recently, in the project I'm working, I've faced this situation: I needed more than one version of the count label, depending on the place I used that label. For example on a page I need a simple label as '1 article\x03@count articles', in other place something like '<span>1</span> article\x03<span>@count</span> articles' and in a 3rd place just 'Article\x03Articles'. But how to accomplish this with the current patch? The patch from #75 only allows us to store and use a single version of the count label but in the real world having multiple arbitrary versions is not so uncommon.

To overcome this problem I'm proposing that we allow multiple versions to be stored in the bundle entity. Then we can retrieve the desired version by passing a context string identifier to ::getCountLabel(). In fact we are doing the same with translated strings were we are able to store contextualised translations and then retrieve them by passing the context identifier. It's the same here. This patch deals with this allowing storage and retrieval of multiple versions of count label.

WARNING: In this patch the NodeTypeForm is not yet fixed to allow multiple versions. Neither the corresponding tests are allowing this in this stage. That part should be reworked. This patch is only a PoC to show the idea.

It also, fixes #76.1 and #76.2. The placeholder stuff needs more thinking.

Status: Needs review » Needs work

The last submitted patch, 77: 2765065-77.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
735 bytes
26.55 KB

Fix the rest/hal tests

idimopoulos’s picture

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 :/

idimopoulos’s picture

Status: Needs review » Needs work

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

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

claudiu.cristea’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
23.59 KB
22.13 KB

#80.1

Why do we need a singular label?

I don't want to copy/paste all the arguments that were already discussed when these label versions were added in #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed to entity types. For a background discussion, please follow that issue, especially #3, #8, #27 to #32.

#80.2

plural_label is a type of data. Please see core/config/schema/core.data_types.schema.yml. label_plural is the name of the bundle config entity property. It has this representation because it follows this unified pattern: label_(singular|plural|count). We want to keep the same key names as in #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed.

#80.3, 4

The unit test that I just added, proves that the return of these methods is typed as string|null, but I agree that the interface @return docs might be confusing so I fixed the docs.

#80.5

Good point. Fixed.

#80.6

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

Well, I think the community like when a specifier is descriptive. I like it too :)

Other changes:

  1. Rerolled.
  2. Split the UI part in #2938251: Allow edit of bundle plural labels in the node type form.
  3. Added a unit test to test all scenarios that may occur in EntityBundleWithPluralLabelsTrait, especially when some plural labels are missed and we want to assure fallback values.
  4. Updated the issue summary.
claudiu.cristea’s picture

Issue summary: View changes

More IS fixes.

claudiu.cristea’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 83: 2765065-83.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

claudiu.cristea’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
24.03 KB
5.55 KB

This umami thing is something new. Improved docs & tests, fix coding standards. IS fixes

idimopoulos’s picture

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.

idimopoulos’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 87: 2765065-87.patch, failed testing. View results

claudiu.cristea’s picture

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

Rerolled.