format_size() fixes

I think a negative size is OK. What if you had a diff system that as well as showing the changes in lines it showed the change in filesize? That could be a negative value.

We could do something like

return ($size < 0 ? '-' : '') . \Drupal::translation()->formatPlural(abs($size), '1 byte', '@count bytes', [], ['langcode' => $langcode]);

but not sure if this is an improvement, maybe just wait for #1131926: format_plural returns a plural value for a negative singular

1. +++ b/core/tests/Drupal/KernelTests/Core/Common/SizeTest.php
   @@ -48,11 +53,8 @@ protected function setUp() {
   +        $this->assertSame($expected, $result, $expected . ' == ' . $result . ' (' . $input . ' bytes)');
   

   What is the reason for putting $expected == $result in here? PHPUnit provides this information out of the box.

2. +++ b/core/tests/Drupal/KernelTests/Core/Common/SizeTest.php
   @@ -62,11 +64,8 @@ public function testCommonFormatSize() {
   -      $this->assertEqual(
   -        $size,
   -        ($parsed_size = Bytes::toInt($string = format_size($size, NULL))),
   -        $size . ' == ' . $parsed_size . ' (' . $string . ')'
   -      );
   +      $parsed_size = Bytes::toInt($string = format_size($size, NULL));
   +      $this->assertEquals($size, $parsed_size, $size . ' == ' . $parsed_size . ' (' . $string . ')');
   

   To be honest I think we should improve the readability of this test and assign $string outside.

Fair :)

I'm not entirely convinced by #5. I'd rather deprecate support for negative amounts and leave it up to the caller on how to handle negative bytes. So we don't change behaviour in Drupal 8 but in Drupal 9 we through an exception of something. I think it would also be useful to have a look at other PHP libraries and see how they handle the negative case.

+ if ($size < 0) {
+ @trigger_error('Using format_size with negative $size argument is deprecated in Drupal 8.7.0 and will throw an exception in Drupal 9.0.0. Instead, handle the case of negative quantity beforehand and always pass a positive value. See https://www.drupal.org/node/3004611', E_USER_DEPRECATED);
+ }

If I had a quota of 2Gb of space and used 2.1Gb would I not have -0.1Gb of space? I think the assertion that you can't have negative sizes is incorrect.

In fact, I think that should definitely be removed. There may a features that can't allow negative sizes but we absolutely have to be able to format a string with a negative size. The alternative is that a developer basically has to write:

$negative = $value < 0 ? '-' : '';
$string = $negative . format_size(abs($value));

And that just doesn't make sense.

Agree with @neclimdul that there will be use cases where negative values make sense. I don't see how it can harm anyone by supporting negative values here, and if we start throwing exceptions (in D9) then there will be edge cases where somehow someone has ended up with e.g. a negative quota, and we would error when displaying when we don't strictly need to.

The only contentious point I feel is "-1 byte" vs "-1 bytes" but that already has its own issue.

longwave described my intended point better than I did. The expected output wasn't as important as the fact that it wouldn't error unexpectedly. Correct output would be nice though ;)

It's not broken as in producing an error - it just always outputs bytes...

>>> format_size(1000);
=> Drupal\Core\StringTranslation\PluralTranslatableMarkup {#1595
     markup: "1000 bytes",
   }
>>> format_size(-1000);
=> Drupal\Core\StringTranslation\PluralTranslatableMarkup {#1600
     markup: "-1000 bytes",
   }
>>> format_size(-10000000);
=> Drupal\Core\StringTranslation\PluralTranslatableMarkup {#1596
     markup: "-10000000 bytes",
   }
>>> format_size(10000000);
=> Drupal\Core\StringTranslation\TranslatableMarkup {#1583
     markup: "9.54 MB",
   }

I'm okay with fix the thing to do -9.54 MB but we should leave the -1 byte/s thing well alone. And fix/discuss that in the other issue.

Whilst working on this I found a bug in really really large sizes > 1024 YB fixed that as well.

I've not included any change to \Drupal\Component\Utility\Bytes::toInt since I think that should have it's own issue. The change in #15 for example will result in breakages of 15 P-bytes whereas before that'd work!?!?!

Small refactoring: I did not change the logic of code. I only removed else in the following code and IMO unnecessary switch statement. The code is a slightly cleaner now.

if ($size < Bytes::KILOBYTE) {
  return \Drupal::translation()->formatPlural($size, '1 byte', '@count bytes', [], ['langcode' => $langcode]);
}
else {

case 'KB':
  return new TranslatableMarkup('@size KB', $args, $options);
case 'MB':
  return new TranslatableMarkup('@size MB', $args, $options);

In addition, there were no s at the end of kilobyte

// Rounded to 1 MB - not 1000 or 1024 kilobyte

+++ b/core/includes/common.inc
@@ -251,42 +251,25 @@ function check_url($uri) {
-    switch ($unit) {
-      case 'KB':
-        return new TranslatableMarkup('@size KB', $args, $options);
-      case 'MB':
-        return new TranslatableMarkup('@size MB', $args, $options);
-      case 'GB':
-        return new TranslatableMarkup('@size GB', $args, $options);
-      case 'TB':
-        return new TranslatableMarkup('@size TB', $args, $options);
-      case 'PB':
-        return new TranslatableMarkup('@size PB', $args, $options);
-      case 'EB':
-        return new TranslatableMarkup('@size EB', $args, $options);
-      case 'ZB':
-        return new TranslatableMarkup('@size ZB', $args, $options);
-      case 'YB':
-        return new TranslatableMarkup('@size YB', $args, $options);
...
+  $args = [
+    '@size' => $rounded_size * $sign,
+    '@unit' => $unit
+  ];
+  $options = ['langcode' => $langcode];
+  return new TranslatableMarkup('@size @unit', $args, $options);

Changing this removes the ability to translate the units KB/MB/GB etc... which must be why that is done like this.

Removing the else makes sense.

Ignore patch #33. Removing else and correction of the comment.

-    $absolute_size = $absolute_size / Bytes::KILOBYTE;
+    $absolute_size /= Bytes::KILOBYTE;

Rerolled.
Addressed #40
Fixed CS errors: Reformatted the switch/case construction.

Should the case 'YB': value be replaced with default: to avoid "Missing return statement" warning in IDE?

Committed and pushed d29f046424 to 8.7.x and 1d78e9d5b6 to 8.6.x. Thanks!