Deprecate format_size() and use Drupal\Core\StringTranslation\ByteSizeMarkup instead

+++ b/core/core.services.yml
@@ -589,6 +589,9 @@ services:
+  size:
+    class: Drupal\Core\Utility\Size
+    arguments: ['@string_translation']

Utility classes are not usually exposed as services, is there any reason this one needs to be?

1. +++ b/core/lib/Drupal/Core/Utility/Size.php
   @@ -0,0 +1,104 @@
   +        $this->translation->translate('@size KB', array(), $options),
   +        $this->translation->translate('@size MB', array(), $options),
   +        $this->translation->translate('@size GB', array(), $options),
   +        $this->translation->translate('@size TB', array(), $options),
   +        $this->translation->translate('@size PB', array(), $options),
   +        $this->translation->translate('@size EB', array(), $options),
   +        $this->translation->translate('@size ZB', array(), $options),
   +        $this->translation->translate('@size YB', array(), $options),
   

   This won't work for localize.d.o. It *has* to be a method called t(), used like $this->t().
   You can copy/paste one of the various other t() methods with their shortened documentation, like in FormBase, until traits happen.

2. +++ b/core/lib/Drupal/Core/Utility/Size.php
   @@ -0,0 +1,104 @@
   +   * @return
   ...
   +   * @param $langcode
   ...
   +   * @param $size
   ...
   +   * @param $size
   ...
   +   * @return
   

   Missing data types

3. +++ b/core/lib/Drupal/Core/Utility/Size.php
   @@ -0,0 +1,104 @@
   +  function format($size, $langcode = NULL) {
   ...
   +  function parse($size) {
   

   public function

4. +++ b/core/lib/Drupal/Core/Utility/Size.php
   @@ -0,0 +1,104 @@
   +namespace Drupal\Core\Utility;
   

   I'm not sure this is the best place for this. Every other Utility class contains only static methods, without any dependency on other services.

+++ b/core/tests/Drupal/Tests/Core/Common/SizeUnitTest.php
@@ -0,0 +1,136 @@
+    foreach (array($this->exactTestCases, $this->roundedTestCases) as $test_cases) {
...
+  function testCommonFormatSize() {
...
+    foreach ($this->exactTestCases as $size) {
...
+  function testCommonParseSizeFormatSize() {
...
+  function testCommonParseSize() {
+    foreach ($this->exactTestCases as $string => $size) {

I wonder whether we should have the same dataProvider just reused multiple times?

using \Drupal\Core\Utility\Size is totally fine. We have other utility services like the link generator which aren't static ones.

Worked on making it more phpunit-ish

Let's see whether this is enough.

.

I agree that we should do something about these helpers, but a "size" service (name) looks/sounds suboptimal.

In essence, these helper functions fall under the umbrella of language/locale dependent (string) output formatting (templating).

In addition, the output is meant for HTML - and may not be suitable for e.g. CLI output. However, code that generates plain text output for e.g. CLI often needs the identical helper functions, but sans HTML output escaping. Sorta like HtmlLocale extends Locale or similar.

FWIW, format_date() & Co fall under the same umbrella.

This was partly done (https://www.drupal.org/node/2253127).

This adds Bytes::format()

Interesting fail here:

Checking for illegal reference to 'Drupal\Core' namespace in /var/www/html/core/lib/Drupal/Component/Utility/Bytes.php
Failed asserting that an array is empty.

which is caused by the class Drupal\Core\StringTranslation\TranslatableMarkup...

#32:

Interesting fail here

That's because you cannot call classes in the Drupal\Core namespace from the Drupal\Component namespace. The component would no longer be a 'component' (i.e. independent from Drupal), in that case.

We faced similar discussions in #2583041: GD toolkit & operations should catch \Throwable to fail gracefully in case of errors, comments #27 and #29.

With the patch here I am extracting a part of #2583041: GD toolkit & operations should catch \Throwable to fail gracefully in case of errors that can work in isolation and should be able to address the main point here, but does not get to the point of deprecating format_size and replacing it with a class. But that may be a next step.

Interdiff not relevant.

Can create the component without the format method. Then add a \Drupal\Core class with a format method, extending from the component. Or move the whole lot into \Drupal\Core is another option.

This patch follows on #33.

We have:

• Added a Bytes::toUnitAndSize() method to return separately a unit of measurement and the size expressed in the unit of measurement, given a byte quantity. Added unit test for that.
• Added a FormatBytesSize class in \Drupal\Core\Utility with a format method where we call Bytes::toUnitAndSize() and format the size string. Adjusted test for that, and added test cases for formatting '0 bytes' and a quantity of bytes that in format_size was resulting improperly rounded.
• Refactored format_size() to use FormatBytesSize::format().
• Added a deprecation annotation for format_size().

Yes, yes

Rerolled.

Plain reroll of #44.

Nits.

1. +++ b/core/includes/common.inc
   @@ -249,44 +249,13 @@ function check_url($uri) {
   + * @deprecated in Drupal 8.4.x, will be removed before Drupal 9.0.0.
   

   Needs a @see reference to the CR. See https://www.drupal.org/core/deprecation

2. +++ b/core/includes/common.inc
   @@ -249,44 +249,13 @@ function check_url($uri) {
    function format_size($size, $langcode = NULL) {
   

   Should yell a @trigger_error(). See https://www.drupal.org/core/deprecation

3. +++ b/core/includes/common.inc
   --- a/core/lib/Drupal/Component/Utility/Bytes.php
   +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   

   Do we need to update the component composer.json description?

4. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -39,4 +39,34 @@ public static function toInt($size) {
   +   *   A quantity of bytes.
   
   +++ b/core/lib/Drupal/Component/Utility/BytesUnitAndSize.php
   @@ -0,0 +1,63 @@
   + * A value object class to store a bytes quantity with an unit of measurement.
   ...
   +   * The bytes quantity.
   ...
   +   * The unit of measurement of this bytes quantity.
   ...
   +   *   The bytes quantity.
   ...
   +   * Gets the bytes quantity.
   ...
   +   * Gets the unit of measurement of this bytes quantity.
   ...
   +   *   The unit of measurement of this bytes quantity.
   

   Quantity? Sounds weird. Amount? Same. Maybe other word? Maybe "count"?

5. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -39,4 +39,34 @@ public static function toInt($size) {
   +   *   NULL if the input value is negative, a
   

   Feels like The NULL explanation should go to the end of docs.

6. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -39,4 +39,34 @@ public static function toInt($size) {
   +   * @see format_size
   

   Cannot refer a deprecated function.

7. +++ b/core/lib/Drupal/Core/Utility/FormatBytesSize.php
   @@ -0,0 +1,53 @@
   +   *   (Optional) Language code to translate to a language other than what is
   

   s/(Optional)/(optional)

And we should replace all occurrences of format_size().

Added draft CR https://www.drupal.org/node/2999981, working on #50

Thank for review @claudiu.cristea.

#50.1 done
#50.2 done
#50.3 I do not think we need to, 'other manipulations' seems good enough
#50.4,5,6,7 done

added a deprecation test for format_size, too.

Let's see how may deprecation error this gets, probably quite a few. Not sure we should do #52 here. Probably better adding a deprecation listener here and remove usages in a follow up.

With all usages replaced.

Fix for failure in #55 + CS cleanup.

Nice!

1. +++ b/core/includes/common.inc
   @@ -249,44 +248,14 @@ function check_url($uri) {
   + * @deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0.
   

   According Drupal core deprecation policy, the CR link should be added also as @see, after @deprecated.

2. +++ b/core/includes/common.inc
   @@ -249,44 +248,14 @@ function check_url($uri) {
   +  @trigger_error('format_size() is deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0. Use \Drupal\Core\Utility\FormatBytesSize::format($size, $langcode) instead. See https://www.drupal.org/node/2999981', E_USER_DEPRECATED);
   
   +++ b/core/tests/Drupal/KernelTests/Core/Common/SizeTest.php
   @@ -58,16 +62,27 @@ public function testCommonFormatSize() {
   +   * @expectedDeprecation format_size() is deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0. Use \Drupal\Core\Utility\FormatBytesSize::format($size, $langcode) instead. See https://www.drupal.org/node/2999981
   

   The message should end with a dot.

3. +++ b/core/lib/Drupal/Core/Utility/FormatBytesSize.php
   @@ -0,0 +1,55 @@
   +class FormatBytesSize {
   

   I'm not sure about the name FormatBytesSize. What is the generic name to describe something that is measuring in bytes, KB, MB, etc? Sometime is called "filesize" but that would be restrictive. Digital size? I see here https://en.wikipedia.org/wiki/Units_of_information the term "units of information". Or "storage size". For example if we use digital size, the the function call would be DigitalSize::format(), which for me would be more meaningful than FormatBytesSize::format(). StorageSize::format(). No idea :)

#59.1 done
#59.2 not sure... see #2848137-66: Replace all calls to db_update, which is deprecated
#59.3 how about \Drupal\Core\Utility\Bytes::toTranslatableString?

#59.2 not sure...

I think https://www.drupal.org/core/deprecation is the official reference. And the "official" pattern they are provided there for @trigger_error() ends with a dot.

#59.3: I was thinking on the FormatBytesSize name. When you see in a caller FormatBytesSize::format() it doesn't look great. But it's probably a matter of taste. I'm not insisting to change that.

#59.2 done
#59.3 et later: I also didn't like the naming. Doing here what @catch suggested in #34. Added a Core/Utility/Byte class extending from Component/Utility/Bytes, with a single toTranslatableString method. So FormatBytesSize::format => Bytes::toTranslatableString

Sorry wrong interdiff in #62, here's the good one.

I'm not sure about the direction this patch has taken.

+++ b/core/includes/file.inc
@@ -8,11 +8,11 @@
 use Drupal\Component\FileSystem\FileSystem as ComponentFileSystem;
 use Drupal\Component\Utility\UrlHelper;
 use Drupal\Component\PhpStorage\FileStorage;
-use Drupal\Component\Utility\Bytes;
 use Drupal\Core\File\FileSystem;
 use Drupal\Core\Site\Settings;
 use Drupal\Core\StreamWrapper\PublicStream;
 use Drupal\Core\StreamWrapper\PrivateStream;
+use Drupal\Core\Utility\Bytes;
 
 /**
  * Default mode for new directories. See drupal_chmod().

Having both Drupal\Component\Utility\Bytes and Drupal\Core\Utility\Bytes seems very weird. And unnecessarily importing the core version is odd too. How about doing something like TranslatableMarkup but as a TranslatableSize? And the new TranslatableSize class could contain all the logic from format_size()?

I think the patch here is unnecessarily complex and introduces new value objects like BytesUnitAndSize for little to no gain. We just have more API to support.

Or do something like date_format()...

function format_date($timestamp, $type = 'medium', $format = '', $timezone = NULL, $langcode = NULL) {
  return \Drupal::service('date.formatter')->format($timestamp, $type, $format, $timezone, $langcode);
}

And introduce a size.formatter service.

Thanks for review @alexpott. I will go for the service then.

+1 and then we get to properly inject the translation service too.

Converted to using a byte_count.formatter service and removed the BytesUnitAndSize value object.

No interdiff, it would make no sense given the extent of changes.

Looking at this makes me wonder if we should add this to \Drupal\Core\StringTranslation\TranslationInterface as a formatSize() method. Because it is always being used when you are outputting translated strings to the user. Or maybe we could introduce a new placeholder for FormattableMarkup that does translation for TranslatableMarkup.

Also this code

  $default_max_size = format_size(file_upload_max_size());
  $form['max_size'] = [
    '#type' => 'textfield',
    '#default_value' => $image_upload['max_size'],
    '#title' => t('Maximum file size'),
    '#description' => t('If this is left empty, then the file size will be limited by the PHP maximum upload size of @size.', ['@size' => $default_max_size]),
    '#maxlength' => 20,
    '#size' => 10,
    '#placeholder' => $default_max_size,
    '#states' => $show_if_image_uploads_enabled,
  ];

Is wrong. Because you need to enter a valid PHP file size so it shouldn't be translated. So this is an actual place where we should use new thing added to the utility. But that's not returning anything in a useful format.

1. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -39,4 +39,33 @@ public static function toInt($size) {
   +    if ($size < 0) {
   +      return NULL;
   +    }
   

   This is odd. And an API change - before if you passed a negative number this would not happen.

2. +++ b/core/tests/Drupal/KernelTests/Core/Common/SizeTest.php
   @@ -33,6 +34,8 @@ protected function setUp() {
   +      // Not 3.78 GB as before #2157945.
   +      '3.77 GB' => 4053371676,
   

   This is mixing a bugfix with a refactor which is never the right thing to do.

Let’s fix the bug first then, instead of refactoring it... :)

For now, just a reroll of #71 to see if it still passes. #72 next.

Dropping the Bytes::toUnitAndSize method in favour of Bytes::toString, stealing from @longwave patch at #3001402-2: editor_image_upload_settings_form() is wrongly translating the max file size.

+++ b/core/lib/Drupal/Component/Utility/Bytes.php
@@ -39,4 +39,33 @@ public static function toInt($size) {
+    $absolute_size = abs($size);
+    if ($absolute_size < Bytes::KILOBYTE) {
+      return $size . ' B'];
+    }
+    // Create a multiplier to preserve the sign of $size.

return $size . ' B'];
Needs more attention

Indeed.

Looks great just needs more refactor to inject service and use local variable to cache calls to `\Drupal::service()`

1. +++ b/core/lib/Drupal/Core/Form/EventSubscriber/FormAjaxSubscriber.php
   @@ -144,13 +144,13 @@ protected function getFormAjaxException(\Exception $e) {
      protected function formatSize($size) {
   -    return format_size($size);
   +    return \Drupal::service('byte_count.formatter')->format($size);
   

   This should use protected var for caching, and probably update constructor to inject new service but keep BC

2. +++ b/core/modules/file/file.module
   @@ -382,12 +382,12 @@ function file_validate_size(FileInterface $file, $file_limit = 0, $user_limit =
   +    $errors[] = t('The file is %filesize exceeding the maximum file size of %maxsize.', ['%filesize' => \Drupal::service('byte_count.formatter')->format($file->getSize()), '%maxsize' => \Drupal::service('byte_count.formatter')->format($file_limit)]);
   ...
   +    $errors[] = t('The file is %filesize which would exceed your disk quota of %quota.', ['%filesize' => \Drupal::service('byte_count.formatter')->format($file->getSize()), '%quota' => \Drupal::service('byte_count.formatter')->format($user_limit)]);
   

   Please use local var instead of "disturbing" \Drupal so many times

3. +++ b/core/modules/file/tests/src/Functional/FileFieldValidateTest.php
   @@ -92,13 +92,13 @@ public function testFileMaxSize() {
   +      $this->assertFileExists($node_file, format_string('File exists after uploading a file (%filesize) under the max limit (%maxsize).', ['%filesize' => \Drupal::service('byte_count.formatter')->format($small_file->getSize()), '%maxsize' => $max_filesize]));
   +      $this->assertFileEntryExists($node_file, format_string('File entry exists after uploading a file (%filesize) under the max limit (%maxsize).', ['%filesize' => \Drupal::service('byte_count.formatter')->format($small_file->getSize()), '%maxsize' => $max_filesize]));
   ...
   +      $error_message = t('The file is %filesize exceeding the maximum file size of %maxsize.', ['%filesize' => \Drupal::service('byte_count.formatter')->format($large_file->getSize()), '%maxsize' => \Drupal::service('byte_count.formatter')->format($file_limit)]);
   +      $this->assertRaw($error_message, format_string('Node save failed when file (%filesize) exceeded the max upload size (%maxsize).', ['%filesize' => \Drupal::service('byte_count.formatter')->format($large_file->getSize()), '%maxsize' => $max_filesize]));
   

   same here

Re. #72, removed the ByteCountFormatter class and service in favour of a new TranslationInterface::formatSize method.

Leaving service injection suggested in #84 for later.

Added a formatSize method onto StringTranslationTrait. This way we can get rid of wrapper formatSize methods where we have translatability! Also added formatSize implementation for a test TranslationManager class.

Adding 'needs issue summary update' since IS is now pretty old.

Fix migrate tests + minor clean-ups

NW for tests

1. +++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
   @@ -80,6 +80,15 @@ protected function formatPlural($count, $singular, $plural, array $args = [], ar
   +  protected function formatSize($size, $langcode = NULL) {
   +    return $this->getStringTranslation()->formatSize($size, $langcode);
   

   that needs unit test coverage

2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
   @@ -153,6 +154,44 @@ public function formatPlural($count, $singular, $plural, array $args = [], array
   +  public function formatSize($size, $langcode = NULL) {
   

   new method also needs tests

3. +++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
   @@ -460,4 +461,42 @@ public function formatPlural($count, $singular, $plural, array $args = [], array
   +  public function formatSize($size, $langcode = NULL) {
   +    $options = ['langcode' => $langcode];
   

   This duplicate of implementation still looks weird but I have no idea how to get rid of it, maybe mocking

Patch from #89 no longer applies. Re-rolled.

Adding unit test for TranslationManager::formatSize.

#92.1 - added test coverage for StringTranslationTrait::formatSize
#92.3 - removed the duplicate implementation, just return the $size argument should be enough for the purposes of this test

Rerolled.

Nits & a question.

1. +++ b/core/includes/common.inc
   @@ -249,48 +247,16 @@ function check_url($uri) {
   + * @deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0. Use
   + *   \Drupal::translation()->formatSize($size, $langcode)
   + *   instead.
   

   The "instead" word can go one line up.

2. +++ b/core/includes/common.inc
   @@ -249,48 +247,16 @@ function check_url($uri) {
   +  @trigger_error('format_size() is deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0. Use \\Drupal::translation()->formatSize($size, $langcode) instead. See https://www.drupal.org/node/2999981.', E_USER_DEPRECATED);
   

   Within single quotes there's no need to escape the backslash (\\Drupal::).

3. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -39,4 +39,33 @@ public static function toInt($size) {
   +  public static function toString($size, $precision = 2) {
   

   I wonder if it's worth considering an external solution such as https://github.com/gabrielelana/byte-units. I have the feeling that we're reinventing the wheel.

4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
   @@ -153,6 +154,44 @@ public function formatPlural($count, $singular, $plural, array $args = [], array
   +    list($rounded_size, $unit) = explode(' ', Bytes::toString($size), 2);
   

   Not so happy with extracting the value from a formatted string. Normally that value should be provided directly from a API/method/function.

Thanks @claudiu.cristea.

1. Done
2. Done
3. Very nice... but that would be a dependency addition, probably to be dealt with separately, and require postponing this one on that. I suppose we need a core committer's blessing for that. Adding tag for framework manager review.
4. Earlier patches were returning a value object here instead, but that were required to be dropped in #67.

The dependency addition should definitely separately and probably will prove very tricky because these are translatable strings. For instance google suggests that the Japanese translation from 1mb is 1メガバイト

Done the framework manager review for the dependency addition. Rule of thumb "don't add things in a refactor".

OK with that. Just a note: I was not referring to the translatable part but to the component. From my point of view this is ready.

+++ b/core/lib/Drupal/Component/Utility/Bytes.php
@@ -39,4 +39,33 @@ public static function toInt($size) {
+  /**
+   * Formats a given byte size in IEC binary units.
+   *
+   * @param int $size
+   *   A count of bytes.
+   * @param int $precision
+   *   (optional) the number of decimals to round to. Defaults to 2.
+   *
+   * @return string
+   *   The formatted string, followed by a space and the unit (e.g. "1023 B",
+   *   "2 KB", "5 MB", "10 GB").
+   */
+  public static function toString($size, $precision = 2) {
+    $absolute_size = abs($size);
+    if ($absolute_size < Bytes::KILOBYTE) {
+      return $size . ' B';
+    }
+    // Create a multiplier to preserve the sign of $size.
+    $sign = $absolute_size / $size;
+    foreach (['KB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB'] as $unit) {
+      $absolute_size /= Bytes::KILOBYTE;
+      $rounded_size = round($absolute_size, $precision);
+      if ($rounded_size < Bytes::KILOBYTE) {
+        break;
+      }
+    }
+    return ($rounded_size * $sign) . ' ' . $unit;
+  }

Actually I'm not sure about this addition. Providing this means that implementations might use this over the translated version - which would be wrong. Imo this shouldn't be public API.

Re #107. Yeah, agree. Making it a protected method somewhere, would solve also #102.4.

We need that, though? See #72 and the follow-up #3001402: editor_image_upload_settings_form() is wrongly translating the max file size

@mondrake that's a fine point. We would need very very clear docs on both methods about when and were they should be used. But on the other hand I thins maybe the simplest thing is to not do this. And in https://www.drupal.org/project/drupal/issues/3001402 provide a langcode of 'en' for the translation to not translate it. We should however add docs to TranslationInterface::formatSize() about when it is appropriate to add a langcode = 'en' option - ie. when you when to suggest a value for php.ini etc...

OK, so will remove Bytes::toString and keep the implementation in TranslationManager::formatSize - even a protected method there could be misused.

Here we go. In the end I sticked to a protected method on TranslationManager, so that we can test it independently.

Needed a re-roll.

Rereolled.

Converted additional usages introduced since #115 and updated the deprecation message.

Converted a stray getMock.

rtbc+1 except

+++ b/core/includes/common.inc
@@ -250,48 +248,15 @@ function check_url($uri) {
+ * @deprecated in drupal:8.8.0 and will be removed before drupal:9.0.0. Use
+ *   \Drupal::translation()->formatSize($size, $langcode) instead.
...
+  @trigger_error('format_size() is deprecated in drupal:8.8.0 and will be removed before drupal:9.0.0. Use \Drupal::translation()->formatSize($size, $langcode) instead. See https://www.drupal.org/node/2999981.', E_USER_DEPRECATED);

+++ b/core/tests/Drupal/KernelTests/Core/Common/SizeTest.php
@@ -57,4 +57,15 @@ public function providerTestCommonFormatSize() {
+   * @expectedDeprecation format_size() is deprecated in drupal:8.8.0 and will be removed before drupal:9.0.0. Use \Drupal::translation()->formatSize($size, $langcode) instead. See https://www.drupal.org/node/2999981.

the format a bit different from #3024461: Adopt consistent deprecation format for core and contrib deprecation messages and phpcs

Adjusted deprecation message according to #123

CR updates should be done after commit

Rerolled and changed another usage just introduced.

Just reroll

Updated the IS with the current solution.

Rerolled the patch.

Thanks for the reroll, @shashikant_chauhan

Deprecation message requires updates.
... in drupal:9.1.0 and is removed from drupal:10.0.0.

Updated CR https://www.drupal.org/node/2999981/revisions/view/11116408/11659397

And here re-roll with updated core ref

Reroll against 9.0.x

Just a reroll.... lots of automerging .. no conflicts

A little fixup.

The problem :-

In system.install system_requirements() -- the container is not yet available. That old chestnut.

\Drupal::translation() returns null .. and errors

While I think about ways to get correctness .. I am just backing out that change.
Other changes in 146 are correctly.

Here is the interdiff.

fewer failing tests.

Minor reroll following #3131258: Remove redundant $message from assertFile(Not)Exists and assertDirectory(Not)Exists calls

The function format_size() is being deprecated.
A @trigger_error() is added to the function.
A deprecation message test is added for the @trigger_error().
The functionality is being moved to the method Drupal/Core/StringTranslation/TranslationManager::formatSize() and the helper method Drupal/Core/StringTranslation/TranslationManager::bytesToSizeAndUnit().
For both methods is testing added.
The main method is added to the interface Drupal/Core/StringTranslation/TranslationInterface.
All function calls to format_size() have been replaced.
All changes look good to me.
Their is an CR added and it looks good to me.
For me it is RTBC.

Reroll, no conflicts just automerging.

Re-roll and added types to arguments of new method

Missed to fix translation manager

The change to int was wrong, it can accept floats and strings (as number)
moreover https://wiki.php.net/rfc/string_to_number_comparison is mostly accepted

Plain reroll of #156. This issue has been hopping on and off the RTBC queue for the last year. I wonder what's preventing commit.

1. So one consideration we have to make here is that every class that uses StringTranslationTrait is suddenly going to get a new method. At least for contrib it looks like this will break one contrib module see - http://codcontrib.hank.vps-private.net/search?text=function+formatSize&f... and http://codcontrib.hank.vps-private.net/node/30322124

   Given the tiny number of usages of this method in core I'd be tempted to not add this method to the trait in this issue and call $this->getStringTranslation()->formatSize() instead.

2. +++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
   @@ -80,6 +80,15 @@ protected function formatPlural($count, $singular, $plural, array $args = [], ar
   +  /**
   +   * Generates a string representation for the given byte count.
   +   *
   +   * @see \Drupal\Core\StringTranslation\TranslationInterface::formatSize()
   +   */
   +  protected function formatSize($size, $langcode = NULL) {
   

   We need to add:

   • @param docs
   • typehints for param and return
   • @return docs

3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
   @@ -109,4 +109,25 @@ public function translateString(TranslatableMarkup $translated_string);
   +   * @param int $size
   +   *   A size in bytes.
   

   '1 GB' is not an int.

4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
   @@ -109,4 +109,25 @@ public function translateString(TranslatableMarkup $translated_string);
   +   * @return \Drupal\Core\StringTranslation\TranslatableMarkup|\Drupal\Core\StringTranslation\PluralTranslatableMarkup
   +   *   A translated string representation of the size.
   +   */
   +  public function formatSize($size, $langcode = NULL);
   

   We to add typehints - probably just for langcode. $size can be a string, int or float. I think we can add a return typehint of TranslatableMarkup because PluralTranslatableMarkup inherits from that.

5. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
   @@ -153,6 +154,74 @@ public function formatPlural($count, $singular, $plural, array $args = [], array
   +  protected function bytesToSizeAndUnit($size, $precision = 2) {
   

   This was added in #36 without explanation. Is this expansion of API necessary here? I'm not convinced and can't find any explanation of why on the issue. Why isn't this inlined as before? If we add this anywhere it should be on the Bytes class - as this method has nothing to do with translation (in and of itself). But then people might be tempted to use it when really we want people to use the translated string. I think this should be inlined again unless there is a concrete reason.

   If we do add this we need scalar typehints and a return typehint.

6. +++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
   @@ -17,7 +17,7 @@ class BytesTest extends TestCase {
   -   * @param int $size
   +   * @param string $size
   

   Out-of-scope here. This change is not touching Bytes::toInt()

7. Lastly - given we're refactoring I think we should ask if formatSize() is a good name. There are many sizes - tshirt sizes and query counts to give examples that are both sizes but should never be passed to this method. Perhaps we should take this opportunity to improve upon the name. formatByteSize() for example is more descriptive.

#168.5 I vaguely remember discussions at the time on another issue to allow different precisions in the formatting, but yea, oos here. I think I went for a separate method to allow testing it separately - I guess it can be reverted.

#168.7 that was kinda suggested earlier around #58-ish, too.

Here's fixes for type-hints 161-162 with fix #168 (3, 4, 6)

Working on rename formatByteSize()

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
@@ -119,15 +119,15 @@ public function formatPlural($count, $singular, $plural, array $args = [], array
-   * @param int $size
+   * @param int|float|string $size

Whoops I got this wrong. I was thinking of bytes::toInt() which is kinda the opposite of this. This actually is an int I think. Sorry.

Here's a rename, let's see what bot will say, will explore more strict interface tomorrow

@alexpott As #162 tests show it's not always int, not sure about float but it could be numeric string

Yes, $size is passed as float as well in tests at least (otherwise it can't fit into big int)

EDIT Ref https://www.php.net/manual/en/language.types.integer.php#language.types....

For file item test the fix is type cast database value to int because db-layer returns ints as strings

--- a/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -1489,7 +1489,7 @@ function template_preprocess_file_link(&$variables) {
   // Set file classes to the options array.
   $variables['attributes'] = new Attribute($variables['attributes']);
   $variables['attributes']->addClass($classes);
-  $variables['file_size'] = \Drupal::translation()->formatByteSize($file->getSize());
+  $variables['file_size'] = \Drupal::translation()->formatByteSize((int) $file->getSize());
 
   $variables['link'] = Link::fromTextAndUrl($link_text, Url::fromUri($url, $options))->toRenderable();
 }

Fix broken test

@andypost ah yeah - i see. If we had union types I think we'd have int|float here - because it does need to be a number type. See the docs for abs().

Thanks @andypost

1. +++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
   @@ -80,6 +80,24 @@ protected function formatPlural($count, $singular, $plural, array $args = [], ar
   +   * @param string|null $langcode
   +   *   (optional) Language code to translate to a language other than what is
   +   *   used to display the page.
   +   *
   +   * @return \Drupal\Core\StringTranslation\TranslatableMarkup
   +   *   A translated string representation of the size.
   +   *
   +   * @see \Drupal\Core\StringTranslation\TranslationInterface::formatByteSize()
   +   */
   +  protected function formatByteSize($size, string $langcode = NULL): TranslatableMarkup {
   +    return $this->getStringTranslation()->formatByteSize($size, $langcode);
   +  }
   

   Do we then agree to keep the method on the trait, re. #168.1? In any case, AFAIK, if we typehint string $langcode = NULL, we cannot pass a NULL to $langcode - it's only going to be initialized to NULL if the argument is missing. Different would be if we typehinted ?string $langcode = NULL, in that case it is possible to pass a null argument. So the doc is incorrect, it should be @param string $langcode.

2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
   @@ -109,4 +109,25 @@ public function translateString(TranslatableMarkup $translated_string);
   +  /**
   +   * Generates a translated string representation for the given byte count.
   +   *
   +   * In some use cases, a size in IEC units (like for example 1 MB, 256 KB,
   +   * etc.) must not be translated to local language. This is the case for
   +   * writing a value in configuration files or objects (php.ini or others),
   +   * and when it is required to enter a value in a form (e.g. to indicate a
   +   * maximum file size). In such cases, this method should be called with an
   +   * 'en' $langcode parameter to keep the value language agnostic.
   +   *
   +   * @param int|float|string $size
   +   *   A size in bytes.
   +   * @param string|null $langcode
   +   *   (optional) Language code to translate to a language other than what is
   +   *   used to display the page.
   +   *
   +   * @return \Drupal\Core\StringTranslation\TranslatableMarkup
   +   *   A translated string representation of the size.
   +   */
   +  public function formatByteSize($size, string $langcode = NULL): TranslatableMarkup;
   +
   

   same as above (for the $langcode typehint)

3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
   @@ -153,6 +154,74 @@ public function formatPlural($count, $singular, $plural, array $args = [], array
   +  /**
   +   * Given a byte size, returns it in IEC binary units.
   +   *
   +   * @param int|float|string $size
   +   *   A count of bytes.
   +   * @param int $precision
   +   *   (optional) the number of decimals to round to. Defaults to 2.
   +   *
   +   * @return array
   +   *   A simple array, containing the size rounded to the highest IEC unit,
   +   *   and the IEC unit itself (e.g. [1023, 'B'], [2, 'KB'], [5, 'MB'], [10,
   +   *   'GB']).
   +   */
   +  protected function bytesToSizeAndUnit($size, int $precision = 2): array {
   

   should be removed and its code inlined per #168.5

4. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
   @@ -98,6 +144,67 @@ public function providerTestTranslatePlaceholder() {
   +  /**
   +   * Tests TranslationManager::testBytesToSizeAndUnit().
   +   *
   +   * @param array $expected
   +   *   The expected return value from
   +   *   TranslationManager::testBytesToSizeAndUnit().
   +   * @param int $size
   +   *   The value for the size argument for
   +   *   TranslationManager::testBytesToSizeAndUnit().
   +   * @param int $precision
   +   *   (Optional) the value for the precision argument for
   +   *   TranslationManager::testBytesToSizeAndUnit. Defaults to 2.
   +   *
   +   * @dataProvider providerTestBytesToSizeAndUnit
   +   */
   +  public function testBytesToSizeAndUnit(array $expected, $size, $precision = 2) {
   +    $translation_manager = $this->createMock(TranslationManager::class);
   +    $method = new \ReflectionMethod(get_class($translation_manager), 'bytesToSizeAndUnit');
   +    $method->setAccessible(TRUE);
   +    $this->assertSame($expected, $method->invoke($translation_manager, $size, $precision));
   +  }
   +
   +  /**
   +   * Provides data for testBytesToSizeAndUnit.
   +   *
   +   * @return array
   +   *   An array of arrays, each containing the argument for
   +   *   TranslationManager::testBytesToSizeAndUnit: expected return array,
   +   *   input size, input precision.
   +   */
   +  public function providerTestBytesToSizeAndUnit() {
   +    return [
   +      [['0', 'B'], 0],
   +      [['1', 'B'], 1],
   +      [['-1', 'B'], -1],
   +      [['1023', 'B'], Bytes::KILOBYTE - 1],
   +      [['1', 'KB'], Bytes::KILOBYTE],
   +      [['1', 'MB'], pow(Bytes::KILOBYTE, 2)],
   +      [['1', 'MB'], pow(Bytes::KILOBYTE, 2) - 1],
   +      [['-1', 'MB'], -(pow(Bytes::KILOBYTE, 2) - 1)],
   +      [['1', 'GB'], pow(Bytes::KILOBYTE, 3)],
   +      [['1', 'TB'], pow(Bytes::KILOBYTE, 4)],
   +      [['1', 'PB'], pow(Bytes::KILOBYTE, 5)],
   +      [['1', 'EB'], pow(Bytes::KILOBYTE, 6)],
   +      [['1', 'ZB'], pow(Bytes::KILOBYTE, 7)],
   +      [['1', 'YB'], pow(Bytes::KILOBYTE, 8)],
   +      [['22.39', 'MB'], 23476892],
   +      [['76', 'MB'], 79691776],
   +      [['1024', 'YB'], pow(Bytes::KILOBYTE, 9)],
   +      [['3.46', 'MB'], 3623651],
   +      [['3.8', 'GB'], 4053837095, 1],
   +      [['3.78', 'GB'], 4053837095, 2],
   +      [['3.775', 'GB'], 4053837095, 3],
   +      [['3.7754', 'GB'], 4053837095, 4],
   +      [['3.77543', 'GB'], 4053837095, 5],
   +      [['59.72', 'PB'], 67234178751368124],
   +      [['194.67', 'YB'], 235346823821125814962843827],
   +      [['76.24', 'GB'], 81862076662],
   +    ];
   +  }
   +
    }
   

   can be dropped when inlining the method's code.

@mondrake see https://3v4l.org/VYaKq - string $langcode = NULL and ?string $langcode are exactly the same (as far as I know).

#178 thanks. I thought I had this discussion already but it turns out it was in Doctrine/DBAL, not Drupal: https://github.com/doctrine/dbal/pull/2766

Does not really matter here, but I tend to agree if we want the argument to be nullable AND default to NULL, then we should add the ? too.

Fix for nullable string, it's already used in core https://git.drupalcode.org/project/drupal/-/blob/9.1.x/core/modules/sear...

Re #176 https://3v4l.org/mdHOv - it consumes any number (stringable numbers as well)

EDIT I was wrong about mixed types https://3v4l.org/Dp4VH

Inlined method, also found strange that "factional bytes"

+      [0.1, NULL, '0.1 bytes'],
+      ['0.6', NULL, '0.6 bytes'],

1. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
   @@ -66,6 +67,53 @@ public function testFormatPlural($count, $singular, $plural, array $args, array
   +      ['0', NULL, '0 bytes'],
   

   We should pass 0 not '0', also later for '0.6'. Let's respect the types.

2. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
   @@ -66,6 +67,53 @@ public function testFormatPlural($count, $singular, $plural, array $args, array
   +      [0.1, NULL, '0.1 bytes'],
   +      ['0.6', NULL, '0.6 bytes'],
   

   I think this is just a bug now, we should not have fractions of bytes... should be '0 bytes' in both cases I would say.

3. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
   @@ -66,6 +67,53 @@ public function testFormatPlural($count, $singular, $plural, array $args, array
   +    $this->assertSame(is_null($langcode) ? '' : $langcode, $result->getOption('langcode'));
   

   can we use $langcode ?? '' instead of is_null($langcode) ? '' : $langcode?

4. +++ b/core/tests/Drupal/KernelTests/Core/Common/SizeTest.php
   @@ -57,4 +57,15 @@ public function providerTestCommonFormatSize() {
   +  public function testFormatSizeDeprecation() {
   

   : void return typehint

5. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/StringTranslationTraitTest.php
   @@ -68,4 +72,20 @@ public function testFormatPlural() {
   +  public function testFormatByteSize() {
   

   same

6. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
   @@ -66,6 +67,53 @@ public function testFormatPlural($count, $singular, $plural, array $args, array
   +  public function providerTestFormatByteSize() {
   

   : array return typehint

7. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
   @@ -66,6 +67,53 @@ public function testFormatPlural($count, $singular, $plural, array $args, array
   +  public function testFormatByteSize($size, $langcode, $expected) {
   

   public function testFormatByteSize(int $size, string $langcode, string $expected): void {

Re #182
1) see #180 about strings and numbers - it's just extra test coverage
2) that's BC and not sure how to deal with it
Remains will address, thanks!

Fix for #182 (except 2 point) - if we round/ceil/floor it to int then new method breaks BC

Also cleaned a bit doc blocks

Add more refs to deprecated function and fix title/summary with new name

IMHO

1) we should file a follow-up to fix the bug in #182.2 - yeah let's not fix a bug in a refactor, but a file size of "345.56 bytes" does not make sense
2) we might consider a conditional deprecation to trigger an error if $size is a string, to be replaced in D10 by an \InvalidArgumentException. That way when time comes we may typehint $size to int|float in PHP8+
3)

1. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
   @@ -66,6 +67,57 @@ public function testFormatPlural($count, $singular, $plural, array $args, array
   +   * @param int $size
   

   int|float|string

2. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
   @@ -66,6 +67,57 @@ public function testFormatPlural($count, $singular, $plural, array $args, array
   +  public function testFormatByteSize($size, $langcode, $expected): void {
   

   ?string $langcode, string $expected

Filed follow-up and added TODO #3161118: Make \Drupal\Core\StringTranslation\TranslationManager::formatByteSize() to return integer amount of bytes
Fixed #188 but #188.1 is wrong about string, it expected to be int|float numeric strings are valid float and integeres are part of PHP, see union types at https://3v4l.org/Dp4VH

Thanks @andypost - re. #188.3.1 the point is that in the dataprovider one of the input records has ‘0.6’ as $size which is a string. But OK, rather minor one this one.

One more related to conversion, which exposes one more rounding issue

Reroll.

Let's hope this gets in sometime (#165)... Rerolled.

Re-rolled the patch in #197. Thanks!

Re-rolled the patch in #203...Thank you!

Fixed following review, last reroll made first 2 points wrong

1. +++ b/core/modules/file/file.module
   @@ -918,7 +925,10 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   -      \Drupal::messenger()->addError(t('The file %file could not be saved because it exceeds %maxsize, the maximum allowed size for uploads.', ['%file' => $original_file_name, '%maxsize' => format_size(Environment::getUploadMaxSize())]));
   ...
   +          '%file' => $file_info->getFilename(),
   

   This change is wrong and broke test

2. +++ b/core/modules/file/file.module
   @@ -1192,7 +1202,7 @@ function file_tokens($type, $tokens, array $data, array $options, BubbleableMeta
   -          $replacements[$original] = format_size($file->getSize());
   +          $replacements[$original] = \Drupal::translation()->formatByteSize($file->getSize());
   

   should be moved out of loop

3. +++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php
   @@ -2,12 +2,14 @@
    class FileWidgetAjaxController {
   +  use StringTranslationTrait;
   

   that's tricky about BC but should be fine

Re-roll for https://www.drupal.org/node/3176667

1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
   @@ -109,4 +109,25 @@ public function translateString(TranslatableMarkup $translated_string);
    
   +  /**
   +   * Generates a translated string representation for the given byte count.
   +   *
   +   * In some use cases, a size in IEC units (like for example 1 MB, 256 KB,
   +   * etc.) must not be translated to local language. This is the case for
   +   * writing a value in configuration files or objects (php.ini or others),
   +   * and when it is required to enter a value in a form (e.g. to indicate a
   +   * maximum file size). In such cases, this method should be called with an
   +   * 'en' $langcode parameter to keep the value language agnostic.
   +   *
   +   * @param int|float $size
   +   *   A size in bytes.
   +   * @param string|null $langcode
   +   *   (optional) Language code to translate to a language other than what is
   +   *   used to display the page.
   +   *
   +   * @return \Drupal\Core\StringTranslation\TranslatableMarkup
   +   *   A translated string representation of the size.
   +   */
   +  public function formatByteSize($size, ?string $langcode = NULL): TranslatableMarkup;
   +
   

   Discussed this with @alexpott and we think there's a way to avoid introducing the additional API surface here.

   format_size() is only used in string translations. So instead of manually calling a method on the arguments to t(), could we look at adding a new placeholder in translated strings - we'd then pass the raw bytes as argument and let the translation system do the conversion via the placeholder. Would mean all the logic staying in a protected method instead of a public one.

   If we were to do that later, we'd need to then deprecate the new methods added here, so it seems worth trying to address here if we can.

2. +++ b/core/modules/file/file.module
   @@ -350,15 +350,22 @@ function file_validate_extensions(FileInterface $file, $extensions) {
    
      if ($file_limit && $file->getSize() > $file_limit) {
   -    $errors[] = t('The file is %filesize exceeding the maximum file size of %maxsize.', ['%filesize' => format_size($file->getSize()), '%maxsize' => format_size($file_limit)]);
   +    $errors[] = t('The file is ^filesize exceeding the maximum file size of ^maxsize.', [
   +      '^filesize' => $file->getSize(),
   +      '^maxsize' => $file_limit,
   +    ]);
      }
    
   

   The actual change for developers then looks something like this. One disadvantage is this changes the translated strings, but since we'd only change this in a minor release anyway that's not a problem.

   Note that ^placeholder isn't really a suggestion, it's just to have something to look at.