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 the Collator return types in phpdoc #460

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

marien-probesys
Copy link

The types were incoherent with the PHP documentation. There are still some issues with types (e.g. constructor should not accept null value). But changing them would break the API so I thought it wasn't a good idea to fix them yet.

Reference: https://www.php.net/manual/class.collator.php

Contribute to #459

The types were incoherent with the PHP documentation. There are still
some issues with types (e.g. constructor should not accept `null`
value). But changing them would break the API so I thought it wasn't a
good idea to fix them.

Reference: https://www.php.net/manual/class.collator.php
@@ -193,7 +193,7 @@ public function getSortKey(string $string)
/**
* Not supported. Get current collator's strength.
*
* @return bool|int The current collator's strength or false on failure
* @return int The current collator's strength or false on failure
Copy link
Author

Choose a reason for hiding this comment

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

I didn't change the comment because the PHP documentation itself is incoherent (the return type is int, but the doc mentions a false value), see https://www.php.net/manual/en/collator.getstrength.php

Copy link
Member

Choose a reason for hiding this comment

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

could it be that older PHP versions can return false for failures while new versions throw exceptions ?

Copy link
Author

Choose a reason for hiding this comment

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

@stof
Copy link
Member

stof commented Jan 10, 2024

e.g. constructor should not accept null value

We must accept null due to the way internal arguments for scalars are all accepting null even when not marked as nullable (see the deprecation introduced in PHP 8.1), while userland types were always stricter in that regard.

@nicolas-grekas
Copy link
Member

Thank you @marien-probesys.

@nicolas-grekas nicolas-grekas merged commit 3450a58 into symfony:1.x Jan 29, 2024
6 of 9 checks passed
@marien-probesys marien-probesys deleted the fix/collator-types branch January 30, 2024 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants