feat: Breaking: Differentiate between kilobyte/kibibyte and megabyte/mebibyte #9277

ThomasMeschke · 2024-11-13T15:20:58Z

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 · 2024-11-13T17:11:18Z

Will do, will add.

kenjis · 2024-11-14T07:12:42Z

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 · 2024-11-14T08:29:59Z

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 · 2024-11-14T09:58:24Z

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 · 2024-11-14T10:02:14Z

How do we address number_to_size? #9271 (comment)

ThomasMeschke · 2024-11-14T11:15:34Z

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 · 2024-11-14T11:23:20Z

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 · 2024-11-14T12:02:25Z

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 · 2024-11-15T04:52:31Z

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

neznaika0 · 2024-11-15T05:53:03Z

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

ThomasMeschke · 2024-11-15T10:38:45Z

"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.

system/Files/File.php

neznaika0 · 2024-11-18T09:46:25Z

And @phpstan-param positive-int $precision

user_guide_src/source/changelogs/v4.6.0.rst

kenjis · 2024-11-19T04:12:48Z

For the next PR, could you use a specific branch name (other than 4.6)?

4.6 is the branch name in this repository, so I cannot get your PR with gh pr checkout command.

$ gh pr checkout 9277
remote: Enumerating objects: 97, done.
remote: Counting objects: 100% (97/97), done.
remote: Compressing objects: 100% (65/65), done.
remote: Total 97 (delta 57), reused 70 (delta 32), pack-reused 0 (from 0)
Unpacking objects: 100% (97/97), 27.88 KiB | 68.00 KiB/s, done.
From github.com:codeigniter4/CodeIgniter4
 * branch                  refs/pull/9277/head -> FETCH_HEAD
hint: Diverging branches can't be fast-forwarded, you need to either:
hint:
hint: 	git merge --no-ff
hint:
hint: or:
hint:
hint: 	git rebase
hint:
hint: Disable this message with "git config advice.diverging false"
fatal: Not possible to fast-forward, aborting.
failed to run git: exit status 128

ThomasMeschke · 2024-11-19T09:15:31Z

For the next PR, could you use a specific branch name (other than 4.6)?

4.6 is the branch name in this repository, so I cannot get your PR with gh pr checkout command.

$ gh pr checkout 9277
remote: Enumerating objects: 97, done.
remote: Counting objects: 100% (97/97), done.
remote: Compressing objects: 100% (65/65), done.
remote: Total 97 (delta 57), reused 70 (delta 32), pack-reused 0 (from 0)
Unpacking objects: 100% (97/97), 27.88 KiB | 68.00 KiB/s, done.
From github.com:codeigniter4/CodeIgniter4
 * branch                  refs/pull/9277/head -> FETCH_HEAD
hint: Diverging branches can't be fast-forwarded, you need to either:
hint:
hint: 	git merge --no-ff
hint:
hint: or:
hint:
hint: 	git rebase
hint:
hint: Disable this message with "git config advice.diverging false"
fatal: Not possible to fast-forward, aborting.
failed to run git: exit status 128

Sure, will do, sorry for the inconvenience 😇

Co-authored-by: kenjis <[email protected]>

system/Files/FileSizeUnit.php

system/Files/File.php

Co-authored-by: Michal Sniatala <[email protected]>

* reorder cases and function * add DocBlock for return and throws

ThomasMeschke · 2024-11-19T15:22:09Z

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 🤯

system/Files/FileSizeUnit.php

system/Files/File.php

system/Files/FileSizeUnit.php

system/Files/File.php

system/Files/FileSizeUnit.php

kenjis · 2024-11-20T08:19:16Z

user_guide_src/source/libraries/files.rst


 .. literalinclude:: files/005.php
    :lines: 2-

 A ``RuntimeException`` will be thrown if the file does not exist or an error occurs.

+getSizeByBinaryUnit()
+=====================
+


Suggested change

.. versionadded:: 4.6.0

kenjis · 2024-11-20T08:19:32Z

user_guide_src/source/libraries/files.rst

+
+getSizeByMetricUnit()
+=====================
+


Suggested change

.. versionadded:: 4.6.0

kenjis · 2024-11-20T08:24:36Z

user_guide_src/source/changelogs/v4.6.0.rst

@@ -277,6 +277,9 @@ Deprecations
    - The properties ``$arguments`` and ``$argumentsClass`` of ``Filters`` have
      been deprecated. No longer used.
    - The ``Filters::getArguments()`` method has been deprecated. No longer used.
+- **File:**
+    - The function ``getSizeByUnit()`` of ``File`` has been deprecated.
+      Use either ``getSizeByBinaryUnit()`` or ``getSizeByMetricUnit()`` instead.


Please add the new methods in the Enhancements section:
https://github.com/codeigniter4/CodeIgniter4/blob/4.6/user_guide_src/source/changelogs/v4.6.0.rst#libraries

Will do. Two separate bullet point, or both in one?

One line would be sufficient.

ThomasMeschke mentioned this pull request Nov 13, 2024

refactor: Breaking: Differentiate between kilobyte/kibibyte and megabyte/mebibyte #9271

Closed

5 tasks

neznaika0 suggested changes Nov 13, 2024

View reviewed changes

kenjis added breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities labels Nov 14, 2024

kenjis changed the title ~~refactor: Breaking: Differentiate between kilobyte/kibibyte and megabyte/mebibyte~~ feat: Breaking: Differentiate between kilobyte/kibibyte and megabyte/mebibyte Nov 14, 2024

ThomasMeschke force-pushed the 4.6 branch from de53b38 to 55cd710 Compare November 14, 2024 08:52

michalsn added the 4.6 label Nov 14, 2024

warcooft suggested changes Nov 18, 2024

View reviewed changes

system/Files/File.php Outdated Show resolved Hide resolved

system/Files/File.php Show resolved Hide resolved

system/Files/File.php Outdated Show resolved Hide resolved

system/Files/File.php Outdated Show resolved Hide resolved

system/Files/File.php Outdated Show resolved Hide resolved

refactor: differentiate between kilo/kibibytes and mega/mebibytes

857da2a

ThomasMeschke force-pushed the 4.6 branch from 55cd710 to 857da2a Compare November 18, 2024 09:48

ThomasMeschke added 8 commits November 18, 2024 10:50

add missing param declarations

34b3cfd

add missing return type declarations

0f32963

add missing use statements

8296dc3

replace pow with expression

5282aa3

add missing underline chars

40a867f

add missing blank line

0e4f240

fix github actions

3ad691a

fix failing jobs

a9862ec

ThomasMeschke added 2 commits November 18, 2024 12:30

fix failing jobs

a186c0f

fix failing jobs

c7f21b1

kenjis reviewed Nov 19, 2024

View reviewed changes

user_guide_src/source/changelogs/v4.6.0.rst Outdated Show resolved Hide resolved

kenjis removed the breaking change Pull requests that may break existing functionalities label Nov 19, 2024

Update user_guide_src/source/changelogs/v4.6.0.rst

bf9a9a8

Co-authored-by: kenjis <[email protected]>

michalsn reviewed Nov 19, 2024

View reviewed changes

system/Files/FileSizeUnit.php Show resolved Hide resolved

system/Files/File.php Outdated Show resolved Hide resolved

system/Files/File.php Outdated Show resolved Hide resolved

ThomasMeschke and others added 3 commits November 19, 2024 15:50

allow FileSizeUnit selection from String

8829ea8

Co-authored-by: Michal Sniatala <[email protected]>

revert refactoring of fallback call

3767b29

Co-authored-by: Michal Sniatala <[email protected]>

substantiate deprecation message

f1464e3

Co-authored-by: Michal Sniatala <[email protected]>

ThomasMeschke force-pushed the 4.6 branch from 40f6d7b to beb2154 Compare November 19, 2024 15:18

refactor FileSizeUnit enum

066c03d

* reorder cases and function * add DocBlock for return and throws

ThomasMeschke force-pushed the 4.6 branch from beb2154 to 066c03d Compare November 19, 2024 15:21

michalsn reviewed Nov 19, 2024

View reviewed changes

system/Files/FileSizeUnit.php Outdated Show resolved Hide resolved

paulbalandan reviewed Nov 19, 2024

View reviewed changes

method names, visibility, return types

5bb2170

ThomasMeschke force-pushed the 4.6 branch from 7093a47 to 5bb2170 Compare November 19, 2024 23:21

michalsn reviewed Nov 20, 2024

View reviewed changes

system/Files/FileSizeUnit.php Outdated Show resolved Hide resolved

kenjis reviewed Nov 20, 2024

View reviewed changes

user_guide_src/source/libraries/files.rst

getSizeByMetricUnit()

=====================

Copy link

Member

kenjis Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

.. versionadded:: 4.6.0

kenjis reviewed Nov 20, 2024

View reviewed changes

reorder enum fields, add user guide information

9a3aacb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Breaking: Differentiate between kilobyte/kibibyte and megabyte/mebibyte #9277

feat: Breaking: Differentiate between kilobyte/kibibyte and megabyte/mebibyte #9277

ThomasMeschke commented Nov 13, 2024 •

edited

Loading

neznaika0 left a comment

ThomasMeschke commented Nov 13, 2024

kenjis commented Nov 14, 2024 •

edited

Loading

ThomasMeschke commented Nov 14, 2024

kenjis commented Nov 14, 2024 •

edited

Loading

kenjis commented Nov 14, 2024

ThomasMeschke commented Nov 14, 2024 •

edited

Loading

kenjis commented Nov 14, 2024

ThomasMeschke commented Nov 14, 2024

kenjis commented Nov 15, 2024

neznaika0 commented Nov 15, 2024

ThomasMeschke commented Nov 15, 2024

warcooft left a comment

neznaika0 commented Nov 18, 2024

kenjis commented Nov 19, 2024

ThomasMeschke commented Nov 19, 2024

ThomasMeschke commented Nov 19, 2024 •

edited

Loading

kenjis Nov 20, 2024

kenjis Nov 20, 2024

kenjis Nov 20, 2024

ThomasMeschke Nov 21, 2024

kenjis Nov 26, 2024

feat: Breaking: Differentiate between kilobyte/kibibyte and megabyte/mebibyte #9277

Are you sure you want to change the base?

feat: Breaking: Differentiate between kilobyte/kibibyte and megabyte/mebibyte #9277

Conversation

ThomasMeschke commented Nov 13, 2024 • edited Loading

neznaika0 left a comment

Choose a reason for hiding this comment

ThomasMeschke commented Nov 13, 2024

kenjis commented Nov 14, 2024 • edited Loading

ThomasMeschke commented Nov 14, 2024

kenjis commented Nov 14, 2024 • edited Loading

kenjis commented Nov 14, 2024

ThomasMeschke commented Nov 14, 2024 • edited Loading

kenjis commented Nov 14, 2024

ThomasMeschke commented Nov 14, 2024

kenjis commented Nov 15, 2024

neznaika0 commented Nov 15, 2024

ThomasMeschke commented Nov 15, 2024

warcooft left a comment

Choose a reason for hiding this comment

neznaika0 commented Nov 18, 2024

kenjis commented Nov 19, 2024

ThomasMeschke commented Nov 19, 2024

ThomasMeschke commented Nov 19, 2024 • edited Loading

kenjis Nov 20, 2024

Choose a reason for hiding this comment

kenjis Nov 20, 2024

Choose a reason for hiding this comment

kenjis Nov 20, 2024

Choose a reason for hiding this comment

ThomasMeschke Nov 21, 2024

Choose a reason for hiding this comment

kenjis Nov 26, 2024

Choose a reason for hiding this comment

ThomasMeschke commented Nov 13, 2024 •

edited

Loading

kenjis commented Nov 14, 2024 •

edited

Loading

kenjis commented Nov 14, 2024 •

edited

Loading

ThomasMeschke commented Nov 14, 2024 •

edited

Loading

ThomasMeschke commented Nov 19, 2024 •

edited

Loading