Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#18 | format_plural-2111349.patch | 18.12 KB | dawehner |
#10 | format_plural-2111349-10.patch | 15.58 KB | tim.plunkett |
#10 | interdiff.txt | 3.42 KB | tim.plunkett |
Comments
Comment #1
dawehnerI am not sure yet how to handle the locale_get_plural() call.
Comment #2
dawehner.
Comment #3
tim.plunkettThese are a little odd, I think most just have the
@deprecated as of Drupal 8.0. Use \Drupal::translation()->formatPlural()
maybe put the example in quotes? It's not clear what is part of it, the "like" or the parens
Trailing comma
public function
Comment #4
dawehnerThis is way better. Thank you.
Good catch.
Additional I did a little bit of test coverage for formatInterval.
Comment #6
dawehner.
Comment #7
vijaycs85looks great. Thanks @dawehner. +1 to RTBC.
Comment #8
vijaycs85lets get this in.
Comment #9
vijaycs85needs a re-roll :(
Comment #10
tim.plunkettThe 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.
Comment #12
tim.plunkett10: format_plural-2111349-10.patch queued for re-testing.
Comment #14
tim.plunkett10: format_plural-2111349-10.patch queued for re-testing.
Comment #16
mikey_p CreditAttribution: mikey_p commented10: format_plural-2111349-10.patch queued for re-testing.
Comment #17
mikey_p CreditAttribution: mikey_p commentedI'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.
Comment #18
dawehnerGood idea.
Comment #19
mikey_p CreditAttribution: mikey_p commentedLooks good. Thanks!
Comment #20
Gábor HojtsyComment #21
webchickCommitted and pushed to 8.x. Thanks!
Now I guess we need another patch to update all of those function calls to method calls?
Comment #22
penyaskito@vijaycs85 created #2149195: Replace format_plural with \Drupal::translation()->formatPlural() and #2149197: Replace format_interval with \Drupal::service('date')->formatInterval()
Comment #23
tim.plunkettI think this should have a change notice.
Comment #24
TR CreditAttribution: TR commentedYeah, 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>
Comment #25
Gábor Hojtsy@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?
Comment #26
Gábor HojtsyOpened #2151527: Support for formatPlural() in PHP as format_plural() is deprecated in potx module to track this change in the extractor.
Comment #27
jgSnell CreditAttribution: jgSnell commentedWorking on the change record.
Comment #28
jgSnell CreditAttribution: jgSnell commentedProposed 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
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) { . . . }
Comment #29
dawehner@jgSnell
Instead of describing how the code changed before and after it would be better to show examples of usage before and after.
Comment #30
jgSnell CreditAttribution: jgSnell commentedThanks, @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
After
---
Example: Formatting how long ago a node was created
Before
After
Comment #31
Gábor Hojtsy@jgSnell: great, please create a change record with that :) https://drupal.org/node/add/changenotice
Comment #32
penyaskitoI'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.
Comment #33
Gábor HojtsyYeah, true. Make it t('@time ago') :)
Comment #34
penyaskitoSo:
Example: Formatting how long ago a node was created
Before
After
Comment #35
sunComment #36
dawehner+1
It is just a small step left to get the change notice out.
Comment #37
jgSnell CreditAttribution: jgSnell commentedWorking on the notice now.
Comment #38
jgSnell CreditAttribution: jgSnell commentedChange notice: https://drupal.org/node/2173787
Comment #39
jgSnell CreditAttribution: jgSnell commentedComment #40
jgSnell CreditAttribution: jgSnell commentedComment #41
Gábor HojtsyYay, can remove from sprint then :)