Problem/Motivation

The views number field handler supports the following use cases:

  • Precision (round the values) (optionally)
  • Configure a decimal / thousand separator
  • Allow to configure single/plural exists for format_plural This is missing from number field formatters in Entity Fields
  • Allow to prefix/suffix the output

On #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality , we switched numeric base fields in Views so that they are rendered by the Entity rendering system (i.e., using Field Formatters to format the output) instead of using the base Views numeric field handler. So, we've lost a bit of functionality and we want to make sure that the formatters for numeric fields in the Entity rendering system gets the functionality that the Views base field handler has.

Proposed resolution

Add the ability to configure a Field UI numeric field formatter to output using format_plural, like you can currently on the \Drupal\views\Plugin\views\field\Numeric class.

We also need a test for foreign languages and format_plural, with both Views and Entity Displays. It should:
- Create a view and an entity display in a language with N plural forms, where N != 2.
- Verify that the formatter works correctly for several field values (picks the correct plural form)
- Verify the settings form works correctly

Remaining tasks

Make a patch, including test.

User interface changes

The Field UI formatter for numbers will have additional options allowing you to specify the output for singular and plural values, similar to how the Views numeric field handler does this.

API changes

Additional settings on the Field UI formatter for numeric fields.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is not really a bug per se, and it's not really a feature since the functionality existed before for numeric base fields and we lost it in #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality .
Issue priority Major because of the regression and the config changes involved.
Disruption Some disruption for configuration of field formatters for numbers, but defaults are provided so it should not actually break anything.
Files: 

Comments

jhodgdon’s picture

Gábor Hojtsy’s picture

Priority: Normal » Major
Gábor Hojtsy’s picture

Issue tags: +Needs tests
FileSize
5.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,052 pass(es), 76 fail(s), and 8 exception(s). View

Did you have this in mind @dawehner?

Gábor Hojtsy’s picture

Status: Active » Needs review

Note that the translatability of the singular/plural variant here is pretty bad, that is an existing problem with the views numeric formatter too though.

dawehner’s picture

In general this doesn't look bad, but sure, we need additional test coverage.

Status: Needs review » Needs work

The last submitted patch, 3: 2449597-3.patch, failed testing.

Gábor Hojtsy’s picture

Thanks. I looked at how to make this translatable, and opened #2453761: Views numeric formatter's plural formatting setting incompatible with many languages.

Gábor Hojtsy’s picture

Status: Needs work » Postponed

Looks like this is impossible to resolve in a useful way without #2453761: Views numeric formatter's plural formatting setting incompatible with many languages. If we go the way of the patch, users cannot create field config with correcy data in languages like Polish, which has multiple plural forms. We need to allow entering as many plural forms as the configuration language allows.

dawehner’s picture

Can we get this patch in without the translation support? Its IMHO orthogonal to add translation support.

jhodgdon’s picture

@dawehner: It seems like if we're to have proper singular/plural support that will actually work for languages with multiple forms, the setting for this in Views and/or formatters needs to be totally redone. So I'm not sure what the benefit of doing this now would be, if we're adding fucntionality to number formatters that would then have to be totally redone?

Gábor Hojtsy’s picture

@dawehner: see @jhodgdon. Basically the current assumption that a plural is either a singular or a plural only holds for some of our supported languages. Think of creating a view on a Polish (3 variants) or a Slovenian site (4 variants). You don't even need to have config translation enabled on the site, you are of course not creating your views/field formatters in English, you would create them in Polish and Slovenian respectively. The current UI assumes that you are using one of the languages that only has 2 variants, a singular and a plural.

