Replace \Drupal\Component\Utility\Bytes::toInt() with \Drupal\Component\Utility\Bytes::toNumber() due to return type

Hi @Chi,
Hope you will fine!!
I am not getting what needs to be done here? can you pls elaborate, if you dont mind?

In

  /**
   * Parses a given byte size.
   *
   * @param mixed $size
   *   An integer or string size expressed as a number of bytes with optional SI
   *   or IEC binary unit prefix (e.g. 2, 3K, 5MB, 10G, 6GiB, 8 bytes, 9mbytes).
   *
   * @return int
   *   An integer representation of the size in bytes.
   */
  public static function toInt($size) {
    // Remove the non-unit characters from the size.
    $unit = preg_replace('/[^bkmgtpezy]/i', '', $size);
    // Remove the non-numeric characters from the size.
    $size = preg_replace('/[^0-9\.]/', '', $size);
    if ($unit) {
      // Find the position of the unit in the ordered string which is the power
      // of magnitude to multiply a kilobyte by.
      return round($size * pow(self::KILOBYTE, stripos('bkmgtpezy', $unit[0])));
    }
    else {
      return round($size);
    }
  }

add a casting to int before returning a value, i.e.

    if ($unit) {
      // Find the position of the unit in the ordered string which is the power
      // of magnitude to multiply a kilobyte by.
      return (int) round($size * pow(self::KILOBYTE, stripos('bkmgtpezy', $unit[0])));
    }
    else {
      return (int) round($size);
    }

that's the right fix IMO, we can't do anything else in a method named toInt...

Patch attached to update the documentation.

I definitely struggle on #4 :)

Also, we need an automated test (or an integration to an existing one?) to ensure we do not incur in regressions.

#8 that won't be enough as it would just fail badly if the returned value is not int, so the casting is necessary anyway (round() returns a float always). Besides that, adding a typehint is also a bigger problem because any class extending from that would fail miserably at syntax checking - a BC issue.

But... I am afraid here we're hitting another problem, i.e. int are not sufficient for representing zettabytes and yottabytes...

#12 try ‘1 YB’

#14 agree. Then we have to make the casting conditional on the result being below PHP_INT_MAX. Below return int, above leave a float. And needs to be documented and tested.

on this

Done #15.

In this case, casting data types should be safe. It replaces the rounded float (e.g. 10.0 -> 10).

+++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
@@ -28,7 +28,8 @@ class BytesTest extends TestCase {
+    $expected = $expected_int > PHP_INT_MAX ? $expected_int : (int) $expected_int;

This is just copying the code under test, which doesn't make it a very good test. Can we add the type to the data provider somehow? Or at least call assertIsFloat/assertIsInt and go back to assertEquals instead of casting here?

OK, this issue title made me laugh.

I think @longwave raises a good point; NW to look into that.

Maybe it is better to add another method toLong() or toFloat() for the caller to use in the cases like 1 YB.

+1 for toInt() to thrown an exception if it exceeds PHP_INT_MAX.

On this.

Throwing an exception here would be a behavior change and hence a BC break... I understand not everybody is uploading zettabyte sized files everyday, but still... :) (that's also a reason IMHO this is a minor issue)

So if we want to do that, we would have to deprecate first, etc. etc.

Let's see if this is better, from the test side - force the number literals in the data provider.

Patch looks visually fine to me but returning float by a method named toInt() doesn't seem contextual. Maybe the method needs renaming?

@Sivaji per Drupal policy we can't just rename a method, unless it's a private one, or marked @internal (and even in those cases I see debate). We would need to add a new toFloat() method instead, deprecate toInt(), and file a follow-up to remove in D10. But that would be a separate issue IMHO. Again, I think 99.999999% of the cases we never really get to convert ZB or YB in real life.

So in #25 we learn that large literal numbers cannot be parsed if above PHP_INT_MAX... interesting. Working out something different.

Following @longwave suggestion in #20.

@mondrake, agreed.

Can I create a new issue to add a new method & deprecate the toInt() in D10?

I suggest naming it to toByte() to allow returning int or float as needed.

@Sivaji sure thing! Everybody can, this is the sense of open source :)

I meant something more like

  public function testToInt($size, $expected_int) {
    $value = Bytes::toInt($size);
    $this->assertEquals($expected_int, $value);
    if ($expected_int > PHP_INT_MAX) {
      $this->assertIsFloat($value);
    }
    else {
      $this->assertIsInt($value);
    }      
  }

Can we assume we are always testing on 64 bit systems (in which case we know for sure which test cases will be float and which will be int), or is that too much of an assumption to make?

#32 AFAIK assertSame also checks the type, so isn't that superflous?
Also about 64-bit I think that's too strong an assumption - fair enough on DrupalCI will be like that, but locally not.

Maybe we can use PHP_INT_SIZE in the test data provider and make the list of expected return values dependent on that, instead: https://stackoverflow.com/questions/670662/whats-the-maximum-value-for-a...

OTOH I'm afraid on 32-bit the parsing will fail for 64-bit only number literals, same way as in #25 the number 1180591620717411303424, which is 1 ZB, fails even on 64-bit.

Actually, can we simply use assertSame without casting at all in the test?

php > print PHP_INT_MAX;
9223372036854775807
php > print gettype(pow(1024, 6));
integer
php > print gettype(pow(1024, 7));
double

Nice and clean, @longwave!

RTBC +1, but I cannot.

1. It looks good to me too. Attaching fix patch (#35) as the latest attachment to be tested by the bot (retested daily).

2. In this case, casting data types should be safe. It always replaces the rounded float (e.g. 10.0 -> 10).

Random test failure.

1. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -32,11 +33,12 @@ public static function toInt($size) {
   -      return round($size * pow(self::KILOBYTE, stripos('bkmgtpezy', $unit[0])));
   +      $result = round($size * pow(self::KILOBYTE, stripos('bkmgtpezy', $unit[0])));
   

   I haven't read this code in a really long time but am struck again that bkmgtpezy sounds like a made-up creature in childrens' literature à la Lewis Carroll. "All mimsy were the bkmgtpezy, and the mome raths outgrabe."

2. --- a/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
   +++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
   @@ -17,10 +17,10 @@ class BytesTest extends TestCase {
   -   * @param int $expected_int
   +   * @param int|float $expected_int
   
   @@ -28,7 +28,7 @@ class BytesTest extends TestCase {
   -    $this->assertEquals($expected_int, Bytes::toInt($size));
   +    $this->assertSame($expected_int, Bytes::toInt($size));
   

   Shouldn't we be testing both types of return now -- int and float? The data provider is all int-y return things. We're solving part of the test coverage problem with the assertSame() but not all of it I think. It could be just one more case with a SRSLY LARGE NUMBER in the data provider.

Looks good other than that; I think we came 'round to a good place. Thanks!

As demonstrated in #34, zettabytes and yottabytes don't fit in a 64 bit int and the pow() function automatically returns a float (double) for these. assertSame checks the type which ensures our method also returns int for the smaller sizes and float for the larger ones. Maybe we need a comment to this effect though?

Added some explanatory comments.

I think this is the maximum we can do with a method called toInt that has to handle non-ints. A follow up could be to deprecate this method in favour of a, say, toNumber method always returning a float.

That works for me.

Turns out, we need to add the words zettabyte and zettabytes to core's dictionary for the spellchecking we just added. (Oddly enough, "yottabytes" is already there.)

+++ b/core/misc/cspell/dictionary.txt
@@ -2249,6 +2249,8 @@ yygroup
+zettabyte
+zettabytes

After running the spellcheck commands on my local, found out we only need to add zettabytes. 1) As yottabytes is not a unknown word, I think that it's better to have zettabytes added to a upstream dictionary. 2) Meanwhile, we are in progress of removing typos/words from the dictionary file, so I tend to ignore it by using cspell:ignore, not adding it to the dictionary.

Attached patch ignored zettabytes. I did run an extra spellcheck on my local, it works. So setting this to RTBC at the same time, as the patch is less than 1KB.

$ yarn run spellcheck:core
yarn run v1.22.4
$ cspell "**/*" "../composer/**/*" "../composer.json"
CSpell: Files checked: 14653, Issues found: 0 in 0 files
✨  Done in 234.63s.

Needs a reroll.

Rerolled and replaced pow() with the ** operator

+++ b/core/lib/Drupal/Component/Utility/Bytes.php
@@ -32,11 +33,12 @@ public static function toInt($size) {
+      $result = round($size * pow(self::KILOBYTE, stripos('bkmgtpezy', $unit[0])));

In PHP 5.6 onwards, you may prefer to use the ** operator.

See https://www.php.net/manual/en/function.pow.php

+++ b/core/lib/Drupal/Component/Utility/Bytes.php
@@ -32,12 +33,13 @@ public static function toInt($size) {
+      $result = round($size * (self::KILOBYTE ** stripos('bkmgtpezy', $unit[0])));
...
+      $result = round($size);

I think we'll have to cast $size to float here, since that was done in #3156879: \Drupal\Component\Utility\Bytes::toInt() - ensure $size is a number type and is probably the reason this needed a reroll.

> replaced pow() with the ** operator

Why? We don't use ** anywhere else in core that I can see, we do use pow(), and (personal opinion) pow() is much easier to read and remember what it does.

1. #52, thanks @mondrake, I did not notice that.
2. #53, just preference, reverted.

Looks good, back to RTBC.

I don't think this is a bug per se - it's certainly unexpected given the name but it has been this way for years. Given we are subtly changing the return type I think we should have a change record - in the very unlikely case that someone is depending on this returning a float.

I don't think this is a bug per se - it's certainly unexpected given the name but it has been this way for years. (...) in the very unlikely case that someone is depending on this returning a float.

The reason for this problem is the misleading name, not the return type so #4 is safer (backwards compatibility).

-   * @return int
-   *   An integer representation of the size in bytes.
+   * @return float
+   *   The floating point value of the size in bytes.
    */
   public static function toInt($size) {

Let's change documentation and deprecate the Bytes::toInt() method.
#3143478: Rename method Bytes::toInt() to Bytes::toNumber()

If so, I would suggest re-scoping here to include #3143478: Rename method Bytes::toInt() to Bytes::toNumber().

  /**
   * Parses a given byte size.
   *
   * @param mixed $size
   *   An integer or string size expressed as a number of bytes with optional SI
   *   or IEC binary unit prefix (e.g. 2, 3K, 5MB, 10G, 6GiB, 8 bytes, 9mbytes).
   *
   * @return int|float
   *   An integer representation of the size in bytes. If the size is larger
   *   than PHP_INT_MAX, then the returned value is a float.
   */
  public static function toNumber($size) {
    $result = self::toInt($size);
    return $result > PHP_INT_MAX ? $result : (int) $result;
  }

Add toNumber(), keep toInt() untouched and deprecate it.

Set back to NR for suggestions of what to do next.

+1 on #59. We should close the other one as duplicate then.

   * @return int|float
   *   An integer representation of the size in bytes. If the size is larger
   *   than PHP_INT_MAX, then the returned value is a float.
   */
  public static function toNumber($size) {

Re #59. IMO toNumber is a suitable name. Dependency on PHP_INT_MAX is not good. Better if it always returns float.

 * @return float
 *   The floating point value of the size in bytes.
 */
public static function toNumber($size) {

Thanks @mondrake and @Krzysztof Domański!

1. Did as #59, but toNumber() returns a float as suggested in #61.
2. CR added. https://www.drupal.org/node/3162663

+++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
@@ -62,4 +67,57 @@ public function providerTestToInt() {
+   * @param int|float $expected_int
...
+  public function testToNumber($size, $expected_int) {
+    $this->assertSame($expected_int, Bytes::toNumber($size));

Self review:

int|float -> float, expected_int -> expected_number

1. BTW
   

   +++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
   @@ -62,4 +67,57 @@ public function providerTestToInt() {
   +   * @group legacy
   ...
   +  public function testToNumber($size, $expected_int) {
   

   Only tests with the `@group legacy` annotation can expect a deprecation.

   So that @group legacy added to testToNumber() as well.

2. Addressing #63

We will remove the Bytes::toInt() method in D10 so it should be like this:

public static function toInt($size) {
  @trigger_error('\Drupal\Component\Utility\Bytes::toInt() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\Component\Utility\Bytes::toNumber() instead. See https://www.drupal.org/node/3162663', E_USER_DEPRECATED);
  return self::toNumber($size);
}

public static function toNumber($size) {
  // Remove the non-unit characters from the size.
  $unit = preg_replace('/[^bkmgtpezy]/i', '', $size);
  // Remove the non-numeric characters from the size.
  $size = preg_replace('/[^0-9\.]/', '', $size);
  if ($unit) {
    // Find the position of the unit in the ordered string which is the power
    // of magnitude to multiply a kilobyte by.
    return round($size * pow(self::KILOBYTE, stripos('bkmgtpezy', $unit[0])));
  }
  else {
    // Ensure size is a proper number type.
    return round((float) $size);
  }
}

Thanks @Krzysztof Domański for the review.

Adjusted following #66

1. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -25,6 +25,21 @@ class Bytes {
   +   * @param mixed $size
   +   *   An integer or string size expressed as a number of bytes with optional SI
   +   *   or IEC binary unit prefix (e.g. 2, 3K, 5MB, 10G, 6GiB, 8 bytes, 9mbytes).
   ...
   +  public static function toNumber($size) {
   

   If it accepts int|string then why it can't accept floats?

   Also numeric strings could be used to pass integers which are bigger then PHP_INT_MAX and converted internally by PHP itself

2. +++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
   @@ -62,4 +67,54 @@ public function providerTestToInt() {
   +      ['2.4', 2.0],
   +      ['', 0.0],
   

   Please add `9223372036854775807` as string

+++ b/core/lib/Drupal/Component/Utility/Bytes.php
@@ -25,6 +25,21 @@ class Bytes {
+   * @param mixed $size

mixed is bad practice, better use string|int|float so when core will require PHP 8 we can use union types

@andypost, thanks for the review!

Addressing #68 and #69

Bytes::toInt() returns Bytes::toNumber() so providerTestToInt() is no longer needed.

--- a/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
+++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
@@ -5,6 +5,8 @@
...
    *
-   * @dataProvider providerTestToInt
+   * @dataProvider providerTestToNumber
    * @covers ::toInt
    *
    * @group legacy
    * @expectedDeprecation \Drupal\Component\Utility\Bytes::toInt() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\Component\Utility\Bytes::toNumber() instead. See https://www.drupal.org/node/3162663
    */
   public function testToInt($size, $expected_int) {
     $this->assertEquals($expected_int, Bytes::toInt($size));
   }

@jungle something went wrong with your latest patch :(

Re #72, yes, it's a 0 bytes patch 🤦

Started from #67, addressed #68, #69, #71.

Thanks!

Replace calls to toInt() with toNumber(). For example:

--- a/core/lib/Drupal/Component/Utility/Environment.php
+++ b/core/lib/Drupal/Component/Utility/Environment.php
@@ -34,7 +34,7 @@ public static function checkMemoryLimit($required, $memory_limit = NULL) {
     // - The memory limit is set to unlimited (-1).
     // - The memory limit is greater than or equal to the memory required for
     //   the operation.
-    return ((!$memory_limit) || ($memory_limit == -1) || (Bytes::toInt($memory_limit) >= Bytes::toInt($required)));
+    return ((!$memory_limit) || ($memory_limit == -1) || (Bytes::toNumber($memory_limit) >= Bytes::toNumber($required)));
   }

Thanks @Krzysztof Domański!

Addressing #77

1. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -25,6 +25,22 @@ class Bytes {
      public static function toInt($size) {
   +    @trigger_error('\Drupal\Component\Utility\Bytes::toInt() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\Component\Utility\Bytes::toNumber() instead. See https://www.drupal.org/node/3162663', E_USER_DEPRECATED);
   +    return self::toNumber($size);
   +  }
   

   Need to add @deprecated and @see to the docblock.

2. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -25,6 +25,22 @@ class Bytes {
   +  public static function toNumber($size) {
   

   : float return typehint can be added now

3. +++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
   @@ -24,41 +26,66 @@ class BytesTest extends TestCase {
   +  public function testToNumber($size, $expected_number) {
   

   testToNumber($size, float $expected_number): void

4. +++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
   @@ -24,41 +26,66 @@ class BytesTest extends TestCase {
   +  public function providerTestToNumber() {
   

   : array return typehint

#79.2 Unfortunately we should not add return typehint to Bytes::toNumber(). Bytes::toInt() inherits Bytes::toNumber().

Ignore.

Thanks @mondrake!

Addressing #79

+++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
@@ -61,7 +61,7 @@ public function testToNumber($size, $expected_number) {
+  public function providerTestToNumber(): array{

One CS violation, fixing.

Thanks @jungle!

+++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
@@ -24,41 +26,66 @@ class BytesTest extends TestCase {
+      // Zettabytes and yottabytes cannot be represented by integers on 64-bit
+      // systems, so pow() returns a float.
       ['1 ZB'  , pow(Bytes::KILOBYTE, 7)],
       ['1 YB'  , pow(Bytes::KILOBYTE, 8)],

nit: the comment here comes from previous patches - not sure it is relevant anymore. Maybe just cast to float as the lines above for consistency.

+++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
@@ -24,41 +26,66 @@ class BytesTest extends TestCase {
+      [1.5, 2.0],

If the new ::toNumber() method does exactly the same as the old one, then why does the test coverage change? Is this just because we were using assertSame() before which is more permissive?

Re-titling since we're essentially renaming with a bc layer, rather than deprecating in favour of an existing alternative.

1. Re #85

   +++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
   @@ -24,41 +26,66 @@ class BytesTest extends TestCase {
        $this->assertEquals($expected_int, Bytes::toInt($size));
   ...
   +    $this->assertSame($expected_number, Bytes::toNumber($size));
   

   Previously, using assertEquals(), now using assertSame()

   • $this->assertEquals(1, 1.0); // Pass
   • $this->assertSame(1, 1.0); // Fail

   So that changed the data provider.

2. Re-titling since we're essentially renaming with a bc layer, rather than deprecating in favour of an existing alternative.

   From my understanding, I think we should change the title back. the old one (toInt()) is deprecated in favor of the new one (toNumber()), it's not renaming.

Thanks!

#1 makes sense and seems in scope to correct the tests to be inline with the documentation fix here.

Howabout 'replace'? Just trying to summarize the actual change more in the title.

Howabout 'replace'? Just trying to summarize the actual change more in the title.

Sounds good. Thanks!

Committed 7567d54 and pushed to 9.1.x. Thanks!