Misuse of formatPlural() in Numeric field prefix/suffix

I grepped for format_plural and formatPlural in Core and there are additional places it is being misused in a similar manner, excluding tests and test classes. I found two:

a) \Drupal\Core\Validation\DrupalTranslator::transChoice()

    // Violation messages can separated singular and plural versions by "|".
    $ids = explode('|', $id);

    if (!isset($ids[1])) {
      throw new \InvalidArgumentException(sprintf('The message "%s" cannot be pluralized, because it is missing a plural (e.g. "There is one apple|There are @count apples").', $id));
    }
    return \Drupal::translation()->formatPlural($number, $ids[0], $ids[1], $this->processParameters($parameters), $this->getOptions($domain, $locale));
  }

Which means that uses of transChoice() are also suspect.

The only call to transChoice in Drupal space (outside of vendor) is in \Drupal\Core\TypedData\Validation\ConstraintViolationBuilder::addViolation().

So this means that calls to addViolation are suspect if they are passing in messages with | in them to indicate singular/plural.

I looked at these, and they all seem to be passing in messages from \Symfony\Component\Validator\Constraint objects or extensions. Which doesn't actually have a message method or member name, but that aside...

So I looked through all the classes that extend this class, and their messages. I do not see any of them actually using this | for singular/plural functionality. My feeling is that we should remove this functionality then... Probably should spin off to a different issue.

b) \Drupal\Core\Datetime\DateFormatter::formatInterval()

      $key = explode('|', $key);
      if ($interval >= $value) {
        $output .= ($output ? ' ' : '') . $this->formatPlural(floor($interval / $value), $key[0], $key[1], array(), array('langcode' => $langcode));
        $interval %= $value;
        $granularity--;
      }

This is also kind of wrong, except that there is some special code in the PO builder that puts these strings in explicitly for formatInterval(). The newer method formatDiff() has made a better choice by putting in literal strings. This one would be easy to fix by putting in literal strings here instead of $key[0], $key[1]. But anyway it isn't broken because of the special PO builder code we have in place.

The point is that the Drupal transChoice() method is overriding what Symfony is doing with a hacky way of trying to allow plurals, which would never work.

It is OK with me to split this into two issues, but the solution is the same for both: remove a few lines of similar, hacky code.

And I do not think we need to add tests for behavior we are removing. It would never of worked correctly with translations, and obviously it was never tested with actual examples. But if we are removing this behavior, we shouldn't also add tests for it. ?!?

Fixing issue link in summary, and removing mention of transChoice() from this issue. I'll do a quick copy/clarity edit of the CR also. Will review patch next.

There's one thing I think we need to fix in the patch:

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
@@ -75,10 +75,8 @@ public function viewElements(FieldItemListInterface $items) {
-        $prefixes = isset($settings['prefix']) ? array_map(array($this, 'fieldFilterXss'), explode('|', $settings['prefix'])) : array('');
...
+        $prefix = isset($settings['prefix']) ? $settings['prefix'] : '';
+        $suffix = isset($settings['suffix']) ? $settings['suffix'] : '';

We've lost the fieldFilterXss() call that was in the original lines here. That probably needs to be replaced?

Other than that, I think the patch looks correct, thanks!

Looks good to me!

Good questions. Ideally, we should probably do a hook_update_N() to fix this up, although it's a bit complicated. We'd need to do something like this:

a) The setting for the prefix/suffix is part of the field config, so we'd need to find numeric field configs with prefix/suffix that actually contain a | character.

b) We'd then want to remove everything after the | character (and the | character itself) from the prefix/suffix field config setting, so that the widget output would be the same as it was previously, because this is what the patch is doing:

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/NumberWidget.php
@@ -100,12 +100,10 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
 
     // Add prefix and suffix.
     if ($field_settings['prefix']) {
-      $prefixes = explode('|', $field_settings['prefix']);
-      $element['#field_prefix'] = $this->fieldFilterXss(array_pop($prefixes));
+      $element['#field_prefix'] = $this->fieldFilterXss($field_settings['prefix']);

So it basically used to display just the part before the |

c) The formatters are really complicated to update though. If they are in a 2-plural language like English, it's not too horrible, but even there the algorithm would be something like: In any formatters for this field in any view mode (is it even possible to find them??), if they have the "Display prefix/suffix" setting checked, we would want to uncheck that, and instead make a format_plural that looks like [ here, $prefixes and $suffixes are explode('|', prefix/suffix setting ]:
singular: $prefixes[0] . ' @count ' . $suffixes[0]
plural: $prefixes[1] . ' @count ' . $suffixes[1]
then concatenate these with the special format_plural character and save as the plural forms config.

But if the language of the view mode/formatter config has nplurals != 2, I have no idea what the "right" thing to do would be for this case. So I'm not sure it's really possible to migrate this...

The other problem with (c) is that the other issue is blocked right now because we have no way to translate view mode/formatter settings in the UI at all, so there is no way to test (even manually) and see if it's actually working (and I have no idea really if it is or not). So we may never actually get that in before 8.0. As it's not Critical (and neither for some reason is the "you cannot translate this stuff" issue), it will not block release. But I don't think we should release with this issue's broken code in Core, so I don't think we should block fixing this issue on possibly getting that format_plural issue done.

So... My feeling is that maybe instead of (c) the better thing to do would be to just do (b) on this issue, and if any are found with | in them, warn the user that their formatters may need attention. Then hurry up and add the feature for format_plural on the other issue so that they can actually do something useful.

Thoughts?

Hopefully @catch will comment on the ides in #14 so we can decide on a direction.

Actually I don't think it's even possible to do #14(c). The reason is that field formatter config is not stand-alone. It can be embedded in Entity View Modes, Views, whatever the Display Suite module is using for its config, etc. I don't think there is a systematic way for an update function to find all embedded field formatters in existing config, at least not that I'm aware of.

So. All I think we can really do on this issue is:

a) The setting for the prefix/suffix is part of the field config, so find numeric field configs with prefix/suffix that actually contain a | character.

b) If there are any, remove everything after the | character (and the | character itself) from the prefix/suffix field config setting, so that the widget output would be the same as it was previously, because this is what the patch is doing:

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/NumberWidget.php
@@ -100,12 +100,10 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen

     // Add prefix and suffix.
     if ($field_settings['prefix']) {
-      $prefixes = explode('|', $field_settings['prefix']);
-      $element['#field_prefix'] = $this->fieldFilterXss(array_pop($prefixes));
+      $element['#field_prefix'] = $this->fieldFilterXss($field_settings['prefix']);

So it basically used to display just the part before the |

c) If any field settings are found with | in them, warn the user that their formatters may need attention, which could be part of views or view modes or other configuration from contributed/custom modules.

d) Then hurry up and add the feature for format_plural on the other issue #2449597: Number formatters: Make it possible to configure format_plural on the formatter level so that they can actually make the desired output using the real format plural stuff.

Oh! We shouldn't change the behavior. My mistake -- we should keep the last part of the explode() to keep the behavior the same as it was before the patch on the widget. Let's see what you did in the patch... So...

I guess I wasn't clear in #17 -- that is all what we need to do in a hook_update_N() function, which should end up with there being no | left in the field settings configs, and the user being warned after the update if there were some that got transformed. And I guess since it was array_pop() in the widget, we should do the same in the update function.

And then in NumericFormatterBase and NumericWidgetBase, we can just get rid of all the explode() logic and just use the field settings prefix/suffix without any transformation (except for running it through XSS filtering).

Right?

Yes, I suppose we could fix up Entity View Modes and Views where they use Numeric field formatters, in an update function, and then also still warn the user that they need to take a look for other uses. But the only way to do that is to have functionality to migrate it to, which doesn't currently exist until #2449597: Number formatters: Make it possible to configure format_plural on the formatter level is taken care of. Unfortunately, I am not sure that will ever get done. It's currently waiting on tests, which are actually quite difficult to write for that issue, and actually it hasn't even gotten a manual "yes this is working" test yet.

On the other hand, this issue is a major i18n API misuse bug, and is removing some code/functionality that never would have even worked for anything but an English-only site. Leaving code like this in the code base will lead to people copy/pasting it elsewhere, and leading to more broken code.

So my feeling is that we should not postpone this issue on that other one. I think we should do what we can do now on this issue, and if the other one gets fixed we can go back and fix up the update code here to migrate the stuff for people who haven't updated yet.

Great. I think we should also warn the user that we are doing that. And this should be in an update function.

And then in the Formatter and Widget we would just ignore any | in there, and print out the prefix/suffix as-is (after XSS filtering).

Wow, looks great!

Only a couple of minor things to fix:

1. +++ b/core/modules/field/field.install
   @@ -0,0 +1,65 @@
   + * Install, update and uninstall functions for the field module.
   

   Nitpick: Comma needed after "update". And "for the Field module" (capitalize Field).

2. +++ b/core/modules/field/field.install
   @@ -0,0 +1,65 @@
   +  // Give the user the name of the prefix/suffix fields that where adjusted.
   

   where => were

3. +++ b/core/modules/field/field.install
   @@ -0,0 +1,65 @@
   +      '@prefixes' => implode(', ', $adjusted_prefix) ?: 'none',
   +      '@suffixes' => implode(', ', $adjusted_suffix) ?: 'none',
   

   'none' needs to be translated.

4. +++ b/core/modules/field/field.install
   @@ -0,0 +1,65 @@
   +    $msg = 'Some of your prefix/suffix fields configured pipe symbols to format singular/plural form. This is no longer supported, so the fields were adjusted. See the change record https://www.drupal.org/node/2552871 for more information.';
   +    $msg .= '<br\>Prefix fields adjusted: @prefixes';
   +    $msg .= '<br\>Suffix fields adjusted: @suffixes';
   +    $message = \Drupal::translation()->translate($msg, $args);
   +    return $message;
   

   Um. This will not work for translation:

   a) You cannot pass variables into translate(). You have to pass in a literal string. So don't make $msg a variable, just put it all into the call to translate().

   b) I think in order for the POTX extractor to find this, you really need to use the t() function wrapper and not Drupal::translation()->translate(). At least, that has been my understanding.

Looks good to me, and I think everything has been addressed. Updated summary slightly.

We have a separate issue to add back an *actually working* feature for prefix/suffx/plurals:
#2449597: Number formatters: Make it possible to configure format_plural on the formatter level
because the order of what goes into prefix and what goes into suffix is not even always the same across languages.

We can merge this issue back into that other one, and then we can do the migration of settings there.

Well that is interesting. So are you saying that we should consider instead of #2449597: Number formatters: Make it possible to configure format_plural on the formatter level, putting the plural stuff as a field setting and not on the formatter?

Well. OK. Let's step back a bit. Here's the current state of affairs:

a) Prefix/suffix are defined in Field settings, entered and stored with | as the separator between plural forms in an implied array. [Note that if we're going to allow plurals, we really should be using a full-fledged formatPlural() input widget for this setting, which would let you enter the right number of plural forms based on the config language, rather than asking you to enter singular/plural forms separated by | and assuming there are 2 forms in every language, which is incorrect.]

b) In the Widget, it picks out the last element of the implied array of plural forms and uses that for the field prefix/suffix when you edit the data. Since you can only display 1 form and you don't know the number (someone is editing it, after all), this is probably OK, but it is not clear from the Settings that this is the form that will be displayed on the widget.

c) In the Formatter, if the "use the prefix/suffix" box is checked, it tries to do formatPlural() by passing in the first and second elements of the plural forms array. This has problems because (1) the separator for translation in formatPlural should be some special character and not | so translations will not be picked up by formatPlural() and (2) the formatter code only picks out the first and second of the implied array of plural forms stored in the settings, and some languages have more and (3) I think if you create your field settings in language A and translate into language B (neither of which is English), ... probably this will not work at all.

d) For integer database fields with Views integration (NOT entity fields or entity base fields like comment count), we have always had the ability to format them in Views with a formatPlural formatting using @count. This functionality was also applicable to numeric entity base fields earlier in the D8 cycle but got dropped when base fields were converted to use entity field formatters. We are trying to add this functionality back in #2449597: Number formatters: Make it possible to configure format_plural on the formatter level as functionality on the Entity Field Formatter for numeric fields.

So.

My read of this is that:

1. Prefix/suffix for the widget are kind of like the Label for non-numeric fields. Which means they probably belong on the Field Settings like the main field label.

2. The widget doesn't need or want plural forms. It cannot know what the @count is. It also needs to have something that is translatable, which means prefix/suffix really need to be separate strings by themselves, not munged into a | array.

3. For translation of prefix/suffix for the widget/field settings, the prefix/suffix should be stored and translated together, so that you can transpose them or whatever is needed for different languages. Or they should be stored as a 'prefix @field suffix' string or something like that.

