Skip to content

[IntlDateFormatter] Add changelog entry about making dateType and timeType optional #2633

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
Merged
merged 1 commit into from
Aug 2, 2023
Merged

[IntlDateFormatter] Add changelog entry about making dateType and timeType optional #2633

merged 1 commit into from
Aug 2, 2023

Conversation

alexandre-daubois
Copy link
Contributor

Fix #2425

Girgias
Girgias reviewed Jul 31, 2023
View reviewed changes
reference/intl/dateformatter/create.xml Outdated
Comment on lines 165 to 166
Parameters <parameter>dateType</parameter> and
<parameter>timeType</parameter> are made optional.
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
Parameters <parameter>dateType</parameter> and
<parameter>timeType</parameter> are made optional.
Parameters <parameter>dateType</parameter> and
<parameter>timeType</parameter> are now optional.

reference/intl/dateformatter/create.xml Outdated
Comment on lines 71 to 75
Date type to use (<constant>none</constant>, <constant>short</constant>,
<constant>medium</constant>, <constant>long</constant>,
<constant>full</constant>). This is one of the <link
<constant>full</constant>). This is one of the <link
linkend="intl.intldateformatter-constants">IntlDateFormatter
constants</link>.
constants</link>. The default value is <constant>full</constant>.
Copy link
Member

Choose a reason for hiding this comment

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

Those constants need fixing, as they should actually refer to the class too e.g. <constant>IntlDateFormatter:FULL</constant> as when I first saw this I was thinking there was a mistake in the usage of the constant tags instead of literal...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated! Does it make sense to only mention those 5, and not the IntlDateFormatter::RELATIVE_* ones?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know this extension, but if those are valid options I would say yes.

For the * you can do something like <constant>IntlDateFormatter::RELATIVE_<replaceable>*</replaceable></constant> as we determined in the unmerged PR: #1963

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's updated as well with replaceable 👍

@alexandre-daubois alexandre-daubois force-pushed the intldateformatter-ctor-args branch from 2c272f8 to e2f14b3 Compare July 31, 2023 14:49
Girgias
Girgias reviewed Jul 31, 2023
View reviewed changes
reference/intl/dateformatter/create.xml Outdated
Comment on lines 176 to 178
<row>
<entry>5.5.0/PECL 3.0.0</entry>
<entry>
Copy link
Member

Choose a reason for hiding this comment

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

Actually while you are here the 5.5.0 entry can now be removed as we only document PHP 7.0 upwards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be a good idea to prepare a PR removing changelog entries older than PHP 7.0? I'll take care of it if yes 👍

Copy link
Member

Choose a reason for hiding this comment

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

So currently the manual must document PHP 7 and 8, and should remove information relating to PHP 5.

This is more QA work and I don't know how much I want QA work to happen now as this will impact translations working on catching up with the bug fixes and migration guide/infos that need to be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, got it

reference/intl/dateformatter/create.xml Outdated
Comment on lines 71 to 75
Date type to use (<constant>none</constant>, <constant>short</constant>,
<constant>medium</constant>, <constant>long</constant>,
<constant>full</constant>). This is one of the <link
<constant>full</constant>). This is one of the <link
linkend="intl.intldateformatter-constants">IntlDateFormatter
constants</link>.
constants</link>. The default value is <constant>full</constant>.
Copy link
Member

Choose a reason for hiding this comment

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

I don't know this extension, but if those are valid options I would say yes.

For the * you can do something like <constant>IntlDateFormatter::RELATIVE_<replaceable>*</replaceable></constant> as we determined in the unmerged PR: #1963

@alexandre-daubois alexandre-daubois force-pushed the intldateformatter-ctor-args branch from e2f14b3 to 68fe2f8 Compare August 1, 2023 07:05
Girgias
Girgias reviewed Aug 1, 2023
View reviewed changes
reference/intl/dateformatter/create.xml Outdated
Comment on lines 71 to 77
Date type to use (<constant>IntlDateFormatter::NONE</constant>,
<constant>IntlDateFormatter::SHORT</constant>,
<constant>IntlDateFormatter::MEDIUM</constant>,
<constant>IntlDateFormatter::LONG</constant>,
<constant>IntlDateFormatter::FULL</constant> and
<constant>IntlDateFormatter::RELATIVE_<replaceable>*</replaceable></constant>
as of PHP 8.0.0). This is one of the <link
Copy link
Member

@Girgias Girgias Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actually not sure how much of a fan I am of the current phrasing...

Maybe something more like:

Format of the date determined by one of the IntlDateFormatter constants.
By default IntlDateFormatter::FULL.

And just not show a list of formats, as that seems pretty brittle?

Copy link
Contributor Author

@alexandre-daubois alexandre-daubois Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Updated, it will be more future-proof 👍

@alexandre-daubois alexandre-daubois force-pushed the intldateformatter-ctor-args branch from 68fe2f8 to 16f6715 Compare August 1, 2023 14:42
Girgias
Girgias reviewed Aug 1, 2023
View reviewed changes
reference/intl/dateformatter/create.xml Outdated
Comment on lines 71 to 72
Format of the date determined by one of the <link
linkend="intl.intldateformatter-constants">IntlDateFormatter
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
Format of the date determined by one of the <link
linkend="intl.intldateformatter-constants">IntlDateFormatter
Format of the date determined by one of the
<link linkend="intl.intldateformatter-constants">IntlDateFormatter

The stub generating file doesn't like link tag split up into multiple lines and that always causes weird diffs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well noted, updated 👍

@alexandre-daubois alexandre-daubois force-pushed the intldateformatter-ctor-args branch from 16f6715 to 9c23b0c Compare August 2, 2023 08:09
@Girgias Girgias merged commit 1840828 into php:master Aug 2, 2023
@alexandre-daubois alexandre-daubois deleted the intldateformatter-ctor-args branch August 2, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Girgias Girgias Girgias approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prior to PHP 8.1 2nd and 3rd parameter of IntlDateFormatter::__construct / IntlDateFormatter::create are not optional
2 participants
@alexandre-daubois @Girgias