Replace format_plural with \Drupal::translation()->formatPlural()

Shouldn't we inject the translation service?

Shouldn't we inject the translation service?

My thoughts!

We should consider adding it to FormBase.

Exactly, we should not just simply replace the global function calls with global static method calls.

I must do the same in FormBase and PluginBase...

BTW we have #2151527: Support for formatPlural() in PHP as format_plural() is deprecated open in potx to track this change in the parser.

Let's try to change it.

i dont see any phpunit tests for reviews, so i guess the correct tag is this one

And since i am here

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockCreationTest.php
@@ -188,7 +188,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.'));

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNonNodeTest.php
@@ -197,7 +197,7 @@ function performCommentOperation($comment, $operation, $approval = FALSE) {
-      $this->assertRaw(format_plural(1, 'Deleted 1 comment.', 'Deleted @count comments.'), format_string('Operation "@operation" was performed on comment.', array('@operation' => $operation)));
+      $this->assertRaw(\Drupal::translation()->formatPlural(1, 'Deleted 1 comment.', 'Deleted @count comments.'), format_string('Operation "@operation" was performed on comment.', array('@operation' => $operation)));

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.php
@@ -311,7 +311,7 @@ function performCommentOperation(CommentInterface $comment, $operation, $approva
-      $this->assertRaw(format_plural(1, 'Deleted 1 comment.', 'Deleted @count comments.'), format_string('Operation "@operation" was performed on comment.', array('@operation' => $operation)));
+      $this->assertRaw(\Drupal::translation()->formatPlural(1, 'Deleted 1 comment.', 'Deleted @count comments.'), format_string('Operation "@operation" was performed on comment.', array('@operation' => $operation)));

+++ b/core/modules/file/lib/Drupal/file/Tests/FileManagedTestBase.php
@@ -78,7 +78,7 @@ function assertFileHookCalled($hook, $expected_count = 1, $message = NULL) {
-        $message = format_plural($actual_count, 'hook_file_@name was not expected to be called but was actually called once.', 'hook_file_@name was not expected to be called but was actually called @count times.', array('@name' => $hook, '@count' => $actual_count));
+        $message = \Drupal::translation()->formatPlural($actual_count, 'hook_file_@name was not expected to be called but was actually called once.', 'hook_file_@name was not expected to be called but was actually called @count times.', array('@name' => $hook, '@count' => $actual_count));

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.php
@@ -167,7 +167,7 @@ function testForum() {
-    $unread_topics = format_plural($unread_topics, '1 new post', '@count new posts');
+    $unread_topics = \Drupal::translation()->formatPlural($unread_topics, '1 new post', '@count new posts');

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocalePluralFormatTest.php
@@ -126,7 +126,7 @@ function testGetPluralFormat() {
-        $this->assertIdentical(format_plural($count, '1 hour', '@count hours', array(), array('langcode' => $langcode)), $expected_plural_string, 'Plural translation of 1 hours / @count hours for count ' . $count . ' in ' . $langcode . ' is ' . $expected_plural_string);
+        $this->assertIdentical(\Drupal::translation()->formatPlural($count, '1 hour', '@count hours', array(), array('langcode' => $langcode)), $expected_plural_string, 'Plural translation of 1 hours / @count hours for count ' . $count . ' in ' . $langcode . ' is ' . $expected_plural_string);

@@ -214,7 +214,7 @@ function testPluralEditExport() {
-    format_plural(1, '1 day', '@count days', array(), array('langcode' => 'fr'));
+    \Drupal::translation()->formatPlural(1, '1 day', '@count days', array(), array('langcode' => 'fr'));

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -1078,7 +1078,7 @@ protected function tearDown() {
-        $message = format_plural($emailCount, '1 e-mail was sent during this test.', '@count e-mails were sent during this test.');
+        $message = \Drupal::translation()->formatPlural($emailCount, '1 e-mail was sent during this test.', '@count e-mails were sent during this test.');

+++ b/core/modules/user/lib/Drupal/user/Tests/UserLoginTest.php
@@ -149,7 +149,7 @@ function assertFailedLogin($account, $flood_trigger = NULL) {
-        $this->assertRaw(format_plural(\Drupal::config('user.flood')->get('user_limit'), 'Sorry, there has been more than one failed login attempt for this account. It is temporarily blocked. Try again later or <a href="@url">request a new password</a>.', 'Sorry, there have been more than @count failed login attempts for this account. It is temporarily blocked. Try again later or <a href="@url">request a new password</a>.', array('@url' => url('user/password'))));
+        $this->assertRaw(\Drupal::translation()->formatPlural(\Drupal::config('user.flood')->get('user_limit'), 'Sorry, there has been more than one failed login attempt for this account. It is temporarily blocked. Try again later or <a href="@url">request a new password</a>.', 'Sorry, there have been more than @count failed login attempts for this account. It is temporarily blocked. Try again later or <a href="@url">request a new password</a>.', array('@url' => url('user/password'))));

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterUserNameTest.php
@@ -108,7 +108,7 @@ public function testAdminUserInterface() {
-    $message = format_plural(count($users), 'Unable to find user: @users', 'Unable to find users: @users', array('@users' => implode(', ', $users)));
+    $message = \Drupal::translation()->formatPlural(count($users), 'Unable to find user: @users', 'Unable to find users: @users', array('@users' => implode(', ', $users)));

@@ -120,7 +120,7 @@ public function testAdminUserInterface() {
-    $message = format_plural(count($users), 'Unable to find user: @users', 'Unable to find users: @users', array('@users' => implode(', ', $users)));
+    $message = \Drupal::translation()->formatPlural(count($users), 'Unable to find user: @users', 'Unable to find users: @users', array('@users' => implode(', ', $users)));

@@ -130,7 +130,7 @@ public function testAdminUserInterface() {
-    $message = format_plural(count($users), 'Unable to find user: @users', 'Unable to find users: @users', array('@users' => implode(', ', $users)));
+    $message = \Drupal::translation()->formatPlural(count($users), 'Unable to find user: @users', 'Unable to find users: @users', array('@users' => implode(', ', $users)));

@@ -147,7 +147,7 @@ public function testExposedFilter() {
-    $message = format_plural(count($users), 'Unable to find user: @users', 'Unable to find users: @users', array('@users' => implode(', ', $users)));
+    $message = \Drupal::translation()->formatPlural(count($users), 'Unable to find user: @users', 'Unable to find users: @users', array('@users' => implode(', ', $users)));

@@ -157,7 +157,7 @@ public function testExposedFilter() {
-    $message = format_plural(count($users), 'Unable to find user: @users', 'Unable to find users: @users', array('@users' => implode(', ', $users)));
+    $message = \Drupal::translation()->formatPlural(count($users), 'Unable to find user: @users', 'Unable to find users: @users', array('@users' => implode(', ', $users)));

we are supposed to use $this->container->get() inside simpletests instead of \Drupal

Also a reroll

Rerolled and used $this->container->get('string_translation')->formatPlural() in tests. Thanks for the review, Paris!

#2087751: [policy, no patch] Stop using $this->container in web tests :(

Plain reroll of #16 per #20.

21: 2149195-formatPlural-21.patch queued for re-testing.

21: 2149195-formatPlural-21.patch queued for re-testing.

There aren't any out of scope changes and the patch is green.

+++ b/core/includes/common.inc
@@ -862,12 +862,12 @@ function format_xml_elements($array) {
- *   $output = format_plural($node->comment_count, '1 comment', '@count comments');
+ *   $output = \Drupal::translation()->formatPlural($node->comment_count, '1 comment', '@count comments');

+++ b/core/lib/Drupal/Core/Update/Form/UpdateScriptSelectionForm.php
@@ -105,7 +105,7 @@ public function buildForm(array $form, array &$form_state) {
-        $form['start']['#title'] = format_plural(
+        $form['start']['#title'] = $this->formatPlural(

Hm. We've gone from "always call format_plural()" to sometimes call $this->formatPlural() and other times call \Drupal::translation()->formatPlural(). This does not seem to be a huge DX win for me. :\

t() is a bit different because in either case you're calling (whatever OO crap)->t() as opposed to sometimes (but not always) needing to know it's buried underneath a top-level "translation()" object instead.

In other words, I think we should either make it a top-level method on Drupal, or we should make the base classes mock translation() as well, so calls to this can be consistent throughout the code base.

Why..? That is exposing super unimportant implementation details to someone who just. wants. to. freaking. print. a. string. :)

Why..? That is exposing super unimportant implementation details to someone who just. wants. to. freaking. print. a. string. :)

Wait for traits, which will add t()\formatPlural() to every possible class.

deally we should use \Drupal::translation()->translate() instead of t().

Nope, because that would break localize.d.o, which is the entire reason t() exists.

Wait for traits, which will add t()\formatPlural() to every possible class.

Yep, this. See #2079797: Provide a trait for $this->t() and $this->formatPlural(), we need an equivalent.

Yeah, traits are where we ultimately want to go, but there are a pile of dependencies between here and there, starting with #2135199-35: Provide php 5.4 testing on testbots for D8 code without breaking everything else.

My concerns for the purposes of committing this particular patch can be addressed by merely adding a formatPlural() method straight onto the Drupal class, so it retains parity with t(). If we can't or won't do that for some reason, I'd love to understand why, and also how we plan to explain to module developers why sometimes they need an extra ->translate() indirection prior to calling the function they have in mind.

My concerns for the purposes of committing this particular patch can be addressed by merely adding a formatPlural() method straight onto the Drupal class, so it retains parity with t(). If we can't or won't do that for some reason, I'd love to understand why, and also how we plan to explain to module developers why sometimes they need an extra ->translate() indirection prior to calling the function they have in mind.

We don't have a t() method on the \Drupal class as well for a good reason. We need to encourage people to write proper code/inject dependencies instead of writing sort of procedural code. People will understand that they need a method on a class or a dependency before they can do anything.

Just in case, here is also a reroll.

Reworked the latest patch, leaving out the static method call \Drupal::translation()->formatPlural(). There is no agreement on whether to use the static method instead of format_plural, but on the implementation of formatPlural in the Base Classes there is.

The forum block tests fail. Is that using views already? :) No direct forum changes are in this patch.

42: format_plural-2149195_42.patch queued for re-testing.

Remove from sprint.

reroll. let's see what's failing, then I'll try fixing it.

reverted a couple of $this->formatPlural()s in procedural code.

This now sets up formatPlural() on base classes like t() and uses it as appropriate. This looks sane from a DX point of view so there is no difference between t() and format_plural() for calling a local method vs. a global function.

Looks like this is covered in the change record at https://drupal.org/node/2173787

Committed and pushed to 8.x. Thanks!

Yay, thanks all!