Skip to content
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

Fix formatCurrency return type #8349

Merged
merged 1 commit into from Jul 31, 2022
Merged

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Jul 30, 2022

Hi @orklah, I just got an error [TypeDoesNotContainType](https://psalm.dev/056) - 6:9 - string does not contain false.

but NumberFormatter::formatCurrency can return false
See https://www.php.net/manual/fr/numberformatter.formatcurrency.php

@AndrolGenhald
Copy link
Collaborator

AndrolGenhald commented Jul 30, 2022

Do you know what cases this can return false for? If it only returns false when called with invalid arguments then we intentionally exclude it from the return type.

It looks like it happens here, but I don't know what the possibilities for INTL_DATA_ERROR_CODE((nfo)) are or why they would happen. If you can find a case on 3v4l.org that returns false for valid arguments that would be excellent.

Further investigation leads here, which possibly sets it to U_MEMORY_ALLOCATION_ERROR which I'm not sure we care about. I'm always a bit nervous when looking at current code for linked libraries though, since it's possible an older version does something different, and that's not tied directly to the PHP version.

@orklah
Copy link
Collaborator

orklah commented Jul 31, 2022

Usually, we would remove the false return when it was related to wrong param types. I'd say the cases are the same than function that now throws a TypeError in PHP8 instead of returning false.

So if a function still return false in PHP8, I think we should keep the falsable return

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Jul 31, 2022

You can see it's here for

'NumberFormatter::format' => ['string|false', 'num'=>'', 'type='=>'int'],

So it should be consistent and be here for formatCurrency.

It should allow to use a false check

$result = $formatter->formatCurrency(...);
if (false !== $result) {
     ...
}

which is valid php code.

Further investigation leads here, which possibly sets it to U_MEMORY_ALLOCATION_ERROR which I'm not sure we care about.

As soon as it's possible, the false check should not be reported as an error.

So if a function still return false in PHP8, I think we should keep the falsable return

100% agree.

Then if someone want to improve the return type, it should be done with a dynamic return type calculation

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Jul 31, 2022
@orklah
Copy link
Collaborator

orklah commented Jul 31, 2022

Thanks!

@orklah orklah merged commit 6998fab into vimeo:4.x Jul 31, 2022
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants