Skip to content

ext/intl: breakiterator::setText returns false on failure. #10820

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

Closed
wants to merge 1 commit into from

Conversation

devnexen
Copy link
Member

@devnexen devnexen commented Mar 9, 2023

No description provided.

@@ -164,7 +164,7 @@ public function preceding(int $offset): int {}
public function previous(): int {}

/** @tentative-return-type */
public function setText(string $text): ?bool {} // TODO return false instead of null in case of failure
public function setText(string $text): bool {} // TODO return false instead of null in case of failure
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove todo?

Copy link
Member Author

Choose a reason for hiding this comment

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

absolutely.

@devnexen devnexen force-pushed the breakiterator_false branch from 40d9be4 to 3987cea Compare March 10, 2023 07:34
@devnexen devnexen requested a review from kocsismate March 10, 2023 07:58
@devnexen devnexen force-pushed the breakiterator_false branch from 3987cea to 01d51d7 Compare March 10, 2023 08:41
Copy link
Member

@kocsismate kocsismate left a comment

Choose a reason for hiding this comment

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

This LGTM because the return value change is not fundamental so that at least already existing loose comparisons continue to work

@@ -78,7 +78,9 @@ PHP 8.3 UPGRADE NOTES

- Intl:
. datefmt_set_timezone (and its alias IntlDateformatter::setTimeZone)
now returns true on sucess, previously null was returned.
now returns true on success, previously null was returned.
. breakiterator::setText now returns false on failure, previously
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
. breakiterator::setText now returns false on failure, previously
. IntlBreakIterator::setText() now returns false on failure, previously

Function brances (()) in the upgrading file are sometimes omitted, sometimes not, but personally, I'd prefer to have them if possible.

@kocsismate kocsismate requested a review from Girgias March 28, 2023 06:54
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Yeah this one is IMHO fine because null to false doesn't change a lot of expectation

@devnexen devnexen closed this in 7623bf0 Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants