Problem/Motivation

The main bug of this issue

While trying to write a multilingual test for #2449597: Number formatters: Make it possible to configure format_plural on the formatter level, I noticed the following problem.

The prefix and suffix settings on number fields have these 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').

So it's asking you to use | to separate your singular and plural values. This in itself has problems, because only some languages have singular/plural values -- in many languages there is either only 1 form or there are more than 2, and in some although there are two forms, they don't correspond to "singular" and "plural".

So that is one problem.

Then, in \Drupal\Core\Field\Plugin\Field\FieldFormatter\NumericFormatterBase::viewElements(), there is a blatant misuse of formatPlural() and translations in general for these fields

        $prefixes = isset($settings['prefix']) ? array_map(array($this, 'fieldFilterXss'), explode('|', $settings['prefix'])) : array('');
        $suffixes = isset($settings['suffix']) ? array_map(array($this, 'fieldFilterXss'), explode('|', $settings['suffix'])) : array('');
        $prefix = (count($prefixes) > 1) ? $this->formatPlural($item->value, $prefixes[0], $prefixes[1]) : $prefixes[0];
        $suffix = (count($suffixes) > 1) ? $this->formatPlural($item->value, $suffixes[0], $suffixes[1]) : $suffixes[0];

The problems here:

a) formatPlural assumes that it is getting English text. Here it is actually getting the already-translated text from the config object (translated into the interface language, or the entity language, or whatever). So calling formatPlural() on that will be adding some non-English text into the translation database as English sources. And trying to translate this text, which is already translated, is incorrect/pointless anyway.

b) formatPlural assumes it can translate the English input by pasting it together with a special string that is NOT |, and look up the translation to the target language, which might have n != 2 plural forms. Here, it is only getting two forms from NumericFormatterBase::viewElements() (no matter what language it is or how many forms someone might have entered, separated by |, in the config). Plus, the config string that is the source has | as the separator, not the usual character.

So even if it was always passed in as English (which it isn't), the source string would be something like "Singular|Plural" in the translation database (that is what is stored in the English source config), but formatPlural would be trying to look up something like "Singular[CHAR]Plural", where [CHAR] is the special plural form separator that formatPlural uses, in its database, and it would never find the translation of the configuration.

c) Once those plural forms are obtained from translation, formatPlural will attempt to use the plural rules for the target language to make the correct string. But this will not work here, even if you pass in the translated forms, because for instance, in Russian you need 3 forms to do that and you'll only have passed in 2.

So this is just plain wrong. It may work for English-only sites, but it doesn't work for sites whose main language has != 2 plurals, and definitely will not work with translation at all.

Functionality restoration

Drupal 7 had prefix/suffix capability on numeric fields, so we do not want to lose the capability of having prefixes and suffixes on numeric fields. Otherwise, we could just remove the prefix/suffix fields that do not work and be done.

Also, Drupal 7 had the ability for numeric fields in Views to have plural formatting, which we lost for entity fields when we did #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency. So we had issue #2449597: Number formatters: Make it possible to configure format_plural on the formatter level, but then we decided on that issue that we would mark it as a duplicate of this issue, because it doesn't really make sense to have both prefix/suffix and the format_plural stuff; they pretty much do the same thing. Also we would need to have update/migrate functions that take old prefix/suffix things and migrate/update them to the new format_plural things. So the issues were better combined into one.

So besides the bug described above, this issue is also about restoring some functionality that we had in Drupal 7 and early versions of Drupal 8, which allowed numeric base fields on entities to be formatted with plural formatting in views.

Proposed resolution

a) Take the current, separate prefix/suffix settings out of the Field settings. Replace it with a single setting that supports formatPlural() properly. 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 to save in the settings, and extract the variants into the UI to display to the user.

Here are before/after screen shots of the field settings... Before this patch:
Field settings for numeric field before patch
With the patch:
Field settings for numeric field with the patch

We'll also need to update the previous prefix/suffix settings on the field to this new format-plural setting, using a hook_update_N() function.

b) The widget for numeric fields would have a checkbox to display the new format-plural setting as a prefix and suffix. Similar to what it is doing now with prefix/suffix, it would choose the variant for the last plural setting. Note that the widget currently (without the patch) does not have an option for this -- it is always printing out the prefix/suffix no matter waht -- so for update purposes this setting should default to TRUE.

Screen shot of this setting, with the patch (without this patch there is not a setting for this):
Field settings for numeric field widget with the patch

And here is what the editing screen looks like, with this patch or without it (that doesn't change, assuming the option is checked with the patch) -- it displays the plural form prefix/suffix before/after the field:
Data entry form for numeric field with or without patch

c) The formatter for numeric fields would have a checkbox to choose whether to:
1. Use the format-plural setting from the field
2. Use its own format-plural setting (similar UI to Field setting)
3. Not use format-plural
If 1 or 2 is chosen, it would use formatPluralTranslated to do it right, since the config would already be translated.

Here is what the settings look like before the patch:
Formatter settings for numeric field before patch
And with the patch:
Formatter settings for numeric field with patch
And here is what the formatted field output looks like (with or without the patch; this doesn't change):
Formatter output for numeric field with or without patch

Updating: The formatter currently just has a checkbox of whether to use or not use the prefix/suffix, so that would need to be migrated into "Use the format-plural setting from the field" or "Not use format-plural".

d) We'll need tests for the field settings, widget, formatter, and the update, and we'll need to have the patch update stored Views and View Mode and Form Mode configs, although there may not be many that are affected.

e) We'll also need to update the migration from D6/7 so that it works with the new settings.

Remaining tasks

1. Make a patch. [done -- see screenshots above]

Patch Credit: This patch incorporates most of the patch from #2449597: Number formatters: Make it possible to configure format_plural on the formatter level so we should credit the people who put in work on that patch here as well. These include:
mpdonadio, Gábor Hojtsy, yched, jhodgdon, pcambra, effulgentsia, rteijeiro, dawehner
If they comment here, they should be credited here.

2. Write a change notice. [done]

3. Review/commit.

User interface changes

Non-workable prefix/suffix pluralization stuff will be removed and replaced with working pluralization for field formatters and widgets, restoring functionality that was present in Drupal 6.

API changes

No. Some additions.

Data model changes

The configuration schema for numeric fields, widgets, and formatters changes (with an update function). The old schema model was not actually workable, as described above.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the current usage of formatPlural() in prefix/suffix of numeric field is wrong -- it will not work correctly with translation. Also a duplicate issue was marked Task because it is restoring functionality for formatPlural on views numeric entity base fields, which was lost in a previous issue.
Issue priority Major because it is taking over #2449597: Number formatters: Make it possible to configure format_plural on the formatter level as well, and that was replacing format_plural functionality on numeric Views fields that was present in Drupal 7 and lost in Drupal 8 with #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency.
Prioritized changes The main goal of this issue is a bug fix -- removing wrong usage of formatPlural().
Disruption Shouldn't be much. Update will be provided. But the field, formatter, and widget config schema does change, so exported config will need to be updated as well.

RC Triage

See also Beta table above, which explains the disruption/status/etc. issues.

I think that this should go in sooner rather than later, to minimize disruption. It changes the config schema for numeric fields, formatters, and widgets. There is an update function provided, which will take care of active config, but that doesn't automatically take care of config in config/install and config/optional directories. To minimize the impact on contrib, it seems like stabilizing this configuration earlier is better.

There are not API changes, so other than exported config files, there shouldn't be disruption.

Files: 

Comments

jhodgdon created an issue. See original summary.

jhodgdon’s picture

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.

jhodgdon’s picture

Title: Prefix/suffix setting for Numeric fields is set up completely wrong for translations » Misuses of formatPlural() - on Numeric field prefix/suffix and DrupalTranslator::transChoice()
pjonckiere’s picture

Assigned: Unassigned » pjonckiere
Status: Active » Needs review
Issue tags: +Needs beta evaluation
FileSize
3.71 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,245 pass(es). View

This is a patch to remove the numberformatter ones. Hopefully this will break some tests.

I'll have a look at DrupalTranslator::transChoice() soon, so assigning.

pjonckiere’s picture

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

Given the patch in #4 being green, I think we need additional coverage here. I'll look into that.

As for DrupalTranslator::transChoice(), I'm a bit confused. The documentation on the Symfony interface states:

@param string $id The message id (may also be an object that can be cast to string)

That is clearly singular, whereas the first thing we do is to explode that string because we expect more than one id. So the current situation is wrong, or hacky at least (and thus I'm agreeing with #2). While related, I feel like it deserves a separate issue? Besides, I don't think this should block #2449597: Number formatters: Make it possible to configure format_plural on the formatter level. Thoughts?

jhodgdon’s picture

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. ?!?

pjonckiere’s picture

Right, I just came to that conclusion. It's silly to add a prefix/suffix with a pipe.

I'll look into DrupalTranslator::transChoice() again.

Added beta eval.

pjonckiere’s picture

Assigned: pjonckiere » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs change record

The issue mentioned in #2 on the DrupalTranslator::transChoice() call, seems to be more complex since it's unsure if that code is ever called. So I created a follow-up for that #2552867: Document DrupalTranslator::transChoice()'s non-standard formatPlural() usage.

Added a CR for the prefix/suffix change in behaviour.

I think the patch in #4 is ready for review.

jhodgdon’s picture

Title: Misuses of formatPlural() - on Numeric field prefix/suffix and DrupalTranslator::transChoice() » Misuse of formatPlural() in Numeric field prefix/suffix
Issue summary: View changes

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.

jhodgdon’s picture

Status: Needs review » Needs work

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!

pjonckiere’s picture

Status: Needs work » Needs review
FileSize
1 KB
3.76 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,172 pass(es). View

Yes, indeed!

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +D8 upgrade path

Two questions:

1. What happens to existing data? Is it just going to be used directly with the pipe in it? Is there anything we can do for that?

2. If not, should it actually be merged back into #2449597: Number formatters: Make it possible to configure format_plural on the formatter level so that we can provide an upgrade path from one to the other?

jhodgdon’s picture

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?

pjonckiere’s picture

Status: Needs review » Needs work

I'm also leaning towards (b), but I guess that would be "against protocol". Back to NW to implement either one from #14.

Also, this will need a reroll if #2551511: Remove SafeMarkup::set() from AllowedTagsXssTrait gets in first.

jhodgdon’s picture

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

jhodgdon’s picture

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.

pjonckiere’s picture

Status: Needs work » Needs review

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

array_pop() took the part after the last pipe. I changed it to array_shift() in this patch so it takes the first part.

As for c), I added a warning message that mods/editors see when creating the field setting, or adding/editing a node which contains a pipe in the prefix and/or suffix field. Plus a todo refering the issue mentioned in d).

If this would be ok, we need to change the last part of the CR a bit.

pjonckiere’s picture

FileSize
1.5 KB
4.15 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,966 pass(es). View

Apparently I forgot the patch :/

catch’s picture

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.

Could we do the ones we know about though?

jhodgdon’s picture

Status: Needs review » Needs work

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?

jhodgdon’s picture

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.

catch’s picture

I think in this case we could just remove the content after the pipe (and the pipe), since it already doesn't work at all. It's a little bit of configuration data loss but it's not actually going to affect any working site out there in any tangible way.

Then people can use the new functionality when it works - no migration path between the two.

jhodgdon’s picture

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

pjonckiere’s picture

Assigned: Unassigned » pjonckiere
pjonckiere’s picture

Assigned: pjonckiere » Unassigned
Issue summary: View changes
FileSize
179.6 KB
5.79 KB
7.27 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch misuse_of-2545730-26.patch. Unable to apply patch. See the log in the details link for more information. View

Posting a work in progress. This reverts #19, makes prefix/suffix handling ignore pipes and adds a D8 upgrade path.

The warning message added looks like this:

Now we still need to test the upgrade path where the todo tag is. I'll see if I have some time later today to figure out how to do that.

pjonckiere’s picture

Status: Needs work » Needs review
FileSize
4.51 KB
9.96 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch misuse_of-2545730-27.patch. Unable to apply patch. See the log in the details link for more information. View

Added a test for the upgrade path (not sure how thorough that should be), and fixed a grammar error in the warning message.

Status: Needs review » Needs work

The last submitted patch, 27: misuse_of-2545730-27.patch, failed testing.

pjonckiere’s picture

FileSize
10.33 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
pjonckiere’s picture

Status: Needs work » Needs review

The last submitted patch, 26: misuse_of-2545730-26.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 29: misuse_of-2545730-29.patch, failed testing.

pjonckiere’s picture

Assigned: Unassigned » pjonckiere

Seems I missed to rename a function during the reroll.

pjonckiere’s picture

Assigned: pjonckiere » Unassigned
Status: Needs work » Needs review
FileSize
782 bytes
10.29 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,445 pass(es). View

Here we go.

jhodgdon’s picture

Status: Needs review » Needs work

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.

pjonckiere’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
10.32 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
70.03 KB

Thanks for the review!

1. fixed
2. fixed
3. fixed
4. Actually, the t() function doesn't work there for some reason, probably Drupal isn't bootstrapped far enough. Looking in views.install, I found that Drupal::translation()->translate() does work, so I changed it to that. Now the message is fully translatable:

I also removed the breaks, since you can't translate them out-of-the-box in the translate interface. They aren't considered safe. It's a bit of readability lost, but I couldn't find a solution for that yet.

Edit: in the screenshot, it seems like the breaks aren't removed, but that is just a coincidence.

Status: Needs review » Needs work

The last submitted patch, 36: misuse_of-2545730-36.patch, failed testing.

pjonckiere’s picture

Status: Needs work » Needs review
FileSize
774 bytes
10.31 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,686 pass(es). View

Seems like the rebase fix in #34 got lost in my local repo. Added that again, with the @inheritdoc this time :p

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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

Gábor Hojtsy’s picture

Issue tags: +sprint
alexpott’s picture

Assigned: Unassigned » yched
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs subsystem maintainer review

I had a look at this recently and first of all thought that is was a bug and then I ended up not so sure it was. I know it is certainly strange and hard to follow through. Going to email @yched for some history. I'm also not sure how this plays with config translation - probably not well. And the patch here might well be the correct thing to do - but it does not hurt to ask.

yched’s picture

I have to say I was never very familiar with formatPlural() / format_plural() :-/

Digging in git history, this comes from waaaaay back in CCK 4.7, it was introduced in #112204: Add prefix/suffix to $node->field_foo['view'] :

  $prefixes = explode('|', $field['prefix']);
  $suffixes = explode('|', $field['suffix']);
  if ($prefixes) {
    if (sizeof($prefixes) > 1) {
      $prefix = format_plural($item['value'], $prefixes[0], $prefixes[1]);
    }
    else {
      $prefix = $field['prefix'];
    }
  }
  if ($suffixes) {
    if (sizeof($suffixes) > 1) {
      $suffix = format_plural($item['value'], $suffixes[0], $suffixes[1]);
    }
    else {
      $suffix = $field['suffix'];
    }
  }
  return $prefix . $value . $suffix;

It later got streamlined a bit using ternaries, and sanitized using filter_xss(), but it's more or less the same code than we have in current HEAD :

$prefixes = isset($settings['prefix']) ? array_map(array('Drupal\Core\Field\FieldFilteredString', 'create'), explode('|', $settings['prefix'])) : array('');
$suffixes = isset($settings['suffix']) ? array_map(array('Drupal\Core\Field\FieldFilteredString', 'create'), explode('|', $settings['suffix'])) : array('');
$prefix = (count($prefixes) > 1) ? $this->formatPlural($item->value, $prefixes[0], $prefixes[1]) : $prefixes[0];
$suffix = (count($suffixes) > 1) ? $this->formatPlural($item->value, $suffixes[0], $suffixes[1]) : $suffixes[0];
$output = $prefix . $output . $suffix;

I don't know if the semantics changed between D4.7 format_plural() and D8 formatPlural(), or if this code was always wrong from the get go (granted, we didn't have entity translation in D4.7, if that's of any relevance), but I'll trust i18n folks on the proper approach here...

Gábor Hojtsy’s picture

Yeah so this use of format plural assumes that the config is in English or at least in a language that has identical plural rules to English if we allow for a more permissive use of format plural (but in this use the foreign language source string will end up in the locale DB among everything else which is English in source). If we'd want to support prefix/suffix plurality, that would need a much more sophisticated approach for input which considers the language of the config. Eg. if you are entering your config in Polish (on a Polish native site), that would require a lot more input variants than just 1 or 2. I am not sure how could we support a variant input field like that which may take 1 if there is no plurality needed or some other number based on the language of the config.

Is this feature used widely?

yched’s picture

Is this feature used widely?

Hard to say. @Killes seemed to want it back in the CCK 4.7 issue :-)
More seriously, it's likely to be fairly useful for "suffix as a non-abbreviated unit" ? (1 meter, 2 meters...)

So if I get this right (I very well might not...),
- the translation of the prefix/suffix should be taken care of by config translation (of field config entities)
- $field_definition->getSetting('prefix') gives us a prefix in the correct/current UI language, possibly with plural variants using the LOCALE_PLURAL_DELIMITER, and we should rather use formatPluralTranslated() to only leverage the locale_get_plural() / "get numeric index of the plural variant to use for $number in $language" logic, on the already-translated string ?

But then right, asking people to enter the translation of the field setting in the LOCALE_PLURAL_DELIMITER format is not exactly great UX. Sounds like a multi-value/add-more input widget on a config form, which we'd have to build from scratch (and the "automated config translation forms" would likely not cater for those). Also, figuring out the right string for the right index might not be too obvious ?

Finally, I'm not sure whether that would still work with a formulation of "prefix + suffix", or would require to move to a single, more translation-aware "formatting string" ("prefix @count suffix") ?

yched’s picture

Regarding UX challenges, don't we already have the case in Views, where the existence of singular/plural string settings apparently motivated #2454859: Not possible to format plural an already translated string and the introduction of formatPluralTranslated() ?

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Yeah we have a pattern of input fields for the likes of #2454859: Not possible to format plural an already translated string that is language-aware. We can use that for the prefix/suffix. What I wanted to get at is that prefix/suffix also attempts to support a single input value that is not pluralized, eg. 'm', 'USD', '$', etc (vs. 'meter'/'meters', 'dollar'/'dollars'). Making people enter that X times (the number of plural variants for the language of the config being edited) and then translate it X times sounds non-ideal as well. So maybe a non-plural single input field and a checkbox to make it plural in which case it is swapped with the plural input field (with #states). That sounds like the all-serving solution. Then this comes down to adding that checkbox and the format plural supporting stuff to the config and an upgrade path.

yched’s picture

What I wanted to get at is that prefix/suffix also attempts to support a single input value that is not pluralized, eg. 'm', 'USD', '$', etc (vs. 'meter'/'meters', 'dollar'/'dollars'). Making people enter that X times (the number of plural variants for the language of the config being edited) and then translate it X times sounds non-ideal as well

Agreed. Is that a case the current "pattern of input fields for the likes of #2454859: Not possible to format plural an already translated string" (were those in Views ?) didn't have to bother with ?

If so, then yes,

a non-plural single input field and a checkbox to make it plural in which case it is swapped with the plural input field (with #states). That sounds like the all-serving solution

sounds like a good approach.

Also, do you think splitting prefix/suffix still works, or do we have to combine into a single "prefix @count suffix" formatting string ? (end of #44)

Gábor Hojtsy’s picture

I think either works. At least introducing the plural support as a new checkbox + fields is backwards compatible in terms of config storage as well if we store them as-is. If we are to merge prefix and suffix that would be entirely different config storage too. Also you would need to repeat the prefix and suffix N times even if only one of them needs pluralization, so keeping them separate may be best.

catch’s picture

@Gabor so are you suggesting making that change in this issue? Or going ahead and removing the current non-working support then adding it back again elsewhere?

Gábor Hojtsy’s picture

Yeah, hm, so I did not consider the very legitimate use cases for this feature before. If we do this patch now, it would lead to loosing that configuration and when we add it back in people will need to manually add it in :/ It looks to me like updating the data to a new structure of a flip-switch boolean and another string for plurals is not that far fetched here while it would keep the data.

jhodgdon’s picture

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.

yched’s picture

because the order of what goes into prefix and what goes into suffix is not even always the same across languages.

This is why I was wondering earlier whether separate prefix/suffix strings were still workable wrt translatability, or whether we needed a single "prefix @count suffix" string, where translations can freely reshuffle what is before and after @count ;-)

The current, separate 'prefix' & 'suffix' settings could be migrated by simple concatenation ?
The input UI might be less friendly though (need to remember to to put "@count" without typos at the right place...).

So yeah, not sure :-/

jhodgdon’s picture

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?

yched’s picture

Not really, I'm talking about how those settings are organized (two separate 'prefix' and 'suffix' or one single 'formatting string'), I didn't intend to question where they live ("configured in the field, used across all displays" or "configured formatter by formatter"). Unless I'm missing something, that should be orthogonal ?

- Currently they are in field settings, and are used across all displays (the formatter just opts in or out by a 'display the field's prefix & suffix' boolean setting), and also by the input widget.
- Moving them to formatters could be debated for more flexibility I guess (like display "2 meters" on full node but "2 m" on teasers), but that becomes more painful to confugure (and translate) over and over in each view mode, and we'd lose them in the widget unless we add them there too.

I guess "defined in the field, overridable in each display" would be the best of both worlds. But that sounds like a different discussion than either this issue or #2449597: Number formatters: Make it possible to configure format_plural on the formatter level - more a feature request that is not really related to fixing any of those two issues ?

jhodgdon’s picture

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?

yched’s picture

Thanks for the summary @jhodgdon. It was not clear in my mind anymore that one of the aspects was fixing the feature regression of Views fields losing the ability to specify custom formatPlural / @count output when they were moved to using regular formatters - your point d) in #55.

Still not sure about prefix/suffix vs. one @count string, so I'm leaving that aside for the rest of this comment.

If we're going to add those settings to the formatter-level (which is needed for the Views case above), I'd really advocate we keep the ability to specify defaults in the field, overridable in the formatter, to avoid the massive UX pain of having to reconfigure/retranslate them in each view mode.

Views needs the abilty to have specific output strings configured for a specific formatter used for a specific field in a specific display, that's one thing, but on the EntityViewDisplay (non-Views) side IMO 99% use cases are fine with just one "prefix / suffix" pair (or the @count string) configured only once on the field and just reused on all view modes by default - meaning, the use case I mentioned in #54 (display "2 meters" on full node but "2 m" on teasers) is valid but IMO very much an edge case, would be sad to make life more complex for all other cases.

(As a side note, that "default value in field / overridable in each display" pattern is also what I think we should have done for field labels - just saying, that's not something for this issue, just it's a model that makes sense IMO for the right combination of flexibility and usability. Views sure know about that pattern ;-) )

That would be something like :
- field settings specify prefix/suffix (or @count string)
- the widget uses those, with variant for @count 2 (because it's as good a guess we can make for "some value X currently being edited") ?
- the formatter has a "display prefix/suffix" boolean (because you might still want just the raw number)
- if checked, an additional setting lets the formatter use specific values (again, whether that's a prefix/suffix or @count string) instead of the ones specified by the field

What do you think ?

yched’s picture

Assigned: yched » Unassigned

Also, unassigning, consider me pinged :-)

jhodgdon’s picture

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

Gábor Hojtsy’s picture

Right, I was considering #2449597: Number formatters: Make it possible to configure format_plural on the formatter level as adding plural features to a different part of number formatting. And technically it is. This one is about prefix and suffix plurality and that one is about rewriting the whole value overall with a plural-aware variant. Given that allows people to prefix/suffix the value and use @count to place the value where it is in the string between pluralized prefix/suffix, it does sound like a workaround for this broken singular/plural prefix/suffix feature indeed. So that being said, I don't see a value in supporting the plurality on both the prefix/suffix and as a standalone overall replacement version like in #2449597: Number formatters: Make it possible to configure format_plural on the formatter level and as @yched indicated the replacement variant is more flexible (and also easier to provide a UI for).

jhodgdon’s picture

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

jhodgdon’s picture

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

yched’s picture

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.

I might have missed something, but I thought we'd always use formatPluralTranslated() since the setting string comes already translated from the config entity (and we know what is the associated language with the $settings_langcode introduced in #2449597: Number formatters: Make it possible to configure format_plural on the formatter level).
In which case would we use formatPlural() ?

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

One more thought. If we omit #60-f, and leave that for an 8.1 feature request, I think it would be ***way*** simpler and satisfy most of the objectives for most users

Sorry, not sure I get what that last scenario ("skipping f") would mean exaclty. Leave out the part where the formatter can specify its "@count" string overriding the field ? I though that was needed for Views fields ?

jhodgdon’s picture

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

jhodgdon’s picture

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

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

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.

yched’s picture

Not sure I'll be able to work on a patch, sorry :-/, and thanks @jhodgdon !

It's likely that we won't be able to change the formatters construct after RC. Which only leaves us with the option to pass the langcode within the $settings array, which I wanted to avoid, but well...

At least, we should prefix it with an underscore ($settings['_settings_langcode']), to denote it's "special" and limit the chances of clashing with an actual setting of an actual formatter somewhere.

@catch and @alexpott recommended that approach (_ prefix) in a different but similar case (was : what if we find CMI needs to add a new top-level entry like 'dependencies' or 'uuid' to all config records in a later point release ? Answer : then we'll prefix it with _. Ugly but non-clashing)

jhodgdon’s picture

That sounds like an excellent idea.

plach’s picture

Issue tags: +language-config
jhodgdon’s picture

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

jhodgdon’s picture

Issue summary: View changes

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

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
Related issues: +#2499639: Use better labels for numeric fields when using a multiple plural forms language

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

jhodgdon’s picture

FileSize
25.7 KB

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

mpdonadio’s picture

I worked on #2449597: Number formatters: Make it possible to configure format_plural on the formatter level. I'll take a look at #72 when I get a chance, but it may not be until Sunday.

jhodgdon’s picture

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.

jhodgdon’s picture

(adding credit for mpdonadio from the other issue)

jhodgdon’s picture

Issue summary: View changes

Adding note to summary about patch credit.

Status: Needs review » Needs work

The last submitted patch, 72: 2545730-first-new-attempt-72.patch, failed testing.

yched’s picture

Thanks @jhodgdon. I didn't focus too much on the actual "plural logic" for now :-)

  1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
    @@ -197,6 +197,9 @@ public function getRenderer($field_name) {
    +      // Record the language that the configuration was translated into, which
    +      // is needed for plural formatting (for example).
    +      $configuration['settings']['_settings_langcode'] = $this->langcode;
    

    Nitpick : we're not "recording" (writing to a storage of some sort) anything here :-)

    I'd suggest :
    "Pass the language of the Display configuration down to each formatter, as it might be relevant for some of the formatter settings (plural formatting, for example)." ?

    Also, we'd need to do the same for widgets in EntityFormDisplay, for consistency (way less invasive now that we don't modify constructors)

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -39,16 +67,76 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +      '#options' => array(
    +        NumericFormatterBase::FORMAT_PLURAL_USE_FIELD_SETTING => $this->t('Use setting from the field settings'),
    +        NumericFormatterBase::FORMAT_PLURAL_USE_FORMATTER_SETTING => $this->t('Override the setting from the field settings'),
    +        NumericFormatterBase::FORMAT_PLURAL_NONE => $this->t('No prefix/suffix'),
    +      ),
    

    Nitpick : wouldn't we want "no prefix/suffix" to be the first option ?

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -39,16 +67,76 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +    $elements['format_plural_values'] = array(
    +      '#type' => 'fieldset',
    +      '#title' => $this->t('Prefix/suffix override'),
    +      '#after_build' => [[get_class($this), 'formatPluralValuesAfterBuild']],
    +    );
    

    Don't we want some #states magic to only show this when 'Override' is selected ?

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -56,8 +144,8 @@ public function settingsSummary() {
    -    if ($this->getSetting('prefix_suffix')) {
    -      $summary[] = t('Display with prefix and suffix.');
    +    if ($this->getSetting('format_plural') != NumericFormatterBase::FORMAT_PLURAL_NONE) {
    +      $summary[] = $this->t('Display prefix/suffix.');
         }
    

    We show the same summary for "override" & "no override" ?

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
    @@ -74,13 +162,18 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    +      $plural_format = '';
    +      if ($this->getSetting('format_plural') == NumericFormatterBase::FORMAT_PLURAL_USE_FIELD_SETTING) {
    +        $plural_format = $settings['format_plural_string'];
    +      }
    +      elseif ($this->getSetting('format_plural') == NumericFormatterBase::FORMAT_PLURAL_USE_FORMATTER_SETTING) {
    +        $plural_format = $this->getSetting('format_plural_string');
           }
    

    Nit : for consistency with the settings name ('format_plural' is the controlling tri-state, not the string)
    s/$format_plural/$plural_string ?

    Also, for clarity, maybe
    s/$settings/$field_settings
    (pre-existing var name, but the difference really matters now)

  6. --- a/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
    +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
    

    Now that the _settings_langcode is back in the $settings passed to each formatter, we should make sure that it is never written back into to the Display's yaml (there are places like EntityDisplayBase::onDependencyRemoval() where we update the content of the EntityDisplay based on the formatter::getSettings())

    We could do this :
    - in EntityDisplayBase::setComponent(), make sure we remove any $options['settings']['_settings_langcode'] if it is pased in.
    - maybe also in EntityDisplayBase::toArray() for good measure, add a line within the loop that does unset($properties['content'][$field_name]['settings']['_settings_langcode'])

    This sort of messy stuff is why I wanted to avoid putting the langcode in $settings ;-). But as discussed, no other option now...

  7. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilderInterface.php
    @@ -118,7 +118,9 @@ public function resetCache(array $entities = NULL);
    +   *       formatter's default settings. If there is a '_settings_langcode'
    +   *       value, it will override the language used for formatting, which is
    +   *       normally the current interface language.
    

    I'd suggest :
    "A '_settings_langcode' entry should be present to indicate the language of the settings that are being provided (that is typically the langcode of the config record where the settings come from). If absent, the current interface language will be assumed."

  8. +++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
    @@ -92,6 +92,9 @@ public function createInstance($plugin_id, array $configuration = array()) {
    +   *       A _settings_langcode value will be added, giving the language code
    +   *       of the parent entity (entity view mode, view, etc. that the formatter
    +   *       is embedded in).
    

    That comment is lying atm, FormatterPluginManager::getInstance() currently does nothing about '_settings_langcode'.

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

yched’s picture

[edit : fixed numbering in my previous comment]

Also : Since this changes the way prefix/suffix settings are formulated in numeric fields and formatters, we'll need to provide an update function to adapt existing records (fields, entity displays, views as well I guess ?)

yched’s picture

Last : form the guidelines in https://groups.drupal.org/node/486388, we should probably be aware that this is unlikely to be eleigible for getting in before 8.0.0, but only in a later patch or minor release...

jhodgdon’s picture

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

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
38.8 KB
19.72 KB

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

Status: Needs review » Needs work

The last submitted patch, 82: 2545730-next-pass-82.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
41.88 KB
3.92 KB

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.

Status: Needs review » Needs work

The last submitted patch, 84: 2545730-next-pass-84.patch, failed testing.

jhodgdon’s picture

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.

mpdonadio’s picture

Status: Needs work » Needs review

TestBot, take me away!

Status: Needs review » Needs work

The last submitted patch, 86: 2545730-next-pass-86.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
52.12 KB
1.57 KB

This just fixes the fails in MigrateFieldWidgetSettingsTest.

No clue what is wrong in UpdatePathTestBaseFilledTest, and it is taking forever to run locally.

Status: Needs review » Needs work

The last submitted patch, 89: misuse_of-2545730-89.patch, failed testing.

jhodgdon’s picture

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.

mpdonadio’s picture

#91, it's easier read the patch in place, and also notice the $expected array keeps getting reused which makes it harder to follow. I removed the 'format_plural' from the expected results for things that I don't have plural forms (textfields, emails, and phone numbers) defined in core.data_types.schema.yml per the patch. If they were in the schema, the default value would have been in actual value when it gets pulled from the component.

jhodgdon’s picture

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

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
16.27 KB
60.46 KB

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.

jhodgdon’s picture

FileSize
633 bytes
60.46 KB

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

jhodgdon’s picture

FileSize
60.46 KB
877 bytes

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!

The last submitted patch, 94: misuse_of-2545730-94.patch, failed testing.

The last submitted patch, 95: misuse_of-2545730-95.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 96: misuse_of-2545730-96.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
60.55 KB
4.56 KB

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

Status: Needs review » Needs work

The last submitted patch, 100: misuse_of-2545730-100.patch, failed testing.

jhodgdon’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.83 KB
2.76 KB
65.1 KB

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.

Status: Needs review » Needs work

The last submitted patch, 102: misuse_of-2545730-102.patch, failed testing.

mpdonadio’s picture

Assigned: jhodgdon » mpdonadio

Fixing the fails.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
65.16 KB
2.73 KB

Mostly fixed NumericFieldsUiTest, but it throws some notices and warnings at line 102 from EntityFormDisplayBase. Looks like part of a render array is wonky, but want to clear my brain before trying to track that down.

Status: Needs review » Needs work

The last submitted patch, 105: misuse_of-2545730-105.patch, failed testing.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
65.19 KB
871 bytes

NumericFieldsUiTest passes locally for me.

jhodgdon’s picture

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

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

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

mpdonadio’s picture

The update test sounds like overkill. You are just testing that nothing breaks output wise when you update config. I would think that you just need to

- add a field w/ a plural setting and export the config for use in the upgrade test
- make a node, render the field to make sure it is OK
- perform upgrade
- render the field again to make sure it is unchanged

I don't see why the forms and Views need to be part of it?

jhodgdon’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 111: misuse_of-2545730-111.patch, failed testing.

mpdonadio’s picture

FileSize
20.58 KB

I think this is a good interdiff.

jhodgdon’s picture

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.

jhodgdon’s picture

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

Status: Needs review » Needs work

The last submitted patch, 115: misuse_of-2545730-115.patch, failed testing.

jhodgdon’s picture

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

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
83.48 KB
1.82 KB

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

jhodgdon’s picture

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.

jhodgdon’s picture

Issue summary: View changes

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.

jhodgdon’s picture

Issue summary: View changes

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

jhodgdon’s picture

Issue summary: View changes

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.

jhodgdon’s picture

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

jhodgdon’s picture

Issue summary: View changes
Issue tags: +rc target triage

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

jhodgdon’s picture

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

jhodgdon’s picture

Issue summary: View changes

Minor summary updates...

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/NumberWidget.php
    @@ -100,13 +114,19 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +    if ($field_settings['format_plural_string'] && $this->getSetting('format_plural')) {
    +      // The field setting is a string containing all plural variants, suitable
    +      // for use in formatPlural() methods, but already translated into the
    +      // UI language. Pop off the last plural variant, and use the parts
    +      // before/after @count in that string as the prefix/suffix for this field.
    +      $values = explode(PluralTranslatableMarkup::DELIMITER, $field_settings['format_plural_string']);
    +      $labels = explode('@count', array_pop($values));
    +      if (isset($labels[0])) {
    +        $element['#field_prefix'] = FieldFilteredMarkup::create(trim($labels[0]));
    +      }
    +      if (isset($labels[1])) {
    +        $element['#field_suffix'] = FieldFilteredMarkup::create(trim($labels[1]));
    +      }
         }
    

    Well, it would be PluralTranslatableMarkup::createFromTranslatedString() as happens in NumericFormatterBase:: viewElements(), not formatPlural(). Calling formatPlural() would be incorrect as per this very issue.

    I am also wondering if popping the last variant is correct. Of course we don't know how much the number will be at the end, but we do know the current number. Maybe our best bet is to pick the right variant for the current number, no?

  2. +++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatableMarkup.php
    @@ -123,7 +127,10 @@ public function render() {
         $arguments = $this->getArguments();
    -    $arguments['@count'] = $this->count;
    +    // Allow overrides of what gets substituted in for @count.
    +    if (!isset($arguments['@count'])) {
    +      $arguments['@count'] = $this->count;
    +    }
         $translated_array = explode(static::DELIMITER, $this->translatedString);
     
         if ($this->count == 1) {
    @@ -142,9 +149,9 @@ public function render() {
    
    @@ -142,9 +149,9 @@ public function render() {
           }
           else {
             // If the index cannot be computed or there's no translation, use the
    -        // second plural form as a fallback (which allows for most flexibility
    +        // last plural form as a fallback (which allows for most flexibility
             // with the replaceable @count value).
    -        $return = $translated_array[1];
    +        $return = $translated_array[count($translated_array) - 1];
    

    Why do these need to be changed here?

  3. +++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
    @@ -84,6 +84,59 @@ protected function getNumberOfPlurals($langcode = NULL) {
    +  protected function getPluralLabels($langcode) {
    +    $plurals = $this->getNumberOfPlurals($langcode);
    

    The introduction of this method is welcome. We have several copy-pasted scenarios for forms already. I understand you don't want to reach out and start using this function there too, but it will need to happen. Maybe an issue open with a todo would help :)

  4. +++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
    @@ -84,6 +84,59 @@ protected function getNumberOfPlurals($langcode = NULL) {
    +    // @todo The return values for both cases should be more relevant to the
    +    // actual plural rules of the language; see
    +    // https://www.drupal.org/node/2499639.
    

    Says both but then has 3 cases :)

  5. +++ b/core/modules/system/system.install
    @@ -1845,5 +1846,131 @@ function system_update_8013() {
    +    $field_storage_configs = \Drupal::entityManager()->getStorage('field_storage_config')->loadByProperties(array('type' => $type));
    ...
    +      $field_configs = \Drupal::entityManager()->getStorage('field_config')->loadByProperties(array('field_name' => $field_name));
    ...
    +        if ($view_displays = \Drupal::entityManager()->getStorage('entity_view_display')->loadByProperties($properties)) {
    +          foreach ($view_displays as $view_display) {
    ...
    +            if ($component = $view_display->getComponent($field_name)) {
    ...
    +              $view_display->setComponent($field_name, $component)->save();
    ...
    +            if ($component = $form_display->getComponent($field_name)) {
    ...
    +              $form_display->setComponent($field_name, $component)->save();
    

    Are we supposed to use entity and display APIs like this in update functions?

  6. +++ b/core/modules/system/system.install
    @@ -1845,5 +1846,131 @@ function system_update_8013() {
    +      $displays = $view->get('display');
    ...
    +        $view->set('display', $displays);
    +        $view->save();
    

    Are we supposed to be allowed to use the views API like this in updates?

jhodgdon’s picture

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

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
84.6 KB
5.73 KB

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?

Status: Needs review » Needs work

The last submitted patch, 129: misuse_of-2545730-129.patch, failed testing.

jhodgdon’s picture

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

jhodgdon’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Hm, I don't have a strong opinion on needing to improve the form input here. The patch just keeps the existing behavior. I think its not ideal, but there is already much going on here, so no need to dive into that one too. Test coverage looks good to me and the UI changes make sense as well :) Yay!

The last submitted patch, 129: misuse_of-2545730-129.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 131: misuse_of-2545730-131.patch, failed testing.

jhodgdon’s picture

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.

jhodgdon’s picture

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.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Will look into failure.

The last submitted patch, 131: misuse_of-2545730-131.patch, failed testing.

mpdonadio’s picture

Just making some notes here as these tests take so long to run locally...

Patch last passed at #2598696: Rollback should not delete uid 1.

Patch first failed at #2491875: EntityViewsData adds Operations links to all entities, which won't work if the entity type has no list builder, leading to WSOD on some views.

git diff d934d53..bad2aea4 didn't reveal anything obvious, but #2337191: Split up EntityManager into many services did land in the middle of those. I reverted that, and there was still a fail in UpdatePathRC1TestBaseFilledTest.

Only clue so far is

PHP Fatal error: Class name must be a valid object or a string in /Users/mpdonadio/Documents/PhpStorm/drupal-8.0.x/core/modules/simpletest/simpletest.module on line 348

The classname that gets shifted off the list at that point appears to be an empty string (or otherwise empty thing).

No real idea what is going on here.

The last submitted patch, 129: misuse_of-2545730-129.patch, failed testing.

alexpott’s picture

Assigned: mpdonadio » jhodgdon

Wow this issue got complicated. The solution looks really good. Does this actually work in d7?

  1. +++ b/core/modules/system/system.install
    @@ -1845,5 +1846,131 @@ function system_update_8013() {
    +    if (!$field_storage_configs) {
    +      continue;
    +    }
    ...
    +      $field_configs = \Drupal::entityManager()->getStorage('field_config')->loadByProperties(array('field_name' => $field_name));
    ...
    +        if ($view_displays = \Drupal::entityManager()->getStorage('entity_view_display')->loadByProperties($properties)) {
    

    We should not be using the entity manager here only the config factory.

  2. +++ b/core/modules/system/tests/fixtures/update/drupal-8.numeric-field-data-2545730.php
    @@ -0,0 +1,93 @@
    +// Define a complete set of configurations, with dependencies.
    +$configs = [
    +  'core.entity_form_display.node.number_test.default',
    +  'core.entity_view_display.node.number_test.default',
    +  'field.field.node.number_test.field_test_int',
    +  'field.storage.node.field_test_int',
    +  'node.type.number_test',
    +  'views.view.number_test',
    +];
    +
    +$data = [];
    +foreach ($configs as $config) {
    +  $file = 'drupal-8.' . $config . '-2545730.yml';
    +  $data[$config] = \Drupal\Component\Serialization\Yaml::decode(file_get_contents(__DIR__ . '/' . $file));
    +  $connection->insert('config')
    +    ->fields(array(
    +      'collection',
    +      'name',
    +      'data',
    +    ))
    +    ->values(array(
    +      'collection' => '',
    +      'name' => $config,
    +      'data' => serialize($data[$config]),
    +      ))
    +    ->execute();
    +}
    

    Unfortunately this is not as simple as this - we also need to deal with the key value lookup table and add their uuids

alexpott’s picture

Issue tags: -rc target triage +rc target

Discussed with @catch and we agree that we should try to fix this bug during RC.

alexpott’s picture

Assigned: jhodgdon » Unassigned

Dunno why I assigned to @jhodgdon

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

I'm working on this while @jhodgdon is unavailable. Thanks for the input, I'll start on incorporating it, but I may need to hop on IRC for some help with the KV table / UUIDs.

mpdonadio’s picture

First pass at #143-01. Totally untested.

Not 100% sure what #143-02 means here.

Status: Needs review » Needs work

The last submitted patch, 147: misuse_of-2545730-147.patch, failed testing.

jhodgdon’s picture

Issue tags: -rc target

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.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned

Sorry, I got sidetracked and forgot about this. Unassigning myself for now, as my I won't have "Drupal time" until the weekend.

jhodgdon’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Version: 8.1.x-dev » 8.2.x-dev
Priority: Major » Normal

The core committers and Entity and Field maintainers discussed this issue today. The current behavior in HEAD is mostly incompatible with multilingual sites and misuses translatable strings, and @alexpott noted that it could also result in it appearing that already-translated strings still needed translation. So this is definitely a bug. However, it is limited to field prefix/suffix functionality so fairly narrow in impact compared to the other related bugs around plurals. Based on that, we decided this was a normal bug.

I'm also moving this to 8.2.x since we need to basically completely replace the functionality and make a user-facing change in order to fix this properly (as per the summary). The screenshots in the summary look like they are on a good track to me. Once the patch is passing tests we should probably get a usability review.

Thanks everyone, especially @mpdonadio and @jhodgdon for carrying this forward.

The last submitted patch, 147: misuse_of-2545730-147.patch, failed testing.

mpdonadio’s picture

Issue tags: +Needs reroll
$ git checkout 61583a65e30118944b8e34617d71d9cbd6e28a15
$ git apply --index misuse_of-2545730-147.patch
$ git rebase 8.2.x
First, rewinding head to replay your work on top of it...
Applying: misuse_of-2545730-147.patch
Using index info to reconstruct a base tree...
M	core/config/schema/core.data_types.schema.yml
M	core/config/schema/core.entity.schema.yml
M	core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
M	core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
M	core/lib/Drupal/Core/Entity/EntityDisplayBase.php
M	core/lib/Drupal/Core/Entity/EntityViewBuilder.php
M	core/lib/Drupal/Core/Entity/EntityViewBuilderInterface.php
M	core/lib/Drupal/Core/Field/FormatterPluginManager.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/DecimalFormatter.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/IntegerFormatter.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldType/IntegerItem.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldType/NumericItemBase.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/NumberWidget.php
M	core/lib/Drupal/Core/Field/WidgetPluginManager.php
M	core/lib/Drupal/Core/StringTranslation/PluralTranslatableMarkup.php
M	core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
M	core/modules/field/src/Plugin/migrate/process/d6/FieldFormatterSettingsDefaults.php
M	core/modules/field/src/Plugin/migrate/process/d6/FieldInstanceSettings.php
A	core/modules/field/src/Tests/Migrate/d6/MigrateFieldFormatterSettingsTest.php
A	core/modules/field/src/Tests/Migrate/d6/MigrateFieldInstanceTest.php
A	core/modules/field/src/Tests/Migrate/d6/MigrateFieldWidgetSettingsTest.php
M	core/modules/field/src/Tests/Number/NumberFieldTest.php
A	core/modules/field_ui/src/Tests/EntityDisplayTest.php
A	core/modules/field_ui/src/Tests/EntityFormDisplayTest.php
M	core/modules/node/tests/modules/node_test_views/test_views/views.view.test_nid_argument.yml
M	core/modules/system/src/Tests/Update/UpdatePathRC1TestBaseFilledTest.php
M	core/modules/system/src/Tests/Update/UpdatePathRC1TestBaseTest.php
M	core/modules/system/system.install
M	core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_taxonomy_term_relationship.yml
M	core/modules/views/src/Entity/Render/EntityFieldRenderer.php
M	core/modules/views/src/Plugin/views/field/Field.php
M	core/modules/views/tests/modules/views_test_config/test_views/views.view.test_row_render_cache.yml
Falling back to patching base and 3-way merge...
Auto-merging core/modules/views/tests/modules/views_test_config/test_views/views.view.test_row_render_cache.yml
Auto-merging core/modules/views/src/Plugin/views/field/Field.php
CONFLICT (content): Merge conflict in core/modules/views/src/Plugin/views/field/Field.php
Auto-merging core/modules/views/src/Entity/Render/EntityFieldRenderer.php
Auto-merging core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_taxonomy_term_relationship.yml
Auto-merging core/modules/system/system.install
Auto-merging core/modules/node/tests/modules/node_test_views/test_views/views.view.test_nid_argument.yml
Auto-merging core/modules/field_ui/tests/src/Kernel/EntityFormDisplayTest.php
Auto-merging core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php
Auto-merging core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldWidgetSettingsTest.php
CONFLICT (content): Merge conflict in core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldWidgetSettingsTest.php
Auto-merging core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldInstanceTest.php
Auto-merging core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldFormatterSettingsTest.php
Auto-merging core/modules/field/src/Tests/Number/NumberFieldTest.php
CONFLICT (content): Merge conflict in core/modules/field/src/Tests/Number/NumberFieldTest.php
Auto-merging core/modules/field/src/Plugin/migrate/process/d6/FieldInstanceSettings.php
Auto-merging core/modules/field/src/Plugin/migrate/process/d6/FieldFormatterSettingsDefaults.php
Auto-merging core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
Auto-merging core/lib/Drupal/Core/StringTranslation/PluralTranslatableMarkup.php
Auto-merging core/lib/Drupal/Core/Field/WidgetPluginManager.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/NumberWidget.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldType/NumericItemBase.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldType/IntegerItem.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/IntegerFormatter.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/DecimalFormatter.php
Auto-merging core/lib/Drupal/Core/Field/FormatterPluginManager.php
Auto-merging core/lib/Drupal/Core/Entity/EntityViewBuilderInterface.php
Auto-merging core/lib/Drupal/Core/Entity/EntityViewBuilder.php
Auto-merging core/lib/Drupal/Core/Entity/EntityDisplayBase.php
Auto-merging core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
Auto-merging core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
Auto-merging core/config/schema/core.entity.schema.yml
Auto-merging core/config/schema/core.data_types.schema.yml
Failed to merge in the changes.
Patch failed at 0001 misuse_of-2545730-147.patch

This actually doesn't look like a hard re-roll. The only head scratcher is the conflict in core/modules/views/src/Plugin/views/field/Field.php, mainly because I forgot a lot about this.

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
83.48 KB

I think these are good merges.

Status: Needs review » Needs work

The last submitted patch, 156: 2545730-156.patch, failed testing.

jhodgdon’s picture

That patch failed with:

PHP Fatal error: Cannot redeclare system_update_8014() (previously declared in ./core/modules/system/system.install:1628) in ./core/modules/system/system.install on line 1751

Looks like the patch needs to use a different function name for the hook_update_N() in system.install

mpdonadio’s picture

Not sure why PhpStorm didn't show the syntax error. Guess it hadn't indexed everything...

Status: Needs review » Needs work

The last submitted patch, 159: 2545730-159.patch, failed testing.

jhodgdon’s picture

Getting closer!

So I scrolled back up... once we can get the tests to pass, the only outstanding question was @alexpott's two questions/suggestions in #143, so we'll need to review for that again and see if it was all addressed. Other than those two possible problems, and the test failures, I looked through the patch and it all still makes sense to me.

Regarding the failures... It looks like we might have a few new .yml files that need similar schema updates to the ones that were in the patch from before (to update their prefix/suffix settings to the new way of doing things). YML files to look at:
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_row_render_cache_none.yml
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_duplicate_field_handlers.yml

Fixing those two files might get the tests to pass, I'm not sure.

Regarding #143... I think that the current patch takes care of item 1 (using pure config and not the entity manager in the hook_update_N() function).

But I think neither @mpdonaido nor I knows what #143 item 2 means. We'll need to get @alexpott to clarify I think.

alexpott’s picture

Re #143.2 config entities maintain a key value lookup to make searching by uuid quick. Writing directly to the config table is not going to work. It is better to use low level config services to write so that \Drupal\Core\Config\Entity\Query\QueryFactory::onConfigSave() has a chance to do it's stuff.

jhodgdon’s picture

Oh, I see. So in that .php file we are setting up the database for config by inserting into the 'config' database table. Instead, we should be doing a config save thing. But... can we do that if the configuration is using an old schema (because we are testing updating from the old schema to the new schema)?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 159: 2545730-159.patch, failed testing.

mpdonadio’s picture

Issue tags: +Needs reroll
mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
83.46 KB

Rebased my local branch of this against origin/8.3.x, merge conflict in system.install. Checkout out origin/8.3.x system.install and added update hook w/ new name for 8.3.x.

Status: Needs review » Needs work

The last submitted patch, 167: 2545730-167.patch, failed testing.

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

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