feat: Differentiate between kilobyte/kibibyte and megabyte/mebibyte

[ThomasMeschke]

Notice
This originated from another PR: #9271

Description
Noticed that the file size calculation mixes up the SI unit prefixes with the NIST/IEC unit prefixes, by taking "kb" and "mb" but calculating "kib" and "mib".
Fixed by providing both possibilities.

This therefore will break existing code!

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[neznaika0]

Find all places with method.
user_guide_src/source/libraries/files.rst
user_guide_src/source/libraries/uploaded_files.rst
user_guide_src/source/libraries/files/005.php
user_guide_src/source/libraries/files/013.php

add changelog to user_guide_src/source/changelogs/v4.6.0.rst

[ThomasMeschke]

Will do, will add.

[kenjis]

Why do you need this enhancement?
Frankly, many devs do not seem to need this.
It seems in this framework "kb" means 1024 bytes.

[ThomasMeschke]

Why do you need this enhancement? Frankly, many devs do not seem to need this. It seems in this framework "kb" means 1024 bytes.

#9271 (comment)
Stumbled across this due to a mismatch with another component in use.
"Need" is not how I would see it. More like a "really want to have it right".

[kenjis]

I got your intention.
But we do not prefer breaking changes.
Strictly speaking, we do not allow any breaking changes in minor/patch updates.
But if we think it is a bug, we can fix it in minor updates.

I don't think it is worth adding this breaking change. Because many devs do not need this feature.
What do you think to add a new method or new method parameter for kib/mib?

[kenjis]

How do we address number_to_size? #9271 (comment)

[ThomasMeschke]

I got your intention. But we do not prefer breaking changes. Strictly speaking, we do not allow any breaking changes in minor/patch updates. But if we think it is a bug, we can fix it in minor updates.

I don't think it is worth adding this breaking change. Because many devs do not need this feature. What do you think to add a new method or new method parameter for kib/mib?

I might be getting you wrong, but new parameter is exactly what I did, didn't I?
Or do you mean leave "kb" and "mb" for 1024 an use new ones for 1000?
What would they be? I fear this then gets even more "wrong" or at least confusing than it already is 😕

Regarding number_to_size():
We could introduce two new methods: number_to_bytes_binary and number_to_bytes_metric, let number_to_size internally call the metric binary edition, so its behavior does not change, and mark it as deprecated.

[kenjis]

If we merge this PR, existing apps may break. Because the behavior will change.

-$kilobytes = $file->getSizeByUnit('kb'); // 250.880
+$kilobytes = $file->getSizeByUnit('kb'); // 256.901

This is a breaking change, and we would like to avoid to break existing apps as possible.

Adding a new parameter means something like this:

$kilobytes = $file->getSizeByUnit('kb'); // 250.880
$kilobytes = $file->getSizeByUnit('kb', false); // 256.901

This does not break existing apps.

[ThomasMeschke]

Ah, I see. Well, I personally think introducing breaking changes from time to time is unavoidable, but I can absolutely see the impact vs benefit side of this very instance.
Other option would be the same as for number_to_size, introduce a getSizeByUnitMetric and getSizeByUnitBinary, both allowing "kb" and "mb", calculating differently, and the let getSizeByUnit internally call the Binary version for now and mark it as deprecated.

[kenjis]

Yes, adding new method(s) is an option.

[neznaika0]

"Boolean" methods are usually better separated. This is clearly visible in the code

[ThomasMeschke]

"Boolean" methods are usually better separated. This is clearly visible in the code

Yeah, also not a big fan of bool params to distinguish between behaviors. It would also violate the S in SOLID.
So, if you are fine, I would rework the change to the two-method approach?

[warcooft]

Could you add a new parameter $precision to the this method? This would make it more flexible and allow for better control over the precision of the output.

[neznaika0]

And @phpstan-param positive-int $precision

[ThomasMeschke]

Somebody motivated to enrich the contribution guide about how to run all the code checks locally? 😇

Edit: Just took a look into the running checks and noticed the composer scripts. Didn't even know those were a thing 🤯

[paulbalandan]

@ThomasMeschke could you address the remaining review comments? and squash the commits? so we can merge this PR

[ThomasMeschke]

Which comments do you mean? I though I got all of them...

[michalsn]

@ThomasMeschke Well, that's strange - usually, GitHub would resolve a conversation automatically if the changes to the files were made, but here for some reason, it didn't happen. The unresolved conversations are an indicator for us that not all requested changes were implemented. In the future, please resolve the conversation manually if you see that your changes weren't taken into account.

[michalsn]

I think we can squash on merge.

[paulbalandan]

Thanks, I didn't see it was already resolved albeit no indication it was resolved. 😅

[paulbalandan]

Thank you, @ThomasMeschke