Skip to content

ext/intl: Delete outdated TODO message to support numeric-castable objects as date/time values in IntlDateFormatter->format()#21959

Open
LamentXU123 wants to merge 1 commit intophp:masterfrom
LamentXU123:clean-up-
Open

ext/intl: Delete outdated TODO message to support numeric-castable objects as date/time values in IntlDateFormatter->format()#21959
LamentXU123 wants to merge 1 commit intophp:masterfrom
LamentXU123:clean-up-

Conversation

@LamentXU123
Copy link
Copy Markdown
Contributor

@LamentXU123 LamentXU123 commented May 6, 2026

Allow IntlDateFormatter->format() to accept objects that support numeric casting, in addition to IntlCalendar and DateTimeInterface as per the TODO message.

@LamentXU123 LamentXU123 requested a review from devnexen as a code owner May 6, 2026 01:11
@LamentXU123 LamentXU123 changed the title ext/intl: Allow numeric-castable objects as date/time values in IntlDateFormatter::formatObject() ext/intl: Allow numeric-castable objects as date/time values in IntlDateFormatter->format() May 6, 2026
Comment thread ext/intl/common/common_date.cpp Outdated
rv = U_MILLIS_PER_SECOND * (double)Z_LVAL(casted);
} else if (Z_TYPE(casted) == IS_DOUBLE) {
rv = U_MILLIS_PER_SECOND * Z_DVAL(casted);
} else {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you need to gate it with !EG(exception)

Copy link
Copy Markdown
Contributor Author

@LamentXU123 LamentXU123 May 6, 2026

Choose a reason for hiding this comment

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

Ah yeah good catch :) Please check it again and see if now its right enough.

Comment thread ext/intl/tests/dateformat_formatObject_numeric_object.phpt Outdated
Comment thread ext/intl/tests/dateformat_format_error.phpt Outdated
Copy link
Copy Markdown
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.

I'm not sure that we should do this. This would differ from the usual semantics of string|int|float|IntlCalendar|DateTimeInterface type if done in userland, especially as we already don't handle stringable objects.

@LamentXU123
Copy link
Copy Markdown
Contributor Author

LamentXU123 commented May 7, 2026

I'm not sure that we should do this. This would differ from the usual semantics of string|int|float|IntlCalendar|DateTimeInterface type if done in userland, especially as we already don't handle stringable objects.

That make sense, I don't have strong opinions on this point since I am submitting this patch as an implementation to the TODO message. Maybe the code owner can decide it.

@devnexen
Copy link
Copy Markdown
Member

devnexen commented May 7, 2026

I planned to have a better look at this this weekend, Gina's comment makes sense ; making me reconsider my initial judgement ... we ll see.

@LamentXU123
Copy link
Copy Markdown
Contributor Author

I planned to have a better look at this this weekend, Gina's comment makes sense ; making me reconsider my initial judgement ... we ll see.

Ah no need to rush this :) I am investigating why we are writing this todo message initially. Will post here.

@LamentXU123
Copy link
Copy Markdown
Contributor Author

LamentXU123 commented May 7, 2026

I would say people initially write this in the very beginning of this file history 2416719 when the usual semantics of string|int|float|IntlCalendar|DateTimeInterface is yet being constructed.

So I will consider this as a outdated TODO comment. I would prefer to delete the comment and remain as-is for the rest of the code base.

@devnexen
Copy link
Copy Markdown
Member

devnexen commented May 8, 2026

I would say people initially write this in the very beginning of this file history 2416719 when the usual semantics of string|int|float|IntlCalendar|DateTimeInterface is yet being constructed.

So I will consider this as a outdated TODO comment. I would prefer to delete the comment and remain as-is for the rest of the code base.

I think it s preferable indeed.

@LamentXU123
Copy link
Copy Markdown
Contributor Author

LamentXU123 commented May 8, 2026

Done. I forget to add [fix CI] >_<
Anyway, its safe to merge

@LamentXU123 LamentXU123 changed the title ext/intl: Allow numeric-castable objects as date/time values in IntlDateFormatter->format() ext/intl: Delete outdated TODO message to support numeric-castable objects as date/time values in IntlDateFormatter->format() May 8, 2026
@devnexen
Copy link
Copy Markdown
Member

devnexen commented May 8, 2026

once done, would you mind ?

  • squashing in 1 commit.
  • having a commit message that reflects what actually is done

Thanks.

@LamentXU123 LamentXU123 force-pushed the clean-up- branch 2 times, most recently from b62e43d to 768ae95 Compare May 8, 2026 06:57
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.

3 participants