[Intl][5.0] Add parameters type-hints #32525
Conversation
| @@ -384,7 +384,6 @@ public function formatFractionDigitsProvider() | |||
| [1.123, '1.1', 1, 1], | |||
| [1.123, '1.12', 2, 2], | |||
| [1.123, '1.123', -1, 0], | |||
| [1.123, '1', 'abc', 0], | |||
There was a problem hiding this comment.
'abc' is passed as the value of the setAttribute method here:
https://github.com/symfony/symfony/pull/32525/files#diff-2a5ae15ec68c5d666ce08a09d75690efR564
It's type-hinted at int so it throws a TypeError, however method's logic seems to handle case where it could be a string:
I decided to keep the type hint as int and remove the use case of a string value
src/Symfony/Component/Intl/Data/Bundle/Writer/TextBundleWriter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Intl/Tests/NumberFormatter/NumberFormatterTest.php
Show resolved
Hide resolved
|
I decided at first to skip type-hints on methods throwing But since #32525 (review) was asked I've decided to be consistent through the rest of the codebase: |
src/Symfony/Component/Intl/Data/Bundle/Writer/TextBundleWriter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Intl/Data/Bundle/Writer/TextBundleWriter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Intl/Data/Generator/AbstractDataGenerator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Intl/DateFormatter/DateFormat/FullTransformer.php
Outdated
Show resolved
Hide resolved
| * @param string $currency The 3-letter ISO 4217 currency code indicating the currency to use | ||
| * | ||
| * @return string The formatted currency value | ||
| * | ||
| * @see http://www.php.net/manual/en/numberformatter.formatcurrency.php | ||
| * @see https://en.wikipedia.org/wiki/ISO_4217#Active_codes | ||
| */ | ||
| public function formatCurrency($value, $currency) | ||
| public function formatCurrency(float $value, string $currency) |
There was a problem hiding this comment.
I‘m not sure if we should actually force float here. Storing a decimal value as string is a valid pattern, especially when it comes to currency amounts.
There was a problem hiding this comment.
AFAIU, this NumberFormatter implementation is a sort of polyfill for Intl::NumberFormatter where $value is typed as float and generates the following error if passed as a string with strict types:
Fatal error: Uncaught TypeError: NumberFormatter::formatCurrency() expects parameter 1 to be float, string given in
So question is, should we accept a wider range of type compared to the implementation it's trying to polyfill ?
Personally I would say no, I would expect this function to also throws a TypeError on string arg if the whole point of this class is to be as close as the "real" one.
I'll leave it type-hinted as a float for now, open to modification if team says otherwise.
There was a problem hiding this comment.
All right, thanks for checking.
|
@tigitz Can you have a look at the remaining comment here? Thank you. |
|
@fabpot yep will do tonight 👌 |
|
Status: Needs Review @derrabus When you have time ;) |
|
It seems almost all classes in intl which are affected here are internal classes, e.g. for polyfill that are not meant to be extended. So we can add type hints in 4.4 to them because we don't need parameter variance from 7.2. What do you think @nicolas-grekas ? |
src/Symfony/Component/Intl/Data/Bundle/Writer/TextBundleWriter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Intl/Data/Bundle/Writer/TextBundleWriter.php
Outdated
Show resolved
Hide resolved
|
@Tobion I was about to answer yes, but |
…y of internal class (Tobion) This PR was merged into the 3.4 branch. Discussion ---------- [Intl] fix nullable phpdocs and useless method visibility of internal class | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | | License | MIT | Doc PR | Fix stuff found in #32525 Commits ------- 63b71b5 [Intl] fix nullable phpdocs and useless method visibility of internal class
|
Thank you @tigitz. |
…, Tobion) This PR was squashed before being merged into the 5.0-dev branch (closes #32525). Discussion ---------- [Intl][5.0] Add parameters type-hints | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | continuation of #24722 and checks for #32179 | License | MIT | Doc PR | N/A This PR replaces docblocks by type hints in the Intl component considering #32179. Some docblocks without valuable information got also removed. Commits ------- ce79f4b [Intl][5.0] Add parameters type-hints
This PR replaces docblocks by type hints in the Intl component considering #32179. Some docblocks without valuable information got also removed.