4. For the formatter, prefix/suffix by themselves don't really make sense because they would both need to be plural forms and probably to translate the plural forms you'd want to start with "prefix @count suffix". Also if you're going to do plural forms, you need a real formatPlural string with @count in there and the right number of plural forms stored with the right separator. So for formatting, you might as well drop the "use the prefix/suffix" checkbox and just have this setting on the formatter, as in #2449597: Number formatters: Make it possible to configure format_plural on the formatter level.

Thoughts?

I think that the suggestion of #56 makes sense, with the caveat that anything with @count in it needs to use the more or less standard UI of separate fields to enter each plural variant in the edit form, and have the class in question munge them together with the standard formatPlural variant separator, so that we can actually translate them and actually use formatPlural.

And I think a string where you insert @count makes more sense to me than separate prefix/suffix strings, because of the "what to put in prefix and what in suffix" ordering (the placement of @count), as well as the words, can definitely change with both language and number (which is why we do plurals that way in the first place).

OK. So should we do this then:

a) Take prefix/suffix off the Field settings.

b) Replace it with a formatPlural() thingy where a user would have N fields to enter information in (for a language with N plural variants), and in the UI they would enter things like:
$@count
1 fish
@count fish
etc.
The field settings code would take the UI and paste it together with the correct formatPlural sep character.

c) Migrate the settings to this new thingy.

d) Widget would have a checkbox to display the prefix_suffix_thingy, and if so, it would choose the variant for the number 2, probably using formatPluralTranslated to do it right since the config would already be translated.

e) Formatter would have a checkbox to display the prefix_suffix_thingy, and if so, it would use formatPlural or formatPluralTranslated to do it right.

f) Formatter would also have a checkbox to toggle whether to use the Field Settings thingy value or its own.

Thoughts? Sorry for the word "thingy" here but I couldn't think of anything else at the moment. ;)

One more thought. If we omit #60-f, and leave that for an 8.1 feature request (along with, as yched suggested, also letting the formatter override the Label, duh), I think it would be ***way*** simpler and satisfy most of the objectives for most users:
- Restore the ability to do formatPlural on the numeric base fields like comment count
- Not misuse formatPlural

I believe you are right on both items in #62.

We need to revive this issue and get it done, mostly with the patch in #2449597: Number formatters: Make it possible to configure format_plural on the formatter level

I am willing to try to make a patch for this, given that several others already have done a bunch of work on it. So assigning to myself, but if someone else wants to do it, feel free to take it and assign to yourself!

I will probably not work on it for a day or two at least, and I'll comment here when I actually am ready to start it so that we do not duplicate effort.

That sounds like an excellent idea.

OK, I'm actually going to start working on this now. Should hopefully have a patch tomorrow or soon anyway.

Updating the issue summary first, with the actual plan from #60/#62.

OK. Here is a first working patch, following the proposal currently in the issue summary.

I've tested manually that I can:
- Make a numeric field on a node and give it (in English) a plural formatting option
- Set the widget either to use that or not use it as field prefix/suffix
- Set the formatter in a view mode either to use the field prefix/suffix thing, override with its own, or not use prefix/suffix formatting.
- Tested the formatter in Views too

I haven't done:
- automated tests
- updates/migrates
- internationalization testing

So... I thought I'd upload this patch to see if @yched or anyone else has feedback about the approach. And also to see what breaks. I do not expect that this will pass current tests. At least, I hope it doesn't, because that would mean that the previous functionality wasn't tested at all, which would be sad.

Let's see what happens... Oh, a few notes/questions on the patch:

1. People who worked on #2449597: Number formatters: Make it possible to configure format_plural on the formatter level patches should be credited here! I got a few but not all have commented on that issue (I've asked them to comment). We're missing
mpdonadio, pcambra, effulgentsia, rteijeiro, dawehner

2. As per @yched / #66, I did not change the formatter constructor, but am passing in the settings language code in the formatter settings as _settings_langcode .

3. The numeric field formatters Integer, Decimal, Etc. - these changed because some settings defaults moved to the base class.