Here is a quick summary of the number of languages that have X number of variants with 2 being admittedly the most common but far from universal (source data is the tiny bit outdated http://cgit.drupalcode.org/l10n_pconfig/tree/l10n_pconfig.module#n115 given that localize.drupal.org now has 30+ more languages):

Number of variants:

# of variants # of languages
2 63
3 11
4 2
5 1
6 1

Implementing with the current singular/plural separation of UI and config storage would only support config languages for 80% of languages listed in l10n_pconfig (which is a subset of localize.drupal.org). We would need to break the API (config format) to store them differently to support the other 20% afterwards.

So it seems like it makes sense to fix it now, provide a good example for contrib and then introduce this, so we don't need to change it soon again.

dawehner’s picture

I see, so we have to actually store more than just singular and plural. Fair!

Gábor Hojtsy’s picture

Yup, #2453761: Views numeric formatter's plural formatting setting incompatible with many languages stores them as one string concatenated with \x03 which is how locale can automatically integrate it. We need to store them as one string not as a config list because then translating config lists is only possible by swapping out items, so you would not be able to add or remove items. #2453761: Views numeric formatter's plural formatting setting incompatible with many languages is getting there in terms of the fixes for views and formatting plurals.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,422 pass(es), 79 fail(s), and 9 exception(s). View
4.36 KB

Making changes to adapt to the new capabilities from #2454859: Not possible to format plural an already translated string and new pattern set up by #2453761: Views numeric formatter's plural formatting setting incompatible with many languages. Still two @todos which I would welcome help with.

1. How to get the langcode of the configuration (entity) for this formatter.
2. How to elegantly alter the submitted values of this form (there does not seem to be a submit method available here, should we delegate it optionally to formatters?).

Status: Needs review » Needs work

The last submitted patch, 15: 2449597-15.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,6 +58,36 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +    // @todo get langcode of this formatter config to use for configuration.
    

    As talked with gabor, we might could pass it along with the configuration

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,6 +58,36 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +    // @todo figure out how to alter the form values when this is submitted.
    

    ... I guess we have to expand the FormatterInterface to have a new method method, and call that in \Drupal\field_ui\Form\EntityDisplayFormBase::copyFormValuesToEntity and \Drupal\views\Plugin\views\field\Field::submitOptionsForm

effulgentsia’s picture

For #17.2, you can use a #value_callback. See FileWidget for an example.

Gábor Hojtsy’s picture

@effulgentsia: the UI we expose here is multiple text input fields. The value we need to save is a concatenated string. So we cannot name the series of fields the same way the concatenated string will be AFAIS. While it seems to be possible to use a value_callback to set a different form value and unset form values entirely (so they don't end up in the resulting config), that sounds like a bit too much side effects from a value callback? :)

If I were to use a value_callback on this, I'd add a new format_plural_string element and assign the value callback to that. Then that value callback would fill in that element with the concatenated value and entirely unset the series of text elements. Is that what you had in mind or something better?

@dawehner: #17/1: you mean via PluginBase::$configuration? That sounds like it would only contain the config of the plugin itself. Is there a way to get the parent config entity used to store the plugin? Or is that cutting through an abstraction we should not cut through? (Ie. do not make an assumption that this plugin is necesarily stored on some config)?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,977 pass(es), 2 fail(s), and 0 exception(s). View
591 bytes

Should have removed the common prefix_suffix not the scale schema from decimal.

Status: Needs review » Needs work

The last submitted patch, 20: 2449597-20.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,980 pass(es), 2 fail(s), and 0 exception(s). View
1.33 KB

Fixing the migrate expectations about keys in config.

Status: Needs review » Needs work

The last submitted patch, 22: 2449597-22.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
8.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,980 pass(es). View
792 bytes

Update the migration defaults too.

Gábor Hojtsy’s picture

FileSize
4.85 KB
10.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,967 pass(es), 1 fail(s), and 1 exception(s). View

Attempted to add tests for the new fields. That exposed that the fields are not actually in the form. Renamed $form to $elements. That works.

Added a format_plural_string element to carry the concatenated value. Used the #value_callback with this. It does not seem to actually see the format_plural_values elements. :/

Status: Needs review » Needs work

The last submitted patch, 25: 2449597-25.patch, failed testing.

effulgentsia’s picture

It might if you give it a higher weight. But also, I think it should be possible to put the #value_callback on the element that is the parent of the textfields. i.e., rename 'format_plural_values' to 'format_plural_string' and put the #value_callback on that. That way, you wouldn't need to mess with $form_state at all, you'd get everything you care about from $input. I'll play with that later unless someone beats me to it.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
10.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,983 pass(es). View
3.13 KB

I think it should be possible to put the #value_callback on the element that is the parent of the textfields. i.e., rename 'format_plural_values' to 'format_plural_string' and put the #value_callback on that.

Sadly there's a limitation with FAPI that complicates an element having a non-array value and child elements. But also, it's weird DX to name the element as '*_string' and then have it contain child elements. So, here's a way that does work. It uses #after_build, which I think is the appropriate place in the pipeline.

rteijeiro’s picture

FileSize
11.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,232 pass(es). View
1.06 KB

Updated formatPluralValuesAfterBuild() function docblock. The rest seems fine.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@effulgentsia, @rteijeiro: thanks, looks great. Now we only need to fix the langcode passing in:

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
@@ -46,10 +58,70 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+    // @todo get langcode of this formatter config to use for configuration.
...
+    $plurals = $this->getNumberOfPlurals();

This one.

@dawehner: what did you have in mind for this one? Not sure how would merging in the langcode of the parent entity work here.

dawehner’s picture

My simple idea was that core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php:193 puts the langcode into the $configuration, coming from $this->language()
Maybe though that solution is too straightforward.

yched’s picture

We need to know the language of the EVD config entity ? Right, that's not passed in atm :-/
Whatever we do here, we'd need to do the same with EntityFormDisplay and Widget::settingsForm()

I guess EVDs / EFDs are not the only ones to delegate settingsForm() on a plugin - how does Views handle this it, for example ?

(which brings : when the formatter settings form is displayed as part of a View in Views UI rather in Field UI's "Manage display", we'll need to receive the language of the View config entity instead ? At any rate, we don't want to hardcode EVDs in formatters, EVDs are only one use case for formatters, those should remain agnostic of their context)

Gábor Hojtsy’s picture

@yched: right, we are coming here from views exactly :) In #2453761: Views numeric formatter's plural formatting setting incompatible with many languages we implemented that in Views' NumericField::buildOptionsForm() as follows:

    $plurals = $this->getNumberOfPlurals($this->view->storage->get('langcode'));

(getNumberOfPlurals() comes from StringTranslationTrait()). So a numeric field plugin knows the view it is on via a $view property. Easy :) I used the word "host entity" when we discussed exactly because as I see it, we want to make the formatter settings reusable. It is already on views and entity displays. So if we want it to be similar to views, then the formatter would have a reference to its "host entity" (EVD or view or something else) to get the langcode from.

Gábor Hojtsy’s picture

Issue tags: -sprint

Just wanted to reiterate that this needs technical direction to proceed. Removing from sprint due to apparent lack of interest :/

Gábor Hojtsy’s picture

Also found this one while reviewing with @effulgentsia:

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
@@ -73,6 +145,10 @@ public function viewElements(FieldItemListInterface $items) {
+      if ($this->getSetting('format_plural')) {
+        $output = $this->formatPluralTranslated($output, $this->getSetting('format_plural_string'));
+      }

This would look ugly. I think it needs a str_replace(LOCALE_PLURAL_FORMULA, ',', $this->getSetting(...))

effulgentsia’s picture

Agreed with #32. My suggestion is to either:
- Add a $settings_langcode constructor parameter to FormatterBase and WidgetBase.
- Change the existing $settings parameter from an array to an object that implements ArrayAccess and getLangcode().

If we do the latter, it would raise the question of whether we should also do the same for $third_party_settings.

And an even harder question: should we also do the same for the $configuration parameter of all other plugin constructors, though maybe that's more a topic for #1764380: Merge PluginSettingsInterface into ConfigurablePluginInterface.

k4v’s picture

Assigned: Unassigned » k4v

picking this up @drupaldevdays

k4v’s picture

Assigned: k4v » Unassigned
yched’s picture

Change the existing $settings parameter from an array to an object that implements ArrayAccess and getLangcode().

Sounds a bit scaringly invasive :-). As you mention, I think it would make sense it this was unified in ConfigurablePluginInterface and we had #1764380: Merge PluginSettingsInterface into ConfigurablePluginInterface. But that doesn't sound like a reasonable prerequisite for this issue here ?

So for now I think passing $settings_langcode as an addition param in the construct would be the most reasonable option. FormatterBase and WidgetBase indeed already have a lot of construct params, but well.. :-/

Also, #1867518: Leverage entityDisplay to provide fast rendering for fields is reshuffling how views render fields, and my latest approach over there switches back to always use a (runtime-defined) EntityViewDisplay internally to "talk to the Formatters". So in that approach Views would simply need to set its own langcode on the EVD it builds, and the code in EVD then passes its langcode to the formatters it creates.
Thus, "passing the settings langode" is covered by EVD for both "regular entity view" and "views rendering of fields".

The APIs for "render a single field / field item with arbitrary formatter plugin_id and settings" would still need adjusting though. That is:
FieldItemInterface::view($display_options)
FieldItemListInterface::view($display_options)
which delegate respectively to:
EntityViewBuidler::viewFieldItem($item, $display_options)
EntityViewBuidler::viewField($items, $display_options)

All of the above would need to be able to specify a langcode for the settings specified in $display_options.

dawehner’s picture

FieldItemInterface::view($display_options)
FieldItemListInterface::view($display_options)
which delegate respectively to:
EntityViewBuidler::viewFieldItem($item, $display_options)
EntityViewBuidler::viewField($items, $display_options)

Well, this really feels like a flaw if we would have to change all those APIs, to include the langcode option here. Honestly, it feels much more straightforward to pass along the langcode
as part of the display_options, given how much the API impact would be otherwise. I'm a bit confused by those though actually need the langcode, Why can't they retrieve the information from the entity itself, if needed?

yched’s picture

Not sure what you mean by "flaw", but sure I don't think I have an issue with having the caller pass the "settings langcode" next to the "settings" entry in $display_options in the methods above. Not having a dedicated argument means we need to have a same default if the caller fails to provide that entry in the array (and they *will* forget it)

Then, the methods have to assign that langcode to the EVD they internally create, and the EVD will pass it along to the formatters it creates.

Just like a Views (after the "leverage EVD" patch) will assign its own langcode to the EVDs it creates, and those EVDs pass it to the formatters as above.

I'm pretty strongly opposed to Formatters having any knowledge of what an EVD is. They receive settings, they just need the associated langcode, not the entity where those settings come from (why would there necessarily be an entity ?)

jhodgdon’s picture

Bump... We still need to figure out what to do here?

pcambra’s picture

Status: Needs work » Needs review
FileSize
13.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,892 pass(es), 127 fail(s), and 30,414 exception(s). View
3.83 KB

Here's a patch that adds the settings langcode in the FormatterBase, let's see if this can be the right approach.

I was looking at #35 but I can't find what LOCALE_PLURAL_FORMULA is in the codebase, I'll ask Gabor for a clarification.

Status: Needs review » Needs work

The last submitted patch, 43: 2449597-format_plural-43.patch, failed testing.

Gábor Hojtsy’s picture

+++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
@@ -69,7 +69,7 @@ public function createInstance($plugin_id, array $configuration = array()) {
+    return new $plugin_class($plugin_id, $plugin_definition, $configuration['field_definition'], $configuration['settings'], $configuration['label'], $configuration['view_mode'], $configuration['third_party_settings'], $configuration['settings_language']);

+++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
@@ -345,6 +345,9 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, arr
+    // Carry the language to the plugin.
+    $display_options['settings_language'] = $this->entity->get('langcode');

Let's name it "settings_langcode" instead because its not a language object per say, its a langcode string. We try to be very consistent about this so people using the API are clear on what they are getting in this value.

pcambra’s picture

Status: Needs work » Needs review
FileSize
14.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,464 pass(es), 463 fail(s), and 2,023 exception(s). View
2.02 KB

Agreed, fixing the langcode thing, also test should be better now.

Status: Needs review » Needs work

The last submitted patch, 46: 2449597-format_plural-46.patch, failed testing.

Gábor Hojtsy’s picture

@pcambra: thanks! Looks like from the fails, the settings_langcode is persisted in config, it should not be. Also, uri_link generation seems to be missing passing this over (or you should use an isset() or !empty() when using it).

  1. +++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
    @@ -69,7 +69,7 @@ public function createInstance($plugin_id, array $configuration = array()) {
    -    return new $plugin_class($plugin_id, $plugin_definition, $configuration['field_definition'], $configuration['settings'], $configuration['label'], $configuration['view_mode'], $configuration['third_party_settings']);
    +    return new $plugin_class($plugin_id, $plugin_definition, $configuration['field_definition'], $configuration['settings'], $configuration['label'], $configuration['view_mode'], $configuration['third_party_settings'], $configuration['settings_langcode']);
    

    This causes the undefined index fails. I think we can do $settings_langcode = isset($configuration['settings_langcode']) ? $configuration['settings_langcode'] : ''; and then use that to pass in?

  2. +++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
    @@ -69,7 +69,7 @@ public function createInstance($plugin_id, array $configuration = array()) {
    @@ -148,6 +148,7 @@ public function prepareConfiguration($field_type, array $configuration) {
    
    @@ -148,6 +148,7 @@ public function prepareConfiguration($field_type, array $configuration) {
           'label' => 'above',
           'settings' => array(),
           'third_party_settings' => array(),
    +      'settings_langcode' => '',
    

    This causes it to persist, no?

pcambra’s picture

Status: Needs work » Needs review
FileSize
13.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,596 pass(es), 2 fail(s), and 14 exception(s). View
1.5 KB

Thanks a lot for the review Gabor, fixed the persistence problem and added an isset in the plugin manager

Status: Needs review » Needs work

The last submitted patch, 49: 2449597-format_plural-49.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
13.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,605 pass(es). View
754 bytes

Adding the settings langcode only when there are display options already.

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation, -Needs tests +sprint

Looks good. I provided a considerable part of this patch, so not sure I can RTBC it too. Looks like we need reviews. @dawehner, @jhodgdon, can you help with reviews?

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/config/schema/core.entity.schema.yml
    @@ -268,33 +268,36 @@ field.formatter.settings.language:
    +    format_plural_string:
    +      type: label
    +      label: 'Singular and one or more plurals'
    

    So a single label can contain all these things? 2 to N values? That seems counter-intuitive.

  2. +++ b/core/lib/Drupal/Core/Field/FormatterBase.php
    @@ -63,7 +63,7 @@
        * @param array $third_party_settings
        *   Any third party settings.
        */
    -  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings) {
    +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, $settings_langcode = '') {
    

    New parameter, but docblock isn't updated.

  3. +++ b/core/lib/Drupal/Core/Field/FormatterBase.php
    @@ -71,6 +71,7 @@ public function __construct($plugin_id, $plugin_definition, FieldDefinitionInter
    +    $this->settingsLangcode = $settings_langcode;
    

    Missing protected $settingsLangcode on the class.

  4. +++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
    @@ -69,7 +69,10 @@ public function createInstance($plugin_id, array $configuration = array()) {
    +    $settings_langcode = isset($configuration['settings_langcode']) ? $configuration['settings_langcode'] : '';
    

    Hrm… so it's optional configuration? Why?

    The docblock two points higher should clarify that.

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,10 +58,69 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +   *   Textfield elements with plural string set.
    

    s/elements/element/
    s/plural string/format_plural_string/

  6. +++ b/core/modules/field/src/Tests/Number/NumberFieldTest.php
    @@ -456,6 +456,21 @@ function testNumberFormatter() {
    +    $edit["fields[${integer_field}][settings_edit_form][settings][format_plural_values][0]"] = '1 item';
    +    $edit["fields[${integer_field}][settings_edit_form][settings][format_plural_values][1]"] = '@count items';
    

    I'd expect this to test with >1 plural form, otherwise we don't know if it really supports >2 values.

Gábor Hojtsy’s picture

@wimleers: Re #53/1: we don't know how many plural variants there are for a language. Config translation works by deep array merging values on top of originals with translations. If we store the singular/plural variants as list items, then a translation with 2 plural forms would leave bogus plural forms if the source used 4 plural forms for example. We use one key to encode the singular/plural pairs, so the override in the config translation (or whatever other override) works trivially with how the overrides are designed to work. Also, its automatically integrated with string extraction which finds the singular/plural pair properly this way in shipped config.

I agree with the other notes, including that it would be best to test this with multiple plurals, although we did test that generally storing and translating them works in #2453761: Views numeric formatter's plural formatting setting incompatible with many languages, it does not hurt to test it here too with this specific implementation.

Wim Leers’s picture

Ok, cool. Is that first paragraph documented somewhere? Can the config schema then do an @see to that?

On the second paragraph: indeed, it is still valuable to test >=2 plurals here, because there is form submission logic to massage the form values into the expected shape.

Gábor Hojtsy’s picture

@wimleers: the use of the label type for this predates this issue (see #2453761: Views numeric formatter's plural formatting setting incompatible with many languages and #2454859: Not possible to format plural an already translated string which uses the same API). Further down, #2454829: Configuration translation UI does not support plural sources/targets is expected to introduce a proper plural label type in config schema, so that it will be more evident from the typing of the config schema (and also evident for config translation UI that is :). Since otherwise config translation UI does not support this yet in a user-sane way, it kind of needs to be resolved before release.

Wim Leers’s picture

Ok, good to hear that that will still be resolved. What about #55.1?

jhodgdon’s picture

I took a careful look at the patch, and the code looks very solid to me.

I have a couple of questions/suggestions about the UI text and the code comments and API docs blocks:

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,10 +58,69 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +        '#title' => ($i == 0 ? $this->t('Singular form') : $this->formatPlural($i, 'First plural form', '@count. plural form')),
    

    So the labels here will say:
    Singular form
    1. plural form
    2. plural form

    Will this make sense to people? In English I guess there will just be "1. plural form", but will it make sense in Russian? What does localize.d.o do here?

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,10 +58,69 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +        '#description' => $this->t('Text to use for this variant, @count will be replaced with the value.'),
    

    Not sure if this text makes it clearer either?

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,10 +58,69 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +      // Simplify user interface text for the most common case.
    +      $elements['format_plural_values'][0]['#description'] = $this->t('Text to use for the singular form, @count will be replaced with the value.');
    +      $elements['format_plural_values'][1]['#title'] = $this->t('Plural form');
    +      $elements['format_plural_values'][1]['#description'] = $this->t('Text to use for the plural form, @count will be replaced with the value.');
    +    }
    

    OK, so this makes it easier for English and other 2-form languages, so that is good. Still not sure that it makes sense for other languages but I am not able to judge...

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,10 +58,69 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
       /**
    +   * Form API callback. Sets the value for format_plural_string.
    +   *
    

    This one-line description doesn't conform to our docs standards. Should probably say:

    After-build callback: Sets the value for the format_plural_string property.

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,10 +58,69 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +   * This method is assigned as a #after_build in settingsForm() method.
    

    Should be:

    This method is assigned as an #after_build function in \namespace\class::settingsForm().

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,10 +58,69 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +   * @param array $element
    +   *   Textfield elements where the plural format should be set.
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    

    Oh, Wim mentioned this. It should either be $elements or the text should be singular.

  7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,10 +58,69 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +   * @return array
    +   *   Textfield elements with plural string set.
    +   */
    

    Here too it should be singular element not elements.

  8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,10 +58,69 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +    // The 'format_plural_values' element is a list of textfields, but we want
    +    // to store a 'format_plural_string' string as the setting value, so add
    +    // that to $form_state. It's not necessary to remove 'format_plural_values'
    +    // from $form_state, because only defined settings get retained anyway.
    +    if ($form_state->isProcessingInput()) {
    

    I think this information would be better off in the function doc block.

    I also think it could be clearer. I didn't understand it and had to read the code to understand what the method is doing.

    Basically, I think what you want to say is something like this, in the doc block:

    The user interface for setting up singular/plural forms is an array of text fields, for usability, but for translation purposes, the values have to be stored as a single string in the field settings. This function implodes the values.

    Or something like that?

Gábor Hojtsy’s picture

@Wim: I am not sure #2454859: Not possible to format plural an already translated string did a good enough job for your (understandable) expectations to document this. But its the same API TranslationInterface::formatPluralTranslated() uses. I think that would be something for #2454829: Configuration translation UI does not support plural sources/targets to resolve because it *needs* the separate config schema type as an anchoring point to document why it is how it is IMHO. At this point where we don't have such a distinct type, its hard to put the docs at a sensible place for it.

Gábor Hojtsy’s picture

@jhodgdon: re #58 1-3 this is the same UI used in #2453761: Views numeric formatter's plural formatting setting incompatible with many languages, #2454829: Configuration translation UI does not support plural sources/targets and also existing UI in locale. So 3 places. Unfortunately there is no good way to reuse the same form API construct, so we copied the same UI around 3 times (and someone implementing will need to do the same). So if we are to do improvements to it, it already pertains to two existing core UIs in Views numeric formatter and locale. I think that should be another issue on its own to manage scope.

jhodgdon’s picture

Cool. I was just asking. If that is the UI we are using everywhere, then at least people will only have one UI to learn. Always a good idea. :)

yched’s picture

Sorry for being so late for feedback, been swamped with client work.

Focusing on the "EVD / FormatterPluginManager / Formatter chain" part of the patch, leaving the specifics of number formatters aside.

I agree with the overall approach, with the following remarks :

  1. Minor: the new $configuration['settings_langcode'] entry should be documented in the phpdoc for FormatterPluginManager::getInstance()
  2. The patch only passes 'settings_langcode' to the formatters instantiated to display settings form in the "Manage display" UI screen.

    - This would be better off in EntityViewDisplayEditForm::getPlugin() IMO.

    - We also need to pass it to formatters instantiated :

    a) at entity->view() runtime
    --> EntityViewDisplay::getRenderer() should also set $configuration['settings_langcode'] = $this->langcode

    b) at single $field->view() runtime
    --> the langcode needs to be passed in somehow as a parameter to FieldItemList::view() / FieldItem::view() (those delegate to EnityViewBuilder::viewField() / EnityViewBuilder::viewFieldItem() resp.), and assigned as the langcode of the EntityViewDisplay returned by EntityViewBuilder::getSingleFieldDisplay(). Then a) ensures it's passed to the formatter.

    c) by Views at display runtime
    --> Views' EntityFieldRenderer::buildFields() just need to assign the langcode of the View the the EVDs created for rendering. Then a) ensures it's passed to the formatter.

    d) by Views UI when it displays formatter settings forms
    --> Views' Field::buildOptionsForm() needs to pass the langocde of the View to the formatter it creates.

  3. Actually, after the cleanups made in EntityViewDisplayEditForm a couple weeks ago (it is now a regular "entity edit" form for EVD config entities), I think it should be doable to replace EntityViewDisplayEditForm::getPlugin() with just $this->entity->getRenderer(), in which case EntityViewDisplay::getRenderer() would be the single point of entry for assigning 'settings_langcode' (outside from Views UI).

    That sounds out of scope for this issue here, though. I opened #2497847: Simplify EntityDisplayEditFormBase ajax rebuild flow to work only with $this->entity, and for now we need to assign 'settings_langcode' in both EntityViewDisplayEditForm and EntityViewDisplay.

  4. Finally, we should keep consistency on the widget side, and introduce the 'settings_langcode" in WidgetBase / WidgetPluginManager / EntityFormDisplay / EntityFormDisplayEditForm as well. The specific case here is about a formatter, but there's no reason why the case couldn't arise for a widget too ?

    Anyway, let's get the formatter side right, and we can discuss whether we handle widgets in the same patch or in a followup (I'd prefer the former, should be fairly straightforward mirroring once formatters are done)

yched’s picture

Status: Needs work » Needs review
FileSize
15.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,616 pass(es), 728 fail(s), and 533 exception(s). View
6.33 KB

Patch does #62 1., 2.a, 2.c, 2.d
(as mentioned in @todo comments, could use some double check by @dawehner or @plach about the Views part)

Leaves out 2.b and FieldItemList::view() / FieldItem::view() for now.

Status: Needs review » Needs work

The last submitted patch, 63: 2449597-format_plural-63.patch, failed testing.

yched’s picture

FileSize
15.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,610 pass(es), 738 fail(s), and 533 exception(s). View
774 bytes

Bleh, langcode is protected, of course.

yched’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 65: 2449597-format_plural-65.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
34.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,256 pass(es), 36 fail(s), and 141 exception(s). View
22.83 KB

Wow, I'm rusty. s/language()->id()/language()->getId()

Also, all formatters extending FormatterBase::__construct() and provide static create() factory methods need to be updated to receive the new $settings_langcode param as well now :-/

Status: Needs review » Needs work

The last submitted patch, 68: 2449597-format_plural-68.patch, failed testing.

yched’s picture

FileSize
36.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,298 pass(es). View
3.72 KB

boring boilerplate copy/paste typo, a leftover language()->id(), and fixing a migrate test that got added meanwhile

yched’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Re @jhodgdon #38/1: @maxocub opened #2499639: Use better labels for numeric fields when using a multiple plural forms language coming from #2454829: Configuration translation UI does not support plural sources/targets to deal with this question, so that should be a great place to deal with it.

jhodgdon’s picture

Status: Needs review » Needs work

This is looking good!

I took a careful look at the code and docs, and have mostly docs/UI text suggestions. I think the code is pretty solid.

  1. +++ b/core/config/schema/core.entity.schema.yml
    @@ -268,33 +268,36 @@ field.formatter.settings.language:
    +    format_plural:
    +      type: boolean
    +      label: 'Format plural'
    +    format_plural_string:
    

    For the label here, maybe it should be "Use plural formatting" ?

  2. +++ b/core/lib/Drupal/Core/Field/FormatterBase.php
    @@ -62,8 +62,10 @@
    +   *   The langcode associated to the settings passed above.
    

    nitpick: associated *with* and probably should say "language code" not "langcode"

    So I think this should say:

    The language code associated with the settings passed above.

    This text is in MANY places in the patch, and should be fixed in all of them. I didn't list them all here.

  3. +++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
    @@ -61,6 +61,9 @@ public function createInstance($plugin_id, array $configuration = array()) {
    +    // @todo Not sure we need to keep that, there should always be a settings_langcode set ?
    

    If this @todo is going to be committed, it needs an issue, but this looks like one we should just resolve before the patch is finalized.

  4. +++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
    @@ -94,6 +97,9 @@ public function createInstance($plugin_id, array $configuration = array()) {
    +   *     - settings_langcode: (string) The langcode associated to the settings
    

    Here's another place:

    ==> language code associated with the settings

  5. +++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
    @@ -94,6 +97,9 @@ public function createInstance($plugin_id, array $configuration = array()) {
    +   *       passed above. That is typically the langcode of the config entity the
    

    langcode => language code

    config => configuration

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
    @@ -52,13 +52,15 @@ class EntityReferenceEntityFormatter extends EntityReferenceFormatterBase implem
    +   * @param string $settings_langcode
    +   *   The langcode associated to the settings passed above.
        * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
        *   The entity manager.
        * @param LoggerChannelFactoryInterface $logger_factory
    

    I don't see $entity_manager in the list of actual function params here? Should be removed. Slightly out of scope I guess.

  7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,10 +58,69 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +      '#title' => $this->t('Format plural'),
    

    How about

    Use plural formatting

    for the title here?

  8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,10 +58,69 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +   * Form API callback. Sets the value for format_plural_string.
    +   *
    +   * This method is assigned as a #after_build in settingsForm() method.
    +   *
    

    See my previous review; not addressed yet. Comment #54, items 4 and 5.

  9. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -46,10 +58,69 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +    // The 'format_plural_values' element is a list of textfields, but we want
    +    // to store a 'format_plural_string' string as the setting value, so add
    +    // that to $form_state. It's not necessary to remove 'format_plural_values'
    +    // from $form_state, because only defined settings get retained anyway.
    +    if ($form_state->isProcessingInput()) {
    

    See previous review, comment #58, item 8. Check on items 6 and 7 also please.

  10. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
    @@ -70,13 +70,15 @@ class ImageFormatter extends ImageFormatterBase implements ContainerFactoryPlugi
        * @param \Drupal\Core\Utility\LinkGeneratorInterface $link_generator
        *   The link generator service.
        */
    -  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, AccountInterface $current_user, LinkGeneratorInterface $link_generator, EntityStorageInterface $image_style_storage) {
    -    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);
    +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, $settings_langcode, AccountInterface $current_user, LinkGeneratorInterface $link_generator, EntityStorageInterface $image_style_storage) {
    +    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings, $settings_langcode);
         $this->currentUser = $current_user;
    

    Out of scope, but not all the params are documented here.

  11. +++ b/core/modules/views/src/Entity/Render/EntityFieldRenderer.php
    @@ -191,6 +191,8 @@ protected function buildFields(array $values) {
    +      // @todo could use double-check from dawehner / plach :-)
    

    Another @todo that needs to be addressed before we commit this.

  12. +++ b/core/modules/views/src/Plugin/views/field/Field.php
    @@ -478,6 +478,8 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +        // @todo could use double-check from dawehner / plach :-)
    

    And another @todo to address.

yched’s picture

Thnking about #62.2.b - fields rendered by :
$entity->field->view($options_with_settings)
$entity->field[N]->view($options_with_settings)

Views doesn't use this since #1867518: Leverage entityDisplay to provide fast rendering for fields a couple weeks ago, but there might still be other cases that still do, and where $options_with_settings is taken from some config entity. The langcode associated with the settings is then that of the config entity, and it needs to be passed along as well.

We have a choice between a new argument, or munging that as a new entry in the $options_with_settings array. One difference would be : is that an optional or a required information. If the caller doesn't specify which langcode goes with the settings it provides, is there a default langcode we can pass to the formatter ? The current content or interface langcode ? Or the langcode of the $entity->field ?

jhodgdon’s picture

So... Let's step back here and think about what we're doing. This issue, at least the summary, is about making sure that Field UI formatters for integer fields have the ability to do format_plural() functionality, right? [I don't think that format_plural() makes any sense at all for anything but integer-valued fields, and come to think of it, looking at the current patch, it appears that this setting may be present for more than just integer-valued fields? Shouldn't be, as I don't think formatPlural will work right? It's on the UI in NumericFormatterBase... not sure how widespread that will be.]

Anyway, since we're formatting an integer-valued entity field, the langcode that is used should always be the language we're rendering the entity in, shouldn't it? I think even in Views that is the case: when we have an integer entity field in Views that is being used, we're supposed to be using the entity rendering system in Views as well, which I think sets up the entity, translates it into the language we're supposed to be using for rendering, and renders the field that way.

Hmm...

On the other hand, it would be similar to fields that have prefix/suffix settings, or field labels, or other user-entered text that is part of the config for that field. In those cases, which language do we choose to translate that configuration text? It seems like that also should be the entity rendering language. But if it isn't (like for instance if we're picking up the UI language for the page), then we should make the same choice for the plural formatting, I think, because it's very similar to a field label or even more similar to a prefix/suffix setting.

So. I guess my conclusion is: whatever we are already doing to pick out which language to use for config translation of the prefix/suffix settings, field labels, and similar field config, we should do the same for this.

Thoughts?

jhodgdon’s picture

I woke up this morning realizing there is more to this, duh. We have to make sure that the language we pass into formatPluralTranslated() matches the language that was used to translate the field settings config for the plural forms. Otherwise, the plural formatting may not work at all and would be at best wrong.

So... let's say we have a node with an int/plural-form field on it, and it has English and Russian translations. We're viewing the Russian translation of that field/node for some reason (whether in Views or on a page like ru/node/5).

Then I'd say that we need to make sure that (a) the field config is translated into Russian (so that we get 3 plural variants out of the field settings config translation), and (b) when we call formatPluralTranslated(), passing in that config translation, we pass in Russian for the language so that the pluralization rules correctly calculate that the value of 7 uses the 3rd form (which wouldn't exist if we'd passed in the English version of the config translation with only 2 plural variants; similarly if we passed in the 3 Russian forms but used the English rules, we'd calculate that 7 is plural and pick the 2nd form instead of the 3rd and that would be wrong too).

Uh. Where's the code in the patch that even uses formatPlural or formatPluralTranslated to format the number? The only calls I see to anything like formatPlural* (in the patch file) are in the settings code. I don't see anything in the field formatting code at all?

Gábor Hojtsy’s picture

@jhodgdon: those are very good points. In #2453761: Views numeric formatter's plural formatting setting incompatible with many languages we assumed that whatever language the view was loaded in will be fine to use for displaying the plural stuff. Regardless of the actual language of the field displayed. That may indeed be a false assumption given that the view may be loaded in a different language compared to the fields shown. In fact the view may be loaded in language A and the view may be displaying fields in languages B, C and D. We did not consider reloading the whole view in a different language (which would be the way to get the right config translation) to somehow pick out the "right" translation. I think entity displays and views are a similar problem in this area because the field does not / should not have control over the language the config was loaded in even if the content field displayed with that config may be in different language from the config. This is true to all labels, prefixes, etc. though, so its both an existing problem and a pretty darn hard thing to solve given how we want to try to keep the elements independent.

I'd say we should assume the config language loaded is fine and use that to display the plural.

jhodgdon’s picture

Well, this certainly won't work at all unless we use the same language for views/field config translation as we use for formatPlural*() calls.

So the field labels, prefixes, suffixes, etc. are also in the views/field config translation language. Given that, to me the plural forms are similar to prefix/suffix, so they should be in the same language. In #76 I suggested that in an ideal world, this language would be the entity rendering language, but I agree that this would be difficult and/or impossible, plus in a View it would probably be weird to have some text that is part of the view config translated into one language and some in another.

So. Agreed. The language we need to pass into formatPlural*() to format integer-valued fields with plural forms is the language that was used to translate the field/view config.

So... Isn't config always translated into the detected/selected/negotiated UI language? If so, isn't that also the default language used by formatPlural*()? If so, we don't need to pass in a language at all?

Maybe there's a flaw in all those ifs, but if not, that would greatly simplify this patch. ;)

jhodgdon’s picture

Just one more point: when we are formatting a field from Views, we substitute in its field settings config as if it was a view mode with a set of field settings config, right? Entity fields don't have config for these settings themselves -- they're stored on each particular view mode or in the view. Presumably that stuff (view mode config or view config) is translated when it's first loaded from the config system... well I have no idea when it's loaded, but in the case of a View, it would be way before we get to the rendering stage, and I would think/hope the view config would be loaded in the UI text selected/detected/negotiated language. I'm not sure what happens in a page like ru/node/5 -- but whatever language the Full view mode config is loaded in, we'd need to use that language.

Gábor Hojtsy’s picture

So... Isn't config always translated into the detected/selected/negotiated UI language? If so, isn't that also the default language used by formatPlural*()? If so, we don't need to pass in a language at all?

Yes it is. That is the same thing we assumed in #2453761: Views numeric formatter's plural formatting setting incompatible with many languages. However, that given config translation may or may not include a translation for the plural form, so the plural variants used live may or may not have as many options as formatPlural() attempts to use. That is not that big of a problem as it may sound because formatPlural() has this:

        // If the index cannot be computed or there's no translation, use
        // the second plural form as a fallback (which allows for most flexibility
        // with the replaceable @count value).
        $return = $translated_array[1];

If site admins see this happening, best they can do is to fix their config translation honestly.

Yes, that may still be the case that the source config had a one variant plural and there was no translation in which case this index will not exist either. This code should be made more robust to handle that case, it was not updated when we started using it for formatting already translated things.

I don't think these are in scope for this issue to fix, but they do affect how formatPlural() is called.

jhodgdon’s picture

OK then.

a) It sounds like we need a separate issue to handle the case of "There aren't even two elements in my array of plural forms" in formatPlural() or is it formatPluralTranslated()? Anyway, wherever it is, so that we don't get a PHP error/warning in $return = $translated_array[1];

b) It seems like all the code in this patch that passes languages around can be removed, because the config will automatically be translated into the UI Interface Text detected/selected language for the current request, and that is also the default language for formatPlural*().

Right?

Gábor Hojtsy’s picture

We at least need to pass around language for the configuration form, which is where we started from. We need to present a UI consistent with the language of the containing config entity, so the resulting translation process make sense, where you translate other settings, the source value for the plural setting will also be in the same language (and use the appropriate amount of variants in the source).

jhodgdon’s picture

That is true. It's a bit confusing because you have the UI language and 2 config languages (source and translation) when presenting a config translation for one of these, and in the config edit case (views or view mode), you have the UI language and the config source language. The number of elements to present has to be relative to the config language (source or translation), so yes we need to pass that language around. Phew! Very confusing.

Gábor Hojtsy’s picture

Yeah you may be using German to edit a Polish view. So when configuring format plural stuff, you need all the variants for Polish (more than German), because the config you are editing mandates that. That you happen to edit it on a German UI does not matter for the number of fields. It will affect the labels printed on the forms however, which is why your ideas in #2499639: Use better labels for numeric fields when using a multiple plural forms language don't work well (to let the labels be translated as appropriate for the config language). (Will post there too).

yched’s picture

It feels weird to state "we only ever need the langcode of the formatter settings when using the formatter to present its settings edit form, we won't need it when the formatter displays values" ?
That might be true for the specific case for formatPlural*() according to the discussion above, but more generally ?

If we do pass the settings_langcode in the Formatter construct, then what I get from the above is :
- If not specified for some reason by the code that instanciates the formatter, we can default to the UI langcode
- If the Formatter has a $this->settings_langcode, then IMO on output NumericFormatter::viewElements() should pass it to formatPluralTranslated(), without relying on the fact that it's most likely equal to that method's default ?

jhodgdon’s picture

I agree that it seems weird to say that we only need the language code explicitly when we're presenting the settings edit form, but I think it's actually true, and getting at the correct language is kind of convoluted.

The problem is that the correct language that we'd need to pass into formatPlural*() in this patch would be "the language the config for the view or view mode or whatever originated the field formatter settings was translated into for this request". That translation is done when that config is first loaded up, not during the actual formatting/rendering process, and it's always using the UI text negotiated/selected/detected language. Of course, that also happens to be the default language for the formatting/rendering process for formatPlural*(), which is why we can get away with not trying to figure out what that language is and pass it around for a while until we get to the formatting step.

The low-level functions for formatPlural*() should still have the ability to pass in an override $langcode -- I'm not advocating for removing that. I'm just saying we don't need to override it in the Number Formatter code from this patch, and since I also think it's both problematic and complicated to override it and get it right (I am not convinced the current patch did get it right, actually), I don't think we should do it.

If we come up with a reason we need this functionality at some later time, that would be the time to go refactor the code, not now when we don't actually need it (ever read "Extreme Programming"? :) Good food for thought).

yched’s picture

If we come up with a reason we need this functionality at some later time, that would be the time to go refactor the code, not now when we don't actually need it

I disagree. If we later decide we in fact need that "settings langcode" injected into the whole Formatter for all its lifetime and operations, we won't be able to do so without breaking existing / contrib formatters.

Settings should go hand in hand with "the langcode of those settings", it's an overlook that they currently don't in HEAD. I'd be much more comfortable with an architecture where Formatters consistently have a "settings langcode" the moment they also have "settings", rather then sprinkling that langcode as a separate opportunistic parameter passed in a couple formatter methods where it was deemed needed and not in others until we find we need them there too. Less headaches maintaining a system where two associated params are not injected in two separate ways.

I am not convinced the current patch did get it right, actually

Do you have more specific concerns ? ;-)
I think the patch is correct. The formatter receives the settings langcode from the thing that also holds the settings - the EVD.

My question in #74 was just about what we do for the API that lets you specify arbitrary runtime settings, and the discussion above gave me the answer : the caller should also pass the associated settings langcode, and we can default to "the current interface langcode" if it doesn't. Patch doesn't do that yet, I will try to update it later today.

yched’s picture

Status: Needs work » Needs review
FileSize
38.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,712 pass(es). View
36.27 KB

Added the behavior mentioned at the end of #87 (adresses #62.2.b)

Gábor Hojtsy’s picture

I am fine with passing the langcode around, even assuming the langcode will not result in *correct* output in a predictable way, because we don't know if the config translation did have that config file translated and if it had, even if that translation contained that key or not, so the actual plural variants may or may not be in the language it is supposedly translated to (which is why its important the formatPlural() stuff is resilient). We cannot really do much about this IMHO, even if we would know the translation status on that very detailed level, it may still be the exact same string but still a valid translation (if the words used are the same across languages). So we need to be prepared for possibly not getting it right either way, there is no need to stress over that IMHO.

jhodgdon’s picture

OK, that's good. But:

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
@@ -196,6 +196,7 @@ public function getRenderer($field_name) {
 
     // Instantiate the formatter object from the stored display properties.
     if (($configuration = $this->getComponent($field_name)) && isset($configuration['type']) && ($definition = $this->getFieldDefinition($field_name))) {
+      $configuration['settings_langcode'] = $this->langcode;

Can we put comments in where we set this ['settings_langcode'] explaining what it is and why this choice was made? Like here, something like:

Record the language that the configuration was translated into, which is needed for plural formatting (for example).

Or something like that.

Also it doesn't look like my previous review comments on the documentation (#73) have been addressed yet, so I will wait to review this patch until they are.

jhodgdon’s picture

Status: Needs review » Needs work

Setting back to needs work based on last comment.

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: -sprint +SprintWeekend2015
FileSize
39.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,962 pass(es), 46 fail(s), and 17,818 exception(s). View

Reroll. Think I merged this properly. I am going to see if I can wrap this up today, assuming this still comes up green.

$ git checkout 2f903e47ac9301fd7f13752e681e1fdf516e8a25

$ git apply --index 2449597-format_plural-88.patch

$ git rebase 8.0.x
First, rewinding head to replay your work on top of it...
Applying: 2449597-format_plural-88.patch
Using index info to reconstruct a base tree...
M	core/config/schema/core.entity.schema.yml
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
M	core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeDefaultFormatter.php
M	core/modules/field_ui/src/Form/EntityViewDisplayEditForm.php
M	core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
M	core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
M	core/modules/migrate_drupal/src/Plugin/migrate/process/d6/FieldFormatterSettingsDefaults.php
M	core/modules/migrate_drupal/src/Tests/d6/MigrateFieldFormatterSettingsTest.php
M	core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
M	core/modules/views/src/Entity/Render/EntityFieldRenderer.php
M	core/modules/views/src/Plugin/views/field/Field.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/views/src/Plugin/views/field/Field.php
Auto-merging core/modules/views/src/Entity/Render/EntityFieldRenderer.php
Auto-merging core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
Auto-merging core/modules/migrate_drupal/src/Tests/d6/MigrateFieldFormatterSettingsTest.php
Auto-merging core/modules/migrate_drupal/src/Plugin/migrate/process/d6/FieldFormatterSettingsDefaults.php
Auto-merging core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
Auto-merging core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
Auto-merging core/modules/field_ui/src/Form/EntityViewDisplayEditForm.php
CONFLICT (content): Merge conflict in core/modules/field_ui/src/Form/EntityViewDisplayEditForm.php
Auto-merging core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeDefaultFormatter.php
CONFLICT (content): Merge conflict in core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeDefaultFormatter.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
CONFLICT (content): Merge conflict in core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
Auto-merging core/config/schema/core.entity.schema.yml
Failed to merge in the changes.
Patch failed at 0001 2449597-format_plural-88.patch
The copy of the patch that failed is found in:
   /Users/matt/Documents/PhpStorm/drupal-8.0.x/.git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

Status: Needs review » Needs work

The last submitted patch, 92: number_formatters_make-2449597-92.patch, failed testing.

mpdonadio’s picture

Blerg. I think I know what happened here; #2399211: Support all options from views fields in DateTime formatters introduced the base class, DateTimeFormatterBase, which also need the $settings_langcode change. Redoing this rebase + manual merge.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
41.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,119 pass(es), 1 fail(s), and 13 exception(s). View

Running out for a bit, but this may be better. DateTimeFieldTest and TimestampFormatterTest pass locally for me now.

Status: Needs review » Needs work

The last submitted patch, 95: number_formatters_make-2449597-95.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
42.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,252 pass(es). View
760 bytes

I could have sworn that DateTimeFieldTest came up green when I ran it locally.

mpdonadio’s picture

FileSize
42.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,270 pass(es). View
15.13 KB

OK, this should take care all of the current feedback except for the @todos: #73-3, #73-11, and #73-12. Need to wrap my head around this patch before looking at them.

mpdonadio’s picture

Read through the patch and the @todos.

#73-3: I ran a side test with

if (!array_key_exists('settings_langcode', $configuration)) {
  throw new \Exception('$configuration[settings_langcode] is missing');
}

in it, and got 452 exceptions. So, either `$configuration += ['settings_langcode' => NULL];` needs to stay and we create a followup or figure out what is going on (I have no clue).

#73-11,12: Hopefully @dawehner can weigh in, but I don't see where else the langcode should come from in this case, so I think these two @todos can just be deleted.

Gábor Hojtsy’s picture

@mpdonadio: re settings_langcode key, looks like the only place it is set in the $configuration array is for entity view displays. Field formatters used with anything else would not have it set. Not sure if field formatters would always be embedded in some kind of entity view display? I don't know the use cases well enough. Looking at the code though, otherwise they don't get a langcode for settings.

jhodgdon’s picture

Field formatters should only be used in entity view modes and Views. Where did these exceptions come from?

jhodgdon’s picture

Status: Needs review » Needs work
Issue tags: +D8 upgrade path, +Needs upgrade path, +Needs issue summary update

Also this patch appears to be changing config schema, so it will need the new "data model changes" section added to the issue summary and it will need an upgrade path. See summary of #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter for an example; in this case, I think it affects both views and view modes with numeric fields, right?

Gábor Hojtsy’s picture

@jhodgdon: this only adds new keys in config which are not required (have default values), so I don't see how this needs an upgrade path?

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Perhaps you're right. I'm really not sure. Restoring status.

jhodgdon’s picture

Issue summary: View changes

Summary seemed unclear in its wording: clarifying slightly what is views field handlers vs. field UI field formatters.

jhodgdon’s picture

Reviving this issue...

I took a look at the test output of #99. It seems that the failure is in node_add_body_field(), which is called implicitly by a lot of tests when they create node types. There's a section in there where it adds a 'text_default' formatter to the Body field for the 'default' view mode, and for some reason, this is triggering the error. So maybe 'settings_langcode' is not being set up properly in this type of code (from node_add_body_field()):

  entity_get_display('node', $type->id(), 'default')
      ->setComponent('body', array(
        'label' => 'hidden',
        'type' => 'text_default',
      ))
      ->save();
mpdonadio’s picture

FileSize
42.13 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,655 pass(es), 1 fail(s), and 36,989 exception(s). View
1.12 KB

Maybe this is better for #73-3? Patch still has @todos about requesting input from @dawehner @plach (#73-11,12).

Status: Needs review » Needs work

The last submitted patch, 107: number_formatters_make-2449597-107.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
43.32 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 54,882 pass(es), 797 fail(s), and 504 exception(s). View
1.83 KB

That went well. Let's see what this does. Note that this has debug code in it to help narrow down what is happening.

Status: Needs review » Needs work

The last submitted patch, 109: number_formatters_make-2449597-109.patch, failed testing.

mpdonadio’s picture

At a loss on this one. I thought #109 would have covered the theory in #106.

Looking at the exception, I can kinda see what is happening, but not why there is a problem with the configuration for some formatter plugins, but not all.

jhodgdon’s picture

OK, let's take a look at the failures from the last patch.

a) There are several tests that failed with a config schema exception, such as
core.entity_form_display.node.page.default:content.body.settings_langcode missing schema

So the problem there is that in

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
@@ -196,6 +196,9 @@ public function getRenderer($field_name) {
 
     // Instantiate the formatter object from the stored display properties.
     if (($configuration = $this->getComponent($field_name)) && isset($configuration['type']) && ($definition = $this->getFieldDefinition($field_name))) {
+      // Record the language that the configuration was translated into, which
+      // is needed for plural formatting (for example).
+      $configuration['settings_langcode'] = $this->langcode;

You cannot actually just add things to configuration that are not part of the schema, with strict schema checking turned on (as it is in Simpletest).

It seems like maybe what needs to happen is to add the settings_langcode information to the formatter instead of to the configuration? The formatter is instantiated just below this line:

       $formatter = $this->pluginManager->getInstance(array(
         'field_definition' => $definition,
         'view_mode' => $this->originalMode,
...

So we could add methods to the formatter to get/set the settings language code maybe?

b) Looking at one of the failures from the exception you added in #109, one of them is in Drupal\aggregator\Tests\AddFeedTest

If you look at the stack trace from your exception, it's happening during the install process for the aggregator module, when Drupal's config system is adding the config items from the config/install directory to the site config. So apparently, one of the stored configs from Aggregator module has a timestamp_ago formatter in it, and during the config save, it's not getting a settings langcode.

The config in this case is
core.entity_view_display.aggregator_feed.aggregator_feed.default.yml
and Drupal's config system is basically reading in this file as a Config object, and then calling $config->save().

I think your code in #109 may not be triggered during the save of a view mode config object?

Again, maybe what we need is for the Formatter class to just have a get/set method for the settings langcode. Have it return the language of the config if it hasn't been set explicitly to something else. Then we don't have to mess with config schema problems and hopefully everything will just work?

mpdonadio’s picture

I think this is somewhat repeating #112, but in my mind it sounds like we need to add settings_langcode to the schema for all of the formatters that have settings (to solve the load/save problem with schema exceptions), and then add ::defaultSettingsLangcode(), ::getSettingsLangcode(), and ::setSettings() to PluginSettingsInterface (not sure if this will have unintended consequences). Then, add FormatterPluginManager::getDefaultSettingsLangcode() and wire it up in FormatterPluginManager::prepareConfiguration() (these last two changes would ensure we always have a settings_langcode).

I think that would eliminate the fragility of this.

?

jhodgdon’s picture

That sounds like a good plan, except I'm not sure about adding to all plugins on PluginSettingsInterface.

It seems like it is just for formatters, so it would be better to add it to Drupal\Core\Field\FormatterInterface (which extends PluginSettingsInterface), not to PluginSettingsInterface? Then the base implementations could also be added to the FormatterBase class, and most likely no contrib modules would be adversely affected by this interface getting new methods, because they probably all extend FormatterBase in their formatters. Right?

mpdonadio’s picture

Yeah, FormatterInterface / FormatterBase would be best; I chased one too many classes when thinking this through. I'll work on it this week. I also pinged @larwolan to see if he wants to help on this issue. We have tag teamed on some issues w/ the timezone differences and cranked them out quickly. This would be nice to have in before RC1.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio
FileSize
45.41 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 94,115 pass(es), 141 fail(s), and 35 exception(s). View
4.37 KB

Ug. Here is the path we need to take. Interdiff is against #98 to make it clear.

The new patch removes the `$configuration += ['settings_langcode' => NULL];` bit and replaces it with our debug blip. In the places where defaults for settings and third_party settings are created, settings_langcode is added in. Then the entity schema is updated in a few places. Then (sigh), settings_langcode is added to the config for the fields on individual modules.

The attached patch does this for the aggregator module, and AddFeedTest should now pass.

This will fail, as I did not update the billion other places where settings_langcode needs to be added. I think it may be safe to search the .yml for "settings_langcode", make sure it is a field, and then add in "settings_langcode: null".

Before I do this, does this sound like the right plan?

mpdonadio’s picture

Status: Needs work » Needs review

Of course I forget to change status...

Status: Needs review » Needs work

The last submitted patch, 116: number_formatters_make-2449597-116.patch, failed testing.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned

Unassigning myself until I actually work on this.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
17.8 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php. View
32.4 KB

OK... So... I discussed this with @mpdonadio in IRC today...

Going back to the patch in #98 again (the last known passing patch before the latest experiments). It is kind of a hybrid, because it is adding settings_langcode to both the FormatterBase as an argument, and also to the settings. We should only have to do one or the other.

So I simplified the approach. I'm adding an interdiff to #98 but the actual patch is smaller, because this takes out the changes to the FormatterBase class constructor (and all its child classes). The idea here is that when we are setting up a Formatter as part of an Entity View Mode, we should at that time add the language code of the Entity View Mode to the formatter settings directly (as part of the $settings array, not a separate component). Since we're already passing $settings around all over the place, this requires much less code.

Let's see how much it breaks... Attached is a new patch, plus an interdiff to #98.

It seems like we will need to add a test to this to verify that the settings form is correct in a non-English language view and entity view mode. The existing test only seems to verify English I think.

Status: Needs review » Needs work

The last submitted patch, 120: number_formatters_make-2449597-119.patch, failed testing.

jhodgdon’s picture

FileSize
17.8 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,706 pass(es), 1 fail(s), and 0 exception(s). View

Doh. Missed a ) in NumericFormatterBase in this line:

+    $plurals = $this->getNumberOfPlurals($this->getSetting('settings_langcode'));

Edited patch, trying again.

jhodgdon’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 122: number_formatters_make-2449597-122.patch, failed testing.

jhodgdon’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
876 bytes
18.66 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,697 pass(es). View

Not too bad -- one test failure, easily fixed (and understandable, given this change). Here's a fix for that. Let the reviews begin!

And I still think we need a test for the settings langcode behavior, with both Views and Entity Displays. It should:
- Create a view and an entity display in a language with N plural forms, where N != 2.
- Verify that the formatter works correctly for several field values (picks the correct plural form)
- Verify the settings form works correctly

I added this need for a test to the issue summary.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

The patch makes sense to me! I agree tests for multilingual scenarios would be ideal :) I only found this nit:

+++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
@@ -92,6 +92,9 @@ public function createInstance($plugin_id, array $configuration = array()) {
+   *       There should be a 'settings_langcode' added, giving the language
+   *       code of the parent entity (entity view display or view that the
+   *       formatter is embedded in).

or panel or whatnot :) I think EVD and Views are just 2 examples?

Marking needs work for needing tests.

jhodgdon’s picture

Good idea. I'm working on a test now...

jhodgdon’s picture

FileSize
18.66 KB
840 bytes
953 bytes

So I'm working on a test for this... I used the UI to make some config objects for the test, and I noticed these problems:

a) When I created a Number (integer) field on my content type, the field setup asked me for prefix/suffix information, with descriptions:

Define a string that should be prefixed to the value, like '$ ' or '€ '. Leave blank for none. Separate singular and plural values with a pipe ('pound|pounds').

Define a string that should be suffixed to the value, like ' m', ' kb/s'. Leave blank for none. Separate singular and plural values with a pipe ('pound|pounds').

How come we are telling people to put plural forms in the prefix/suffix, and how will this possibly work for Russian for instance that has multiple forms?

b) I attempted to translate the settings I had set up for the content type and field. I could translate the content type and field name from within the content type / manage fields / etc. screens. But there are no links there that led me to translating the prefix/suffix/plural/etc. settings for the field.

So I went to Configuration Translation page. admin/config/regional/config-translation
There I found Form modes, but that only let me translate the label of the custom "Register" form mode for user.
Also found Content types, but that just let me translate the name of my content type.
Also found View modes, but that only let me translate the names of view modes.
Also found Content fields, but that only let me translate the label and description of my field.

So ... how do I even translate the plural settings to test this out?

I gave up at this point. Do we need to file a critical bug saying there is no way to translate this stuff? It surely seems critical to me. If I cannot translate it, there is no way to test it that I can see.

Anyway, just so this work I did is not lost, I'm attaching exports of various config items that I created thus far, in a tgz file. Contains a content type with an integer field, and the form/view modes for it, and the fields etc.

And here's an interdiff and new patch to fix the docs issue in the previous patch. Leaving at Needs Work for the tests.

jhodgdon’s picture

Regarding the prefix/suffix stuff... Here's the code that formats the number based on these settings:
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Field!Plugin!Fiel...

So it is not doing the right thing. It is asking people to put in | as the separator, so format_plural() will not be finding the right translated string for this.

I also do not understand why we need prefix/suffix at all if we have the ability to do a plural string thing.

In any case it's wrong and I don't know how to translate it still.

This is a mess.

jhodgdon’s picture

So regarding the prefix/suffix thing, I think the right thing to do is to take out the fake format_plural stuff there. The code that is in there is just plain wrong.

I filed a separate issue to deal with that: #2545730: Misuse of formatPlural() in Numeric field prefix/suffix

I am not sure what to do about the config translation thing. Am I wrong? Where do I go or how can I translate the entity displays and entity form displays? Do we need to file a critical issue to fix this? Having config that cannot be translated is not good.

jhodgdon’s picture

Status: Needs work » Postponed

Just filed #2546212: [PP-1] Entity view/form mode formatter/widget settings have no translation UI for the field/formatter/widget translation issue. I don't think we can write a test here until that is taken care of probably? At least I don't think so.

jhodgdon’s picture

Status: Postponed » Needs work

Actually we can probably test this in Views, which has a translation UI, so I think we can unpostpone this after all.

jhodgdon’s picture

The patch above doesn't apply in a couple of the migrate test parts by the way. Needs reroll I guess.

So. I did a bit of manual testing of the patch (without those two migrate segments) as follows:

a) Imported the config items in the tgz archive on #128, from the Single Import page admin/config/development/configuration/single/import
[You have to import them in the right order: node type, field storage, field, form mode, view mode]

b) Created several "Number test" content items, with numeric values 1, 2, 3, and 7. Verified they output correctly in English anyway.

c) Created a view, also using the format plural formatter. This worked fine too in English. [Export of this is attached here, for help in writing a test.]

d) Turned on multilingual modules and added Russian language (which has multiple plural forms). Note that you need to make sure the .po file is imported because that defines the plural forms, so if you do this in the UI make sure you have the Update module turned on.

e) Translated the view into Russian. The translation form is not working right -- it doesn't give me separate fields for the separate pieces, see attached screenshot.

Anyway, I set the translated field value to

1 ritem@count ritemi@count ritemo@count ritemu

because I don't speak Russian. That funny character is the \x03 character.

f) Tried to display the page from the View. The View Page link is broken, and I filed a separate (unrelated) issue on that. #2561943: Views UI - View Page link on Page display is broken

g) So went to the URL manually. The format plural stuff does appear to be working, because I see 3 different plural forms displayed: ritem, ritemi, ritemo for 1, 2/3, and 7 respectively.

So it looks like the form needs some attention. [item e]

jhodgdon’s picture

Note on the translation of the View into Russian: you cannot see the separation character in the output on drupal.org... There is a \x03 character before each @ sign in (e). In the translation UI, you can see the character and use copy/paste to paste it. Also I don't know how to export my translation of the view, to help in writing the test.

And I'm out of time for this issue, sorry! Hopefully someone else will work on the test.

Gábor Hojtsy’s picture

@jhodgdon: yay! Re the translation UI not working properly:

+++ b/core/config/schema/core.entity.schema.yml
@@ -268,33 +268,36 @@ field.formatter.settings.language:
+    format_plural_string:
+      type: label
+      label: 'Singular and one or more plurals'

Should now use the plural_label type not label. That would fix the translation UI. (This type was made available recently with support in config translation).

yched’s picture

I'm very unkeen on munging settings_langcode inside the 'settings' array :

- The 'settings' array is strictly the domain of each formatter, we explicitly do not put fixed, always-there-for-all-formatters options supported by the Field API in there (e.g 'label display on / off' is not in settings, but is its own option one level above). Formatter options are [fixed options like weight, label display...] + [an array of "settings" entirely up to the formatter]
I'd strongly vote to keep that pattern, it is very structuring throughout all of Field API (widgets, formatters, field types), and it provides future extensibility (i.e we can later add a Field API level option without worrying that it clashes with a setting of some random formatter somewhere - which were issues we had in early versions of CCK where we didn't have this separation).

More specifically in this case : the "language of the settings" is meta-information about the settings, and should be separate from the settings themselves. Munging "payload" and "info about the payload" in the same bag is weird.

- Unlike the rest of 'settings', it is not meant to be stored in config, but is derived from the config (it is the langcode of the config entity that holds the formatter configuration). I.e, it is already stored in another entry in the config record, we don't want duplicate storage.

I know it makes the patch much shorter since it doesn't require modifying Formatter __construct(), but it opens a gap we intentionally closed long ago in CCK.

jhodgdon’s picture

First off... The settings_langcode is not stored in the settings array. However, unless we make a huge number of alterations to the code and API (which is probably not even possible at this point), we (a) need to pass it to the formatter code and (b) don't have an argument to pass it in with.

The problem is that formatters are not stored as their own config items. Formatter config is stored as part of either views config objects, entity display mode config objects, or some other config object defined by a contrib module (like Display Suite displays for instance). The language code is on the containing config object.

So, when the formatter is doing its thing (either putting up an edit form or formatting the data), it doesn't have access to "the language code of the config object that actually contains me" or "the config object that contains me". But it needs the language code information from the parent config object.

We have discussed how to pass in this language code in quite a few of the already 137 comments on this issue, and not come up with a better way to do it.... If you have a better idea that will work in code, I'd love to see it, but ... I think we are stuck with this.

yched’s picture

The settings_langcode is not stored in the settings array

OK, not stored, but placed there at runtime (and I guess filtered back out before saving ? even though I don't see where that happens). That has the same drawback of saying Formatters "you don't own what's in the settings array anymore, Field API can place its own things in there", which is a structuring contract I'd really hate to break for the reasons explained above.

The problem is that formatters are not stored as their own config items. Formatter config is stored as part of either views config objects, entity display mode config objects [...] When the formatter is doing its thing (either putting up an edit form or formatting the data) [...] it needs the language code information from the parent config object

Yes, it is the job of the code that instanciates a Formatter object to pass the langcode of the config it fetched the formatter configuration from. No way around that, but that stays true whether that langcode is a separate constructor param or munged into the $settings array ?

We have discussed how to pass in this language code in quite a few of the already 137 comments on this issue, and not come up with a better way to do it.... If you have a better idea that will work in code, I'd love to see it, but

The patches I posted until #88 did that with a separate constructor argument. That seemed to work ? Sorry for not having been able to follow the issue since then, can you point me to the comments where this doesn't fly ?

jhodgdon’s picture

It looks like I did it in #120. Maybe that was the wrong choice, but the patches in between were doing both (adding to settings and also to the constructor) for some reason, and it seemed simpler to have only one of the two methodologies.

mpdonadio’s picture

I think it would be more accurate to say that the patches before #120 were adding to the configuration and not settings. I recall the IRC conversation w/ @jhodgdon, but not he details. There is a chance that I have this logs saved, but I trashed a bunch of old ones the other day.

The divergence started to address the @todo in #88 in FormatterPluginManager::createInstance(). The proper solution to this was brought up in #116, which involves adding an entry to the base entity schema, and then would require editing a lot of YAML to add in the new entry to configuration sections (unless I overlooked something in the schema hierarchy to let this be inherited).

I would be happy to reroll this from #116 and start to add the config and get the patch green, but in addition to better test coverage for the patch in general, we would need an update hook and update tests. I think the upgrade path for this may be a bear to take on.

Another option would be to reroll from #98, add test coverage, and handle the @todo in FormatterPluginManager::createInstance() as a followup. But, I think that still needs an (much simpler) upgrade path / upgrade test because of the changes to core.entity.schema.yml.

yched’s picture

Thanks for the history rewinding, @mpdonadio. I'd vote to start from #98 again.
The @todo in there should be pretty harmless, and I don't think it derails the approach in #98.

#116 has this snippet, which is IMO very very weird :

@@ -70,6 +70,9 @@ core.entity_view_display.*.*.*:
           settings:
             type: field.formatter.settings.[%parent.type]
             label: 'Settings'
+          settings_langcode:
+            type: string
+            label: 'Language code for settings'
           third_party_settings:
              type: sequence
              label: 'Third party settings'
@@ -123,6 +126,9 @@ core.entity_form_display.*.*.*:
             label: 'Third party settings'
             sequence:
               type: field.widget.third_party.[%key]
+          settings_langcode:
+            type: string
+            label: 'Language code for settings'
     hidden:
       type: sequence
       label: 'Hidden'

The settings_langcode shouldn't reach storage, it is a derived information (the top-level langcode of the config record that holds the formatter configuration) added to the formatter $configuration at runtime, we don't want to save it back as part of the formatter configuration, especially since on the next read we would ditch the value and take the top-level config langcode anyway. So that duplicate storage would be useless only adding confusion and potential inconsistencies.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

OK, I am going to reroll from #98 and make sure that it comes back green. We can then work on the tests and upgrade path. Should have something tonight or tomorrow.

jhodgdon’s picture

Hm. You might want to hold off a bit. We're having some discussions on #2545730: Misuse of formatPlural() in Numeric field prefix/suffix, especially the last few comments starting at #55, about what we should really be doing here.

mpdonadio’s picture

OK, I'll do a straight reroll, make sure it's green, and then let the dust settle on the other issue before proceeding.

paresha.uchdadiya’s picture

Patch Rerolled #128.

mpdonadio’s picture

#145, we want to reroll the patch in #98 and work off of that one.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
42.14 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,578 pass(es), 2 fail(s), and 0 exception(s). View

Reroll from #98

$ git checkout 2449597-98
$ git rebase 8.0.x
First, rewinding head to replay your work on top of it...
Applying: 2449597-format_plural-88.patch
Using index info to reconstruct a base tree...
M	core/config/schema/core.entity.schema.yml
M	core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
M	core/lib/Drupal/Core/Entity/EntityViewBuilder.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/LanguageFormatter.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
M	core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
M	core/modules/field/src/Tests/Number/NumberFieldTest.php
M	core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
M	core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
A	core/modules/migrate_drupal/src/Plugin/migrate/process/d6/FieldFormatterSettingsDefaults.php
A	core/modules/migrate_drupal/src/Tests/d6/MigrateFieldFormatterSettingsTest.php
M	core/modules/views/src/Plugin/views/field/Field.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/views/src/Plugin/views/field/Field.php
Auto-merging core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
Auto-merging core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
CONFLICT (content): Merge conflict in core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
Auto-merging core/modules/field/src/Tests/Number/NumberFieldTest.php
Auto-merging core/modules/field/src/Tests/Migrate/d6/MigrateFieldFormatterSettingsTest.php
Auto-merging core/modules/field/src/Plugin/migrate/process/d6/FieldFormatterSettingsDefaults.php
Auto-merging core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/LanguageFormatter.php
Auto-merging core/lib/Drupal/Core/Entity/EntityViewBuilder.php
Auto-merging core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
Auto-merging core/config/schema/core.entity.schema.yml
Failed to merge in the changes.
Patch failed at 0001 2449597-format_plural-88.patch

Conflict b/c #2529560: Expand support for link objects. Simple update to the ImageFormatter constructor.

$ git rebase --continue
Applying: 2449597-format_plural-88.patch
Applying: Added $settings_langcode to DateTimeTimeAgoFormatter
Applying: Added $configuration[settings_langcode] to DateTimeTimeAgoFormatter
Applying: #90
Applying: #73-1
Applying: #73-2
Using index info to reconstruct a base tree...
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/LanguageFormatter.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
M	core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
M	core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
M	core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
Auto-merging core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
Auto-merging core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/LanguageFormatter.php
Applying: #73-4
Applying: #73-5
Applying: #73-6
Applying: #73-7
Applying: #53-4
Applying: #53-5
Applying: #58-6
Applying: #58-7
Applying: #58-8

Yeah, I like lots of microcommits when addressing feedback. :)

Status: Needs review » Needs work

The last submitted patch, 147: number_formatters_make-2449597-147.patch, failed testing.

mpdonadio’s picture

Ok, I don't get why the assertion on line 502 fails, but the one on line 487 passes. The problem is that the ' is being escaped:

Submitted by aVhPi0ib on Sun, 09/13/2015 - 04:59
ix4jhdd0
bHIJLxiP360,421.00000000xzwvx1ju
atpue9tf
77'584 items

<div class="field field--name-atpue9tf field--type-integer field--label-above">
    <div class="field__label">atpue9tf</div>
              <div content="77584" class="field__item">77&#039;584 items</div>
          </div>

  </div>

The fact that $thousand_separator is random also makes this harder do debug and a ticking time bomb...

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
42.45 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,578 pass(es). View
1.01 KB

Fixed fail in the test from reroll, but didn't do any other work. Randomness in tests needs to be seriously reevaluated. That test has a ticking time bomb that should probably be addressed in a followup at some point.

Assuming this comes back green (it should), do we want to postpone on #2545730: Misuse of formatPlural() in Numeric field prefix/suffix?

mpdonadio’s picture

+++ b/core/modules/field/src/Tests/Number/NumberFieldTest.php
@@ -7,6 +7,7 @@
 namespace Drupal\field\Tests\Number;
 
+use Drupal\Component\Utility\Html;
 use Drupal\Component\Utility\Unicode;
 use Drupal\simpletest\WebTestBase;
 

Blerg. That `use` isn't needed and is cruft from trying to debug the fail. It should be removed.

jhodgdon’s picture

Status: Needs review » Postponed

So I think we should postpone this on #2545730: Misuse of formatPlural() in Numeric field prefix/suffix which will probably more or less adopt this patch with an upgrade path to the existing non-working prefix/suffix stuff.

jhodgdon’s picture

@yched, @mpdonadio I am goin to assign the other issue to myself, because *someone* needs to make a patch. BUT if you want to do it instead, feel free to take it.

jhodgdon’s picture

Note: Anyone who contributed patches/reviews here who has not yet commented on #2545730: Misuse of formatPlural() in Numeric field prefix/suffix, please do so now so you can be credited there. The patch there will incorporate this patch and you should get credit.

Specifically:
mpdonadio, pcambra, effulgentsia, rteijeiro, dawehner
please go comment on that other issue.

jhodgdon’s picture

Status: Postponed » Closed (duplicate)

#2545730: Misuse of formatPlural() in Numeric field prefix/suffix now has a viable patch that includes this functionality, so I am closing this issue as a duplicate.