Skip to content

[12.x] Update "Number::fileSize" to use correct prefix and add prefix param #55678

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 8, 2025

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