4. Both the Formatter and the FieldType needed to have format plural labels generated. Rather than add two more copies of how these labels are generated (see #2499639: Use correct labels for numeric fields when using a multiple plural forms language -- there are already 4 in Core), I added a method to StringTranslationTrait to generate standard labels. Eventually (on #2499639: Use correct labels for numeric fields when using a multiple plural forms language) those other 4 classes that generate these labels should be changed to use the new method, but that seems out of scope for this issue, whereas "only add 1 copy" instead of "add 2 more copies" seemed in scope for this issue. I hope.

5. In order to do both things like thousands separators and decimal points, plus prefix/suffix, in the Formatter, I had to modify the PluralTranslatableMarkup::render() method so that the value it substitutes in for @count could be passed in as an argument to override the default value. So that will allow you to have formatted output like:
$100,000,000.00
which I think is a very common use case, rather than saying "You can either have prefix/suffix that will work and with proper pluralization, or you can have thousands separators but not both", or worse yet having to kludge something together after doing the formatPlural() stuff to then str_replace '100000000' with '100,000,000' or whatever.

Oh it might help if I actually uploaded the patch. Doh!

That's fine. The next time I would have time to work on this is Tuesday (holiday weekend here). Also if you want to take over as "patch maker" instead of "reviewer" again, feel free! This one does need more work (tests, update function, migrate updates). I just decided to start with the code patch (cobbled together from this issue's previous patches and the other one that you worked on). First make sure this is working (at least in English, which it is now) and go from there after seeing what it breaks.

(adding credit for mpdonadio from the other issue)

Adding note to summary about patch credit.

Thanks for your review yched!

Yeah, probably will not be "before 8.0.0 release", but should be OK for 8.0.1 or something. But I am not sure, because the current prefix/suffix stuff is not workable... it's a fairly major bug and I personally think that it would probably be easier to fix config-affecting stuff before the first 8.0 release than after, when people might have more stuff exported to files. But I don't make those decisions. Anyway we have to get to a working patch first, and it should be 8.0.x branch at least, so I think I will continue working and if the patch doesn't get committed for a while, at least it will be ready to go when that time comes.

Anyway. Yeah, update/migrate/tests still need to be done (as noted in #71). States for hiding the options would also be good, yes. And the rest of your comments are excellent suggestions. I'll fix them hopefully in the next round, or at a minimum put @todo in to remember to do them later... also need to fix the 73 test failures...

Here's the next pass. I addressed some of the test failures, hopefully, by fixing up the schema for the formatter and widget and updating a couple of tests and stuff like that. No doubt this will come up with new and better errors now! :)

Also (I hope) I've addressed the review comments in #78-#80, more or less.

Things still to do:

1. Fix the migrations. I think the tests will tell us what needs fixing.
2. Add an update function because the data model changes with this patch. This will also need tests.
3. Add a magic states thing for the numeric formatter settings form. (There is a new @todo in NumericFormatterBase that needs to be taken care of before finalizing this patch, but I don't know how to do it. Yet.)
4. Add tests for the new format_plural functionality, including how it is internationalized in a multiple-plural language like Russian, and some cross-language tests too with settings_langcode coming into play.
5. One thing from #78 I didn't do was item 8:
   

   The method should actually make sure that $settings['_settings_langcode'] is set, and set it as the interface langage if not (we don't want consuming code in formatters to have to do isset() checks on it). And then we should adjust the comment here accordingly :-)

   The problem is that FormatterPluginManager and also WidgetPluginManager currently know nothing about the current interface language. So we would need to dependency inject the 'language_manager' service here to get that information... I'm thinking that adding this new dependency at this stage is not a great idea, and instead we should just let the formatter/widget classes handle it? They need to know more about languages anyway, so I'm inclined to leave the logic there. Especially since the Widget doesn't currently need _settings_langcode (though I've added it as per your request on #78, which seemed quite reasonable.)

   I'm interested in your thoughts on this.

6. Oops. I'd already made the patch and interdiff and in giving it one more look before submitting this comment/patch, I noticed I need to delete the prefix_suffix lines near the patch hunks in core/modules/field/src/Tests/Migrate/d6/MigrateFieldFormatterSettingsTest.php so I'll take care of that the next pass. Tests will fail. ;)

Here's a new patch to fix up (hopefully) some test failures by updating exported config, plus item 6 from #82. I'll need to make a new update function in order to get a lot of the test failures done, but ran out of time this morning... anyway this should fix some of it.

OK, here's the next round. What this does:

a) Adds an update function. I'll still need to make an explicit test for the update function, but since a number of update tests were failing due to not having an update function, if they now pass that will at least be something... I don't seem to be able to get the tests to finish locally... not sure what is going on but let's see what the bot says. Probably not the last answer on the update function.

b) Added a test for the ability of the formatter to override the field setting for the prefix/suffix/plural thing, to the existing test for the ability to format numbers. I also fixed up formatter and widget tests. So now we have test coverage of the functionality, and the UI, in English anyway.

c) I noticed that if you use the UI to set up a formatter, the AfterBuild function in the formatter and field leaves the editing garbage around, so fixed that.

Still not done:
- Test for the update function (not sure if it really works either; most likely it needs modifications)
- Update the existing field/formatter/widget migrate code to migrate into the new settings, and make sure it's well tested
- Add a magic states thing for the numeric formatter settings form. (@todo in NumericFormatterBase)
- It would probably also be good to test the new UI and output in Russian or another language with more than 2 plural forms.

Wow, we got it down to only 1 fail! That's amazing. I can't believe the update function mostly worked in one try.

Hm. Looking at the interdiff in #89, though, I don't think it is correct to do that. I had adjusted that test for what we eventually want the test to do... we need to instead fix Migrate so it does the right thing, rather than testing for the wrong thing. I think anyway. I'll take this over again.

Regarding the 1 remaining fail, I will look into it... I may know what's going on.

Ah, got it. OK, I'll start from #91 patch then. Thanks!

Another pass at this. Things changed:

a) I updated the Migrate stuff. I am not sure why the migrate tests were passing without these changes though. That worries me, because I had already updated the migrate tests to verify that the result of migrating a field with a prefix/suffix was:

+++ b/core/modules/field/src/Tests/Migrate/d6/MigrateFieldInstanceTest.php
@@ -39,8 +39,7 @@ public function testFieldInstanceMigration() {
     $expected = array(
       'min' => 10,
       'max' => 100,
-      'prefix' => 'pref',
-      'suffix' => 'suf',
+      'format_plural_string' => 'pref@count suf',

How in the world was that test passing without this patch? I don't get it.

Oh. So I looked at the test results with "All" classes shown. For some reason ?!? this test is not running at all.

That is a problem. Why would this be?????

b) Replaced deprecated LOCALE_PLURAL_DELIMITER with its new constant on PluralTranslatableMarkup in a couple of places.

c) Fixed the latest test failures. The problem is that PluralTranslatableMarkup was assuming there is always an array element 1 in the array of plural forms. I changed it to use the last available element instead, which I think is a better idea anyway.

d) Added some additional tests for the formatting/widget with plural settings different for different plural forms.

e) Realized the Views part of the update function wasn't quite right for the case of having multiple copies of the same field in the same view display (which actually is pretty common to do for various reasons), and fixed that.

Still not done:
- Test for the update function.
- Figure out why MigrateFieldInstanceTest is not being run.
- Add a magic states thing for the numeric formatter settings form. (@todo in NumericFormatterBase)
- Test the plural forms UI. Right now the plural string is being set outside the UI in the tests.
- Test the plural forms UI and also formatter output in Russian or another language with more than 2 plural forms.

Oh. Doh. Regarding MigrateFieldInstanceTest, my patch introduced a PHP syntax error in that test. Quick fix for that. Not sure why the bot didn't report this? Ugh. This could be a real problem...

Mixologic pointed me to the full console output of that test and I apparently did something similar in NumberFieldTest, so that one also isn't running. Fixed that too...

So now let's see whether the bot thinks we're really done with existing tests!

Whoops. Went a bit wrong in the Views stuff. Not sure about some of the other test failures; let's see about this one.

OK... I think this should do better:

a) Fixed the migrate problem (the test passes locally anyway). Doh. Made same change to the code in the system_update_N() function too.

So assuming the bot agrees: the migrate part of this issue is done, and by updating the existing test, it also has been tested.

b) Fixed the NumberField test problem (passes locally now). Double doh. So that should take care of "make existing tests pass".

c) Added a new test for the plural settings UI for the field and the formatter. This doesn't yet pass locally -- some kind of error is making it take forever, so I'll see if the test bot can make sense of it. The widget is already being tested in the NumberField test too. So I think the UI for settings is tested adequately now, once this test is passing.

This is new file core/modules/field_ui/src/Tests/NumericFieldsUiTest.php.

Anyway, this is getting closer!! Still to be done:

0. Make the new test pass.

1. Test for the update function. It's somewhat tested by the existing update tests (at least that it doesn't crash, because they certainly did fail until that update function was added), but we need a more comprehensive test. To do this:
- Make a field, a view mode, a form mode, and a View using the pre-patch code and export them.
- Make a test that reads in this imported config, runs the update, and verifies that the output is as expected.

2. Add a magic states thing for the numeric formatter settings form. (@todo in NumericFormatterBase). This may also need a test (is that possible?), which could be added to the new core/modules/field_ui/src/Tests/NumericFieldsUiTest.php test if so.

I think I'll put this up for the bot now....

I'm not good with "magic states" so if someone else wants to do (2) that would be great! Either that or I can probably muddle through it next week.

If someone else wants to do (1) that is also fine. Otherwise that will be my next task, but it will not be until Monday next week at the earliest.

Great! Interdiffs look good to me and you got that test I mostly wrote to pass. Thanks!!

Still to do:

1. Test for the update function. It's somewhat tested by the existing update tests (at least that it doesn't crash, because they certainly did fail until that update function was added), but we need a more comprehensive test. To do this:
- Make a field, a view mode, a form mode, and a View using the pre-patch code and export them.
- Make a test that reads in this imported config, runs the update, and verifies that the output is as expected.

2. Add a magic states thing for the numeric formatter settings form. (@todo in NumericFormatterBase). This may also need a test (is that possible?), which could be added to the new core/modules/field_ui/src/Tests/NumericFieldsUiTest.php test if so.

I'll get back on this later today...

Back to me. I'm working on the update test.

I don't think I agree.... If you take a look at the update function, it's fairly complicated, as it is updating field configs, formatter configs on views and view modes, and form mode configs. I think it's best to test them all. Just making a node and verifying it can still be viewed is not very complete of a test.

So I opted instead to write a lower-level test that has all that exported configuration, runs the updates, and tests that the configuration is updated as expected. We have other tests now in this patch that verify with the patched config schema and various settings the output is formatted correctly and the form does what it should, so that should be good.

Actually it was good that I did this, because it found 2 bugs in my update function: one where it was not updating something correctly, and the other where I forgot to update form displays.

Anyway... here's the test but it has a problem in that some other update is not working correctly. Probably I need to export something with an older config schema, but I'm not sure what. All the time I have for today...

Ugh. Couldn't make a clean interdiff file with both the changes and the new files for the tests. Sigh. I am not good at interdiffs.

So things that changed were:
- settings.install - updates to the hook_update_N()
- New files added in core/modules/system/fixtures
- New test file added NumberFieldUpdateTest

Gotta run, uploading the new patch anyway sorry about the interdiff.

Thanks for the interdiff! Yes, that is what changed.

Working on the test failures... also I realized we don't need the field.storage.node.body file in there so that will be gone in the next patch.

So this is a mess.

You cannot just add configuration for fields to the config database, because there also are database tables that get created, and there's something in KeyValue storage that the field API uses to decide whether the field really exists or has been deleted too.

So I'm running into doing something like this to the test to create the database stuff, ugh. All just to create a numeric field and formatter and widget that I can test the update on. Plus I had to patch core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php so I could even see what the exception I was getting was -- that I filed as a separate issue: #2597548: Useless message thrown by SqlContentEntityStorage::wrapSchemaException(). And I don't even know if this is enough yet... but I'm out of time again.

Anyway this is what I have so far but I'm not sure if it works yet. In addition to this interdiff, I removed several of the text fixture export files that were in the previous patch -- anything with "body" in the file name I don't need any more, because I took that out of the field/formatter configs. Those removals are in effect in this patch, but not shown in the interdiff file.

Let's see what the bot says and eventually maybe my local test run will finish and I can maybe see what is going on from the verbose output if it does...

Doh. Simpletest UI is stupid about PHP syntax errors, and instead of crashing, goes into an infinite loop. ?!? Plus the bot still doesn't report these errors in a useful way, unless you look at the console output in plain text. ?!?

Anyway. Fixed that.... Working on getting the test to pass...

WOOHOOO! I got the update test to pass locally! It seems like kind of a hack, but then again everything in it is understandable I think.

So. I think the only thing left to do is the "magic states" thing:
Add a magic states thing for the numeric formatter settings form. (@todo in NumericFormatterBase). This may also need a test (is that possible?), which could be added to the new core/modules/field_ui/src/Tests/NumericFieldsUiTest.php test if so.

Meanwhile, uploading new patch to see if the bot agrees...

And lastly, here is the magic states bit. I've tested this manually... since it's a JavaScript states thing, I do not know if it's possible/desirable to test it with an automated test? Anyway it does work.

So I think this is ready for a Real Review (TM). Removing a couple of tags; note that yched has at least reviewed the approach and earlier patches, and is presumably following the issue now, so I don't think we need special subsystem maintainer review at this point.

Updating the summary to include concerns from #2449597: Number formatters: Make it possible to configure format_plural on the formatter level, which I have just closed as a duplicate.

Fix typos in summary and one place where I lost an issue link.

Highlight patch credit in issue summary... those people mostly haven't come over here to comment...

Also I'm writing a change notice, so that should show up in the sidebar shortly.

I think all that needs to happen on this issue now is a review, and then we can see about whether it should be 8.0 or 8.1 or what.

@yched - would you have time to review this patch?
@Gábor Hojtsy - and/or you?

Thanks! I think it's ready to go. @mpdonadio and I both worked on the patch extensively so we need a separate reviewer...

Requesting triage for RC vs. Postpone... adding info to summary.

Updating the summary again, and adding before/after screenshots.

Minor summary updates...

Thanks for the review in #127! Some thoughts (will do a new patch shortly):

RE item 1, if it's a new entity, we don't have a "previous value", but yes we could use the existing value if there is one. That would complicate the code somewhat. Is it worth it? The previous code this is replacing just popped off the last prefix/suffix, so this is compatible with it. I'm inclined not to invoke formatPlural code that is complicated. Thoughts?

I'll fix that comment though.

RE item 2, the reason is this: In formatting a number, you first apply things like decimal and thousand separator. So you take a number like 123456 and maybe format it as "123.456" for French or something. Then you need to do plural formatting. You need to pass the actual integer 123456 into formatPlural, but what you want to be substituted into @count is "123.456" not "123456".

I felt that the easiest way to accomplish this was to be able to pass in ('@count' => "123.456") into the formatPlural, rather than trying to find the integer that was substituted in after returning from formatPlural and doing some kind of string substitute on it. Besides which, now formatPlural delays casting to string so I think this is really the *only* way to get it done.

That's for the overrides. The other part came up as a test failure in the upgrade/migrate test, because you didn't always have plural forms at all.

RE item 3, consolidating is part of #2499639: Use correct labels for numeric fields when using a multiple plural forms language, but we could also spin off another issue there if we need to. I believe I already commented on that issue that as part of this one, I had to add two more plural forms and therefore made 1 function that generated the labels as a start in consolidating. Anyway it is definitely out of scope for this issue.

RE item 4, I will fix that comment.

RE items 5-6, I modeled this code on existing update functions, so yes I think it's OK.

Ok here is a new patch. I've updated various comments and API documentation to hopefully address the review comments in #127 (see also #128) and to avoid anyone else looking at this code down the road having the same questions.

I'm still open to changing the code for item 1 in #127 (making the prefix/suffix for the widget display the current number's plural form)... I don't think it's worth it, and the previous code was equally unsophisticated about it, but I am open to changing my mind.

I think that all of the other review items have been addressed. See what you think?

Hm. Looks like there's a new test since the previous patch was created, that needs updating.

That test starts with a new database dump for RC1, and then assumes there are no updates to run since that dump. This should fix it. I think. The tests are painfully slow on my local machine, so...

Looks like that did work. Ready for round 2 of reviews...

OK that's odd. That test that was a problem before is now failing again.. or maybe it's a new one that needs the same fix. I've added two more tests to see if I can reproduce the failure again. I may need to modify another one of those tests again. Sigh.

Oh I see. The test that ran on Oct 28 passed. But since then, something changed and the same patch now fails that test on Nov 2nd. I have no idea how to fix it now. Sigh... I am leaving on vacation in 2 days and will be gone until the end of November. I cannot fix this now. Sorry.

Hm. I guess this got stalled while I was away. I'm back now...

@mpdonadio - do you still want to work on it? One way or another, we should still fix this.

We need to revive this issue. It's the only thing left on #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality.