Remove deprecated format_plural usage

testbot?

testbot?

Sadly it does not apply anymore.

Rerolled.
@danylevskyi, do you still want to work in this issue? You have it assigned to yourself. There are still some (new) format_plural() calls in core. You can remove those.

@toddmbloom, Adding an "interdiff" (https://www.drupal.org/documentation/git/interdiff) helps others to evaluate your work.

Unassigned for not stopping anyone to work on this one. Please comment if you want to work on this again.
The patch does not apply anymore.

Working on this...

Replacing the function in the comments.

I got a dude, in core/lib/Drupal/Core/Template/TwigNodeTrans.php line 53:

    // Start writing with the function to be called.
    $compiler->write('echo ' . (empty($plural) ? 't' : 'format_plural') . '(');

This should be modified?

Greetings

Yes, that code is what converts the {% trans %} {%plural %} twig tag in real php code, so must be a valid function.

+++ b/core/modules/views_ui/views_ui.theme.inc
index f03caa5..0d9ba5e 100644
--- a/core/themes/bartik/css/style.css

--- a/core/themes/bartik/css/style.css
+++ b/core/themes/bartik/css/style.css

+++ b/core/themes/bartik/css/style.css
+++ b/core/themes/bartik/css/style.css
@@ -326,11 +326,11 @@ ul.menu li {

@@ -559,7 +559,7 @@ h1.site-name {
diff --git a/core/themes/seven/css/components/menus-and-lists.css b/core/themes/seven/css/components/menus-and-lists.css

diff --git a/core/themes/seven/css/components/menus-and-lists.css b/core/themes/seven/css/components/menus-and-lists.css
index bd2d908..c68808b 100644

index bd2d908..c68808b 100644
--- a/core/themes/seven/css/components/menus-and-lists.css

--- a/core/themes/seven/css/components/menus-and-lists.css
+++ b/core/themes/seven/css/components/menus-and-lists.css

+++ b/core/themes/seven/css/components/menus-and-lists.css
@@ -18,13 +10,11 @@ ul.menu li {
diff --git a/core/themes/seven/css/components/tables.css b/core/themes/seven/css/components/tables.css

diff --git a/core/themes/seven/css/components/tables.css b/core/themes/seven/css/components/tables.css
index 8242d85..14e1c92 100644

index 8242d85..14e1c92 100644
--- a/core/themes/seven/css/components/tables.css

--- a/core/themes/seven/css/components/tables.css
+++ b/core/themes/seven/css/components/tables.css

+++ b/core/themes/seven/css/components/tables.css
+++ b/core/themes/seven/css/components/tables.css
@@ -93,7 +93,7 @@ th.active > a:focus:after,

@@ -93,7 +93,7 @@ th.active > a:focus:after,
diff --git a/core/themes/seven/css/components/views-ui.css b/core/themes/seven/css/components/views-ui.css

diff --git a/core/themes/seven/css/components/views-ui.css b/core/themes/seven/css/components/views-ui.css
index ed33366..d6a159f 100644

index ed33366..d6a159f 100644
--- a/core/themes/seven/css/components/views-ui.css

--- a/core/themes/seven/css/components/views-ui.css
+++ b/core/themes/seven/css/components/views-ui.css

+++ b/core/themes/seven/css/components/views-ui.css
+++ b/core/themes/seven/css/components/views-ui.css
@@ -93,8 +93,8 @@ details.fieldset-no-legend {

The css changes should be from a different patch, not relevant here.

hehe I mixed two patches, sorry :)

Works for me. #18 passes tests and no sign of calls to format_plural function.

Thank you for working on it!

1. +++ b/core/lib/Drupal/Core/Extension/InfoParser.php
   @@ -41,7 +41,7 @@ public function parse($filename) {
   -          $message = format_plural(count($missing_keys), 'Missing required key (!missing_keys) in !file.', 'Missing required keys (!missing_keys) in !file.', array('!missing_keys' => implode(', ', $missing_keys), '!file' => $filename));
   +          $message = \Drupal::translation()->formatPlural(count($missing_keys), 'Missing required key (!missing_keys) in !file.', 'Missing required keys (!missing_keys) in !file.', array('!missing_keys' => implode(', ', $missing_keys), '!file' => $filename));
   

   This is an exeption, let's not use format_plural for that but just an english message which is plural by default.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
   @@ -77,8 +77,8 @@ public function viewElements(FieldItemListInterface $items) {
   -        $prefix = (count($prefixes) > 1) ? format_plural($item->value, $prefixes[0], $prefixes[1]) : $prefixes[0];
   -        $suffix = (count($suffixes) > 1) ? format_plural($item->value, $suffixes[0], $suffixes[1]) : $suffixes[0];
   +        $prefix = (count($prefixes) > 1) ? \Drupal::translation()->formatPlural($item->value, $prefixes[0], $prefixes[1]) : $prefixes[0];
   +        $suffix = (count($suffixes) > 1) ? \Drupal::translation()->formatPlural($item->value, $suffixes[0], $suffixes[1]) : $suffixes[0];
   

   You can use $this->formatPlural instead()

3. +++ b/core/modules/aggregator/src/Plugin/aggregator/processor/DefaultProcessor.php
   @@ -149,7 +149,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -      return ($length == 0) ? t('Unlimited') : format_plural($length, '1 character', '@count characters');
   +      return ($length == 0) ? t('Unlimited') : \Drupal::translation()->formatPlural($length, '1 character', '@count characters');
        }, array_combine($lengths, $lengths));
    
        $form['processors'][$info['id']]['aggregator_teaser_length'] = array(
   diff --git a/core/modules/block_content/src/Form/BlockContentDeleteForm.php b/core/modules/block_content/src/Form/BlockContentDeleteForm.php
   
   index 1d7724d..43b2659 100644
   --- a/core/modules/block_content/src/Form/BlockContentDeleteForm.php
   
   --- a/core/modules/block_content/src/Form/BlockContentDeleteForm.php
   +++ b/core/modules/block_content/src/Form/BlockContentDeleteForm.php
   
   +++ b/core/modules/block_content/src/Form/BlockContentDeleteForm.php
   +++ b/core/modules/block_content/src/Form/BlockContentDeleteForm.php
   @@ -44,7 +44,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   
   @@ -44,7 +44,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
        $instances = $this->entity->getInstances();
    
        $form['message'] = array(
   -      '#markup' => format_plural(count($instances), 'This will also remove 1 placed block instance.', 'This will also remove @count placed block instances.'),
   +      '#markup' => \Drupal::translation()->formatPlural(count($instances), 'This will also remove 1 placed block instance.', 'This will also remove @count placed block instances.'),
          '#access' => !empty($instances),
   
   +++ b/core/modules/block_content/src/Form/BlockContentTypeDeleteForm.php
   @@ -71,7 +71,7 @@ public function getConfirmText() {
        $blocks = $this->queryFactory->get('block_content')->condition('type', $this->entity->id())->execute();
        if (!empty($blocks)) {
   -      $caption = '<p>' . format_plural(count($blocks), '%label is used by 1 custom block on your site. You can not remove this block type until you have removed all of the %label blocks.', '%label is used by @count custom blocks on your site. You may not remove %label until you have removed all of the %label custom blocks.', array('%label' => $this->entity->label())) . '</p>';
   +      $caption = '<p>' . \Drupal::translation()->formatPlural(count($blocks), '%label is used by 1 custom block on your site. You can not remove this block type until you have removed all of the %label blocks.', '%label is used by @count custom blocks on your site. You may not remove %label until you have removed all of the %label custom blocks.', array('%label' => $this->entity->label())) . '</p>';
          $form['description'] = array('#markup' => $caption);
          return $form;
        }
   
   +++ b/core/modules/comment/src/Form/ConfirmDeleteMultiple.php
   @@ -122,7 +122,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
          $this->logger('content')->notice('Deleted @count comments.', array('@count' => $count));
   -      drupal_set_message(format_plural($count, 'Deleted 1 comment.', 'Deleted @count comments.'));
   +      drupal_set_message(\Drupal::translation()->formatPlural($count, 'Deleted 1 comment.', 'Deleted @count comments.'));
   
   +++ b/core/modules/comment/src/Tests/CommentNonNodeTest.php
   @@ -205,7 +205,7 @@ function performCommentOperation($comment, $operation, $approval = FALSE) {
          $this->drupalPostForm(NULL, array(), t('Delete comments'));
   

   Did you considered to use the StringTranslationTrait instead? It provides a formatPlural method already.

4. +++ b/core/modules/config/src/Form/ConfigSync.php
   @@ -238,19 +238,19 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -            $form[$collection][$config_change_type]['heading']['#value'] = format_plural(count($config_names), '@count new', '@count new');
   +            $form[$collection][$config_change_type]['heading']['#value'] = \Drupal::translation()->formatPlural(count($config_names), '@count new', '@count new');
                break;
    
              case 'update':
   -            $form[$collection][$config_change_type]['heading']['#value'] = format_plural(count($config_names), '@count changed', '@count changed');
   +            $form[$collection][$config_change_type]['heading']['#value'] = \Drupal::translation()->formatPlural(count($config_names), '@count changed', '@count changed');
                break;
    
              case 'delete':
   -            $form[$collection][$config_change_type]['heading']['#value'] = format_plural(count($config_names), '@count removed', '@count removed');
   +            $form[$collection][$config_change_type]['heading']['#value'] = \Drupal::translation()->formatPlural(count($config_names), '@count removed', '@count removed');
                break;
    
              case 'rename':
   -            $form[$collection][$config_change_type]['heading']['#value'] = format_plural(count($config_names), '@count renamed', '@count renamed');
   +            $form[$collection][$config_change_type]['heading']['#value'] = \Drupal::translation()->formatPlural(count($config_names), '@count renamed', '@count renamed');
   

   You can already use $this->formatPlural() here

5. +++ b/core/modules/menu_ui/src/Form/MenuDeleteForm.php
   @@ -76,7 +76,7 @@ public function getDescription() {
        if ($num_links) {
   -      $caption .= '<p>' . format_plural($num_links, '<strong>Warning:</strong> There is currently 1 menu link in %title. It will be deleted (system-defined items will be reset).', '<strong>Warning:</strong> There are currently @count menu links in %title. They will be deleted (system-defined links will be reset).', array('%title' => $this->entity->label())) . '</p>';
   +      $caption .= '<p>' . \Drupal::translation()->formatPlural($num_links, '<strong>Warning:</strong> There is currently 1 menu link in %title. It will be deleted (system-defined items will be reset).', '<strong>Warning:</strong> There are currently @count menu links in %title. They will be deleted (system-defined links will be reset).', array('%title' => $this->entity->label())) . '</p>';
   

   This could also use $this->formatPlural()

wow @dawehner thank you :)
and... fixed!

Hey all,

The patch #22 applies without errors. The only place where function format_plural was found in InfoParser.php.

Yes @dawehner said me:

This is an exeption, let's not use format_plural for that but just an english message which is plural by default.

Then RTBC...

Actually, you should still remove the format_plural. Like this:

$message = String::format('Missing required keys (!missing_keys) in !file.', array('!missing_keys' => implode(', ', $missing_keys), '!file' => $filename));

That's probably what @dawehner meant.

I went ahead and made the change. This fixes @dawehner's point 1 in #21.

Fixed. Removed that test that checked for the singular format, because it was removed.

Patch applied cleanly for me. I grepped and didn't find more references to the function.

Patch applied cleanly for me too. There were 3 instances of format_plural in the system module before applying the patch and these were all replaced after the patch was applied.

Patch submitted in #31 applied cleanly and all the instances of format_plural is being replaced by \Drupal::translation()->formatPlural().

Reroll

Fix comments length :)

rerolled...

Reviewing now

+++ b/core/modules/system/src/Tests/Extension/InfoParserUnitTest.php
@@ -67,17 +67,6 @@ public function testInfoParser() {
-    // Tests that a single missing required key is detected.
-    $filename = 'core/modules/system/tests/fixtures/missing_key.info.txt';
-    try {
-      $this->infoParser->parse($filename);
-      $this->fail('Expected InfoParserException not thrown when reading missing_key.info.txt');
-    }
-    catch (InfoParserException $e) {
-      $expected_message = "Missing required key (type) in $filename.";
-      $this->assertEqual($e->getMessage(), $expected_message);
-    }
-

Why is this needed?

Patch 41 applies correctly, all formal_plural instances were removed, (also checked comments), however there is still an outstanding question related to this - #31

1. +++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
   @@ -51,7 +51,7 @@ public function compile(\Twig_Compiler $compiler) {
   -    $compiler->write('echo ' . (empty($plural) ? 't' : 'format_plural') . '(');
   +    $compiler->write('echo ' . (empty($plural) ? 't' : '\Drupal::translation()->formatPlural') . '(');
   

   Has this been proven to work, as opposed to just not break a test?

2. +++ b/core/lib/Drupal/Core/Validation/DrupalTranslator.php
   @@ -41,7 +41,7 @@ public function transChoice($id, $number, array $parameters = array(), $domain =
   -    return format_plural($number, $ids[0], $ids[1], $this->processParameters($parameters), $this->getOptions($domain, $locale));
   +    return \Drupal::translation()->formatPlural($number, $ids[0], $ids[1], $this->processParameters($parameters), $this->getOptions($domain, $locale));
   

   This is OO code, so should be using an injected translation object instead of \Drupal::translation()

3. +++ b/core/misc/drupal.js
   @@ -336,8 +336,8 @@ if (window.jQuery) {
   -   * See the documentation of the server-side format_plural() function for
   -   * further details.
   +   * See the documentation of the server-side \Drupal::translation()->formatPlural()
   +   * function for further details.
   

   Comments should wrap at 80 chars and use the interface name, not \Drupal::translation()

4. +++ b/core/modules/block_content/src/Tests/BlockContentCreationTest.php
   @@ -184,7 +184,7 @@ public function testBlockDelete() {
   -    $this->assertText(format_plural(1, 'This will also remove 1 placed block instance.', 'This will also remove @count placed block instance.'));
   +    $this->assertText(\Drupal::translation()->formatPlural(1, 'This will also remove 1 placed block instance.', 'This will also remove @count placed block instance.'));
   

   Can we use the OO-style in tests? If so, there are lots of tests to update.

5. +++ b/core/modules/node/src/Entity/NodeType.php
   @@ -162,7 +162,7 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   -        drupal_set_message(format_plural($update_count,
   +        drupal_set_message(\Drupal::translation()->formatPlural($update_count,
   

   OO code, should be using injection instead of Drupal::

6. +++ b/core/modules/search/src/Tests/SearchMultilingualEntityTest.php
   @@ -235,7 +235,8 @@ protected function assertIndexCounts($remaining, $total, $message) {
   +    // test avoids using \Drupal::translation()->formatPlural(), so it tests for
   

   This should use the interface name

7. -          $message = format_plural(count($missing_keys), 'Missing required key (!missing_keys) in !file.', 'Missing required keys (!missing_keys) in !file.', array('!missing_keys' => implode(', ', $missing_keys), '!file' => $filename));
   +          $message = String::format('Missing required keys (!missing_keys) in !file.', array('!missing_keys' => implode(', ', $missing_keys), '!file' => $filename));
   +++ b/core/modules/system/src/Tests/Extension/InfoParserUnitTest.php
   @@ -67,17 +67,6 @@ public function testInfoParser() {
   -    // Tests that a single missing required key is detected.
   -    $filename = 'core/modules/system/tests/fixtures/missing_key.info.txt';
   -    try {
   -      $this->infoParser->parse($filename);
   -      $this->fail('Expected InfoParserException not thrown when reading missing_key.info.txt');
   -    }
   -    catch (InfoParserException $e) {
   -      $expected_message = "Missing required key (type) in $filename.";
   -      $this->assertEqual($e->getMessage(), $expected_message);
   -    }
   -
   +++ /dev/null
   @@ -1,8 +0,0 @@
   -# info.yml for testing missing type key.
   -name: File
   -description: 'Defines a file field type.'
   -package: Core
   -version: VERSION
   -core: 8.x
   -dependencies:
   -  - field
   

   These changes are all because of the review comment on #21 that we shouldn't bother using format_plural in exception messages. I've not verified that this changes are correct.

Attempting a reroll first. I will set to 'Needs work' for comment feedback in #44 once the tests pass. This is a reroll from #38 directly, as #41 was just a reroll.

Oops... Forgot the file.

Make sure you use the latest checkout of 8.0.x from git (the most recent beta will usually be too old). Your patch is much smaller than the previous one which is a warning sign. Did you get conflicts locally, and lose those changes?

Please attach an interdiff so others can see your changes, see https://www.drupal.org/documentation/git/interdiff

@ianthomas_uk, thank you for assistance.

Now I've cloned the latest 8.0.x branch.
In this case my patch is not a modification of an existing patch. So I don't need provide an interdiff?
Or I'm wrong?

Your patch should be a modification of #46, which itself was a reroll of #41 so should address the review comments since then.

#46 is 68.55KB, but #52 is 12.5KB. Why is there a huge difference in size?

Why are you testing old patches? That's just adding noise to this issue and hogging the test bots so other people can't test their patches. It's rare for test results to change, unless the previous test had failed due to a temperamental test (which can happen occasionally) or a patch needs a reroll to apply to HEAD (which means your new test result will just say patch does not apply).

Sorry for the noise.

I tried to use the last passed tests patch - it doesn't work for me.
So I tried to find out which one of the last patches I may use for modification.

Please stop retesting patches like that, it won't tell you anything. #46 is the patch you should use as your starting point.

Ok!

Please, correct me if I'm wrong.
To resolve the issue I need apply #46 patch and resolve the remaining usage of format_plural function?

Read and understand the comments above.

- Yes, apply #46 as your starting point, resolving any conflicts that occur.
- If there are any remaining calls to format_plural then they will need to be resolved, but #33 and #34 suggest that there aren't any remaining. Are you sure you haven't just failed to resolve a conflict properly?
- Look for any review comments that haven't been addressed, in this case #43 and #44 (because as #46 says, it was only a reroll and doesn't address those comments).
- Upload your patch and interdiff with a brief note explaining what you've done

Thanks for explaining.

But what I should do if "git apply 2309737-45.patch" returns:

error: patch failed: core/modules/system/system.admin.inc:304
error: core/modules/system/system.admin.inc: patch does not apply

I tried to remove all format_plural usage in #69 from scratch.

See https://www.drupal.org/patch/reroll

@max-kuzomko Thank you for working on this issue. If you are a new contributor, consider attending the Drupal core mentoring office hours https://www.drupal.org/core-office-hours. The mentors can explain the process, from start to finish.

Thanks to all for assistance.

Rerolled #46

I am a bit confused with whatever has happened between comments 46 to 78. So, let me start with a reroll of #46.

#76 and #79 are identical. I guess that validates the reroll at least ;)

Still needs #43/#44 to be addressed though.

For #43.1: I don't see why that block was removed in this issue. It seems it was removed because there is a similar test just before it, but I think it doesn't hurt to test it under slightly different conditions. Anyway, it may not be in scope of this issue. I reverted that change.

#44.1: I think it should work. Wherever format_plural could be called, the \Drupal object would still be in scope. Can you be a bit more specific on how you mean "proven"?

#44.2: I am trying to figure this out. There are two methods I can think of:
1. Make DrupalTranslator a service, so that we can get the service container injected early.
2. Directly pass translation object from the caller. There is only one caller - TypedDataManager, which too does not have a container.

#44.3: Changed.

#44.4: Do you mean injecting the translator? I believe we don't need to do that for tests. If the tests mean to use a different container, they typically call \Drupal::setContainer.

#44.5: Same reasons as point 2. Another note here is that this class calls \Drupal elsewhere directly. I know that this is usually not recommended but I can't think of how to make this a service or inject the container. Any suggestions?

#44.6: Done. It looks a bit weird, but I guess we have no choice.

#44.7: I think this is fine. Using format_plural could be subjective but I think we're fine with String::format in this case, especially because we are using String::format for the same reason a few lines before this.

Interdiff and patch attached.

#44.1 I mean has anyone run the code (that specific line) locally, and confirmed that it works and does what it's meant to. It's too easy with these sort of patches to just assume the tests will catch any mistakes.

#44.2 If we can't work out how to inject it that doesn't need to block the patch from being committed, we're still making progress.

#44.4 You're right, we can use \Drupal for tests. I hadn't noticed it was test code.

(I've not actually looked at the patch yet, just thought you might appreciate answers to your questions).

#81 patch applying failed.

Rerolling with the test fix. Hopefully should pass now.

+++ b/core/modules/system/tests/fixtures/missing_key.info.txt
@@ -1,5 +1,6 @@
+type: module

This is the failure we are expecting here. We can't add it as we are actually expecting the exception.

The reason for failure in #81 (which was valid) is that we are no longer using format_plural for the exception message. It is expecting value 'Missing required keys (type) in core/modules/system/tests/fixtures/missing_key.info.txt.' is equal to value 'Missing required key (type) in core/modules/system/tests/fixtures/missing_key.info.txt.' (note the plural keys). I am changing the expected value to match the new actual value.

The interdiff is against #85 as it is also a reroll. I have reverted the change to missing_key.info.txt.

#90 needed a simple reroll due to a conflict in a test, but is otherwise fine.

I've checked that the line I was worried about in #44.1 does work, although the only current use of it in core is in a test (TwigTransTest, see #2047135: Added support for the Twig {% trans %} tag extension).

We could perhaps use dependency injection in a couple more places (#44.2 and #44.5), but I can't see how we're meant to do that. This at least makes the existing dependency more obvious and easier to refactor due to the use of \Drupal.

All my concerns have been addressed, there are no other calls to format_plural, therefore RTBC.

Committed aff9577 and pushed to 8.0.x. Thanks!

The meta issue has the beta evaluation.

Add this to the CR https://www.drupal.org/node/2173787 - can people try to add the "remove deprecated x usage" issues to the correct CRs before commits. Thanks.