Skip to content

ext/intl: dateformatter settimezone changes on success, returning true #10790

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

ext/intl: dateformatter settimezone changes on success, returning true #10790

wants to merge 1 commit into from
+25 −19

Conversation

devnexen
Copy link
Member

@devnexen devnexen commented Mar 5, 2023

like setcalendar.

@devnexen devnexen requested a review from iluuu1994 March 5, 2023 21:00
iluuu1994
iluuu1994 reviewed Mar 6, 2023
View reviewed changes
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Returning true makes sense to me. Could we add this to the tests that call ut_datefmt_set_timezone_id?

Girgias
Girgias reviewed Mar 6, 2023
View reviewed changes
@Girgias
Copy link
Member

Girgias commented Mar 6, 2023

This is technically an API break, but the current API is so whack that I think it's fine...

@devnexen devnexen force-pushed the intl_dateformat_upd branch from 3565411 to 97e6f3d Compare March 6, 2023 18:39
@devnexen
Copy link
Member Author

devnexen commented Mar 6, 2023 •
edited
Loading

Could we add this to the tests that call ut_datefmt_set_timezone_id?

sure

@devnexen devnexen force-pushed the intl_dateformat_upd branch 3 times, most recently from d8ffc93 to f6a8163 Compare March 6, 2023 21:19
@devnexen devnexen force-pushed the intl_dateformat_upd branch from f6a8163 to 84b446d Compare March 7, 2023 10:57
kocsismate
kocsismate reviewed Mar 7, 2023
View reviewed changes
UPGRADING Outdated
@@ -76,6 +76,10 @@ PHP 8.3 UPGRADE NOTES
- Dom:
. Changed DOMCharacterData::appendData() tentative return type to true.

- Intl:
. IntlDateformatter::setTimeZone (and its alias dafmt_set_timezone)
Copy link
Member

Choose a reason for hiding this comment

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

FYI: it's actually the other way around. dafmt_set_timezone has the implementation, and IntlDateformatter::setTimeZone is the alias. @alias foo is stubs basically mean that "this function/method is an alias of foo"

Copy link
Member Author

Choose a reason for hiding this comment

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

good point

@devnexen devnexen force-pushed the intl_dateformat_upd branch from 84b446d to f3fe321 Compare March 7, 2023 14:25
kocsismate
kocsismate reviewed Mar 9, 2023
View reviewed changes
UPGRADING Outdated
@@ -76,6 +76,10 @@ PHP 8.3 UPGRADE NOTES
- Dom:
. Changed DOMCharacterData::appendData() tentative return type to true.

- Intl:
. datefmT_set_timezone (and its alias IntlDateformatter::setTimeZone)
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
. datefmT_set_timezone (and its alias IntlDateformatter::setTimeZone)
. datefmt_set_timezone (and its alias IntlDateformatter::setTimeZone)

@devnexen devnexen force-pushed the intl_dateformat_upd branch from f3fe321 to fc8b8a8 Compare March 9, 2023 12:24
iluuu1994
iluuu1994 reviewed Mar 9, 2023
View reviewed changes
ext/phar/phar_object.c Outdated
@@ -2889,8 +2889,6 @@ PHP_METHOD(Phar, setStub)
efree(error);
RETURN_THROWS();
}

RETURN_TRUE;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like an accidental unrelated change?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes..

@devnexen devnexen force-pushed the intl_dateformat_upd branch from fc8b8a8 to 5b05cb8 Compare March 9, 2023 12:32
@devnexen
Copy link
Member Author

devnexen commented Mar 9, 2023

@iluuu1994 does it look ok now ? 🙂

iluuu1994
iluuu1994 approved these changes Mar 9, 2023
View reviewed changes
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Looks good from my side, thank you @devnexen 🙂

@devnexen devnexen closed this in 4567708 Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kocsismate kocsismate

@Girgias Girgias

@iluuu1994 iluuu1994

Assignees
No one assigned
Labels
Extension: intl
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@devnexen @Girgias @iluuu1994 @kocsismate