Feature/support scaled percentages #54049
Conversation
This change more closely aligns the return value of the percentage method in the `Illuminate/Support/Numbers` class with the return value of the underlying PHP NumberFormatter class. The NumberFormatter class expects a decimal and returns a scaled value by a factor of 100. While I think it is potentially useful to get back the original input localized and formatted, I also think it is useful to get back the scaled and formatted version of the input as well. The changes made here were to help maintain backward compatibility with the current implementation of the percentage method.
There was a problem hiding this comment.
I agree that this is not a breaking change, however, due to its implementation already diverging from the one of the underlying NumberFormatter class, this doesn't actually bring the implementations (much|any) closer together, due to the introduction of a new workaround parameter that doesn't exist on said base class. The original implementation is unfortunate and this is probably the only way to fix this without introducing breaking changes, if we actually want to change the behavior at this point still.
| if ($scaled) { | ||
| return $formatter->format($number); | ||
| } | ||
|
|
||
| return $formatter->format($number / 100); |
There was a problem hiding this comment.
We could do the following instead, in case, we want to do something with the formatted output or the like in the future:
| if ($scaled) { | |
| return $formatter->format($number); | |
| } | |
| return $formatter->format($number / 100); | |
| if (! $scaled) { | |
| $number /= 1000; | |
| } | |
| return $formatter->format($number); |
There was a problem hiding this comment.
This was close to my original implementation but breaks backward compatibility. I do like this much better though. Should I refactor and point to the master branch instead of 11.x?
|
I agree this implementation is not ideal but as you mentioned this is the cleanest way I could think of without making braking changes. I still think this is worth fixing and valuable to the framework. Would you prefer if I made breaking changes and got them in with the next major release? |
In the end, this would be Taylor's choice to make |
|
Thanks for your pull request to Laravel! Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include. If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions! |
Summary
This change aligns the return value of the
percentagemethod in theIlluminate/Support/Numberclass with the behavior of the underlying PHPNumberFormatterclass. TheNumberFormatterclass expects a decimal input and returns a scaled value by a factor of 100 (e.g.,0.75becomes75%).While returning the original, unscaled input localized and formatted is useful, it's also important to provide the scaled and formatted percentage as it is commonly expected in most applications.
Benefits to End Users
Why It Doesn't Break Existing Features
scaledparameter (defaulting tofalse) ensures that existing implementations of thepercentagemethod remain unaffected. The method will continue returning the percentage as before unless thescaledparameter is explicitly set totrue.How It Makes Building Web Applications Easier
75%in English or75%in French) can now easily take advantage of the built-in localization capabilities of theNumberFormatterclass.NumberFormatter's behavior, this change ensures consistent handling of percentages across applications, aligning with established practices in web and software development.