Updated: Comment 0

Problem/Motivation

Proposed resolution

Move format_plural to the string translation service and format_interval to the date service.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Issue tags: +D8MI
FileSize
11.16 KB

I am not sure yet how to handle the locale_get_plural() call.

dawehner’s picture

Status: Active » Needs review

.

tim.plunkett’s picture

  1. +++ b/core/includes/common.inc
    @@ -941,39 +941,12 @@ function format_xml_elements($array) {
    + * @deprecated as of Drupal 8.0. Use the string_translation service formatPlural
    + *   method.
    
    @@ -1051,31 +1024,11 @@ function format_size($size, $langcode = NULL) {
    + * @deprecated as of Drupal 8.0. Use the date service formatInterval method.
    

    These are a little odd, I think most just have the @deprecated as of Drupal 8.0. Use \Drupal::translation()->formatPlural()

  2. +++ b/core/lib/Drupal/Core/Datetime/Date.php
    @@ -43,16 +44,37 @@ class Date {
    +   * This array is keyed by strings representating the unit (like 1 year|@count
    +   * years) and with the amount of values of the unit in seconds.
    

    maybe put the example in quotes? It's not clear what is part of it, the "like" or the parens

  3. +++ b/core/lib/Drupal/Core/Datetime/Date.php
    @@ -43,16 +44,37 @@ class Date {
    +    '1 sec|@count sec' => 1
    

    Trailing comma

  4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -31,4 +31,53 @@
    +  function formatPlural($count, $singular, $plural, array $args = array(), array $options = array());
    
    +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -127,6 +127,43 @@ public function translate($string, array $args = array(), array $options = array
    +  function formatPlural($count, $singular, $plural, array $args = array(), array $options = array()) {
    

    public function

dawehner’s picture

Issue tags: +PHPUnit
FileSize
15.42 KB
8.34 KB

These are a little odd, I think most just have the @deprecated as of Drupal 8.0. Use \Drupal::translation()->formatPlural()

This is way better. Thank you.

public function

Good catch.

Additional I did a little bit of test coverage for formatInterval.

Status: Needs review » Needs work

The last submitted patch, date-2111349-4.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
388 bytes
15.42 KB

.

vijaycs85’s picture

looks great. Thanks @dawehner. +1 to RTBC.

vijaycs85’s picture

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

lets get this in.

vijaycs85’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

needs a re-roll :(

tim.plunkett’s picture

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

The conflict was just on EntityManager/EntityManagerInterface.

format_interval() has a default value of 2 for granularity, this patch was removing it from the method. I've added it in and added test coverage as well.

Status: Needs review » Needs work

The last submitted patch, 10: format_plural-2111349-10.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

10: format_plural-2111349-10.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 10: format_plural-2111349-10.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

10: format_plural-2111349-10.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 10: format_plural-2111349-10.patch, failed testing.

mikey_p’s picture

Status: Needs work » Needs review

10: format_plural-2111349-10.patch queued for re-testing.

mikey_p’s picture

I'm not sure how this works, but shouldn't TranslationManager have some tests? It seems like that would't be too hard except for the call to language_default() in the constructor.

dawehner’s picture

FileSize
18.12 KB

Good idea.

mikey_p’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Thanks!

Gábor Hojtsy’s picture

Issue tags: +language-ui, +sprint
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Now I guess we need another patch to update all of those function calls to method calls?

penyaskito’s picture

tim.plunkett’s picture

Title: Move format_plural to the string translation service and format_interval to the date service. » Change notice: Move format_plural to the string translation service and format_interval to the date service.
Priority: Normal » Major
Status: Fixed » Active
Issue tags: +Needs change record

I think this should have a change notice.

TR’s picture

Yeah, definitely needs a change notice. I pulled the latest D8 HEAD this evening and all sorts of things broke. There's nothing mentioned in the change notices yet, and the drupal.org issue queue search is still totally broken after the migration so I can't even search for format_plural. I found this issue by pure luck, paging through the issue queue.

It's pretty frustrating trying to do D8 development when a lot of things aren't being documented along the way. Even the change notices aren't sufficient, because all they tell you is that A changed to B. But there's so much being changed that B is replaced by C, then C is replaced by D, and a search (when it even works, and when there are change notices actually written) doesn't tell you anything about D. All you know is that you're getting error messages about A, with no guidance about what has replaced it.

</rant>

Gábor Hojtsy’s picture

@webchick, @penyaskito: so we are also using t() a lot of global places and we did not replace them with \Drupal::translation()->translate() either. Also looks like where t() is put on as a method we would need formatPlural() or something along those line as well? Not just simply replace the global functions with global static method calls?

Gábor Hojtsy’s picture

Opened #2151527: Support for formatPlural() in PHP as format_plural() is deprecated in potx module to track this change in the extractor.

jgSnell’s picture

Assigned: Unassigned » jgSnell

Working on the change record.

jgSnell’s picture

Status: Active » Needs review

Proposed change notice:

Topic
format_plural has moved to translation service as formatPlural. formatInterval has moved to date service as formatInterval.

Description
format_plural() and format_interval() have been deprecated. Use \Drupal::translation()->formatPlural() and \Drupal::service('date')->formatInterval() instead.

Summary

  • Deprecated format_plural() and format_interval()
  • Created \Drupal::translation()->formatPlural()
  • Created \Drupal::service('date')->formatInterval()
  • Added test cases for above functions

Before (in /core/include/common.inc)

function format_plural($count, $singular, $plural, array $args = array(), array $options = array()) { . . . }

After in (/core/lib/Drupal/Core/StringTranslation/TranslationManager.php)

public function formatPlural($count, $singular, $plural, array $args = array(), array $options = array()) { . . . }

---

Before (in/core/include/common.inc)

function format_interval($interval, $granularity = 2, $langcode = NULL) { . . . }

After (in /core/lib/Drupal/Datetime/Date.php)

public function formatInterval($interval, $granularity = 2, $langcode = NULL) { . . . }

dawehner’s picture

@jgSnell
Instead of describing how the code changed before and after it would be better to show examples of usage before and after.

jgSnell’s picture

Thanks, @dawehner. Still getting the hang of this. :)

Perhaps change out the before/after bits in #28 with the below?

Example: Formatting total comment count plural text

Before

<?php
$output = format_plural($node->comment_count, '1 comment', '@count comments');
?>

After

<?php
$output = \Drupal::translation()->formatPlural($node->comment_count, '1 comment', '@count comments');
?>

---
Example: Formatting how long ago a node was created

Before

<?php
$ago = format_interval((time() - $node->created) , 2) . t('ago');
?>

After

<?php
$ago = \Drupal::service('date')->formatInterval((time() - $node->created) , 2) . t('ago');
?>
Gábor Hojtsy’s picture

@jgSnell: great, please create a change record with that :) https://drupal.org/node/add/changenotice

penyaskito’s picture

I'm not quite happy with the formatInterval example, and probably is not a problem with the example itself but a bug somewhere in our code.

formatInterval() checks the best unit for a given interval, and formats the number in that date, and then translate. After that, we concat the translation of 'ago'.

But '2 years ago' would be translated to 'hace 2 años' in Spanish, and we don't have a way of translating that in a clean way.

Gábor Hojtsy’s picture

Yeah, true. Make it t('@time ago') :)

penyaskito’s picture

So:

Example: Formatting how long ago a node was created

Before

$ago = t('@time ago',  array('@time' => format_interval((time() - $node->created) , 2));

After

$ago = t('@time ago',  array('@time' =>  \Drupal::service('date')->formatInterval((time() - $node->created) , 2));
sun’s picture

Issue tags: +@deprecated
dawehner’s picture

+1
It is just a small step left to get the change notice out.

jgSnell’s picture

Working on the notice now.

jgSnell’s picture

jgSnell’s picture

Assigned: jgSnell » Unassigned
Priority: Major » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record
jgSnell’s picture

Title: Change notice: Move format_plural to the string translation service and format_interval to the date service. » Move format_plural to the string translation service and format_interval to the date service.
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, can remove from sprint then :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.