Skip to content

Conversation

@Boy132
Copy link
Contributor

@Boy132 Boy132 commented May 8, 2025

Currently Number::fileSize uses 1024 as base to do the calculations but labels it wrong. With base 1024 a binary prefix should be used.
Normally people expect 1000 as base but for more technical applications base 1024 is needed. That's why there is now an optional parameter to use the binary prefix.

I also added RB/ RiB and QB/ QiB to the units array.

@taylorotwell taylorotwell merged commit 58ce619 into laravel:12.x May 8, 2025
30 checks passed
@huangdijia
Copy link
Contributor

This is a breaking change.

@francoism90
Copy link

Shouldn't bool $useBinaryPrefix = false be true? As this indeed may be breaking like @huangdijia commented.

@Pyker
Copy link

Pyker commented May 14, 2025

@taylorotwell This is a breaking change. $useBinaryPrefix needs to default to true for it to not be a breaking change. Either that or change it to $useSiPrefix instead (and update the ternary operator logic), which can then be defaulted to false.

The breaking change is clearly demonstrated by the values in the tests changing from what is expected: https://github.com/laravel/framework/pull/55678/files#diff-c547ec5639f2da21195501634ff9f2e492634de0158eb8085c3c073fb3c730f7L177-L178

@Boy132
Copy link
Contributor Author

Boy132 commented May 14, 2025

Even with $useBinaryPrefix defaulting to true the value is different.
But I wouldn't call that a breaking change since the human readable representation should be only used for display.

@Pyker
Copy link

Pyker commented May 14, 2025

Yes, but even in display terms it's a breaking change, because there are UX implications to something changing from 100 MB to 104.86 MB out of nowhere (104 857 600 bytes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants