Skip to content

ext/intl: Add IntlDatePatternGenerator skeleton methods - #23078

Open
LamentXU123 wants to merge 5 commits into
php:masterfrom
LamentXU123:datepatterngenerator-skeletons
Open

ext/intl: Add IntlDatePatternGenerator skeleton methods#23078
LamentXU123 wants to merge 5 commits into
php:masterfrom
LamentXU123:datepatterngenerator-skeletons

Conversation

@LamentXU123

Copy link
Copy Markdown
Member

This add missing Skeleton functions similar to #23049 and #23017 for IntlDatePatternGenerator. This adds IntlDatePatternGenerator::getSkeleton() and IntlDatePatternGenerator::getBaseSkeleton()

INTL_METHOD_CHECK_STATUS(dtpgo, "Pattern is not a valid UTF-8 string");

UnicodeString result = base
? dtpgo->dtpg->getBaseSkeleton(pattern, DTPATTERNGEN_ERROR_CODE(dtpgo))

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.

per icu headers

* getSkeleton() works exactly like staticGetSkeleton().
* Use staticGetSkeleton() instead of getSkeleton().

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.

Thus, since the output wouldn't depend on the generator's locale, the constructor is more a "gimmick" than actually meaningful. wdyt ?

@LamentXU123 LamentXU123 Aug 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

per icu headers

* getSkeleton() works exactly like staticGetSkeleton().
* Use staticGetSkeleton() instead of getSkeleton().

I saw this line ofc. I made the decision to use this because this is a directly exposure and using staticGetSkeleton might confuse people when reading this line (why not use ICU's getSkeleton in the getSkeleton() implementation)

per source:

void DateTimePatternGenerator::GetSkeleton(UnicodeString &buffer) const {
    staticGetSkeleton(this, buffer);
}

void DateTimePatternGenerator::staticGetSkeleton(const DecimalFormat *df, UnicodeString &buffer) {
    buffer.clear();
     balabalabala....
}

They are literally the same. wdyt?

(I think the the reason why the documentation recommend people to use the static version is because GetSkeleton is a virtual instance member method of DateTimePatternGenerator. However, since we are just exposing it to PHP userland I don't think we need to take this into consideration)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok I see the reason to use the static version. Ignore my comment above

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.

Yes, neither overload touches PHP's "this" at all, which proves your locale point. The generator instance only carries the error state, so constructing one loads locale data for nothing.

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.

note: I hope you do not have too much to "cram" before the feature freeze btw ?

@LamentXU123 LamentXU123 Aug 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thus, since the output wouldn't depend on the generator's locale, the constructor is more a "gimmick" than actually meaningful. wdyt ?

This is indeed a good suggestion. In this case they should be static methods.

note: I hope you do not have too much to "cram" before the feature freeze btw ?

I won't :) This just happens to came across my mind when I am reviewing your SpoofChecker PR yesterday.

Yes, neither overload touches PHP's "this" at all, which proves your locale point. The generator instance only carries the error state, so constructing one loads locale data for nothing.

Exactly.

Co-Authored-By: David CARLIER <devnexen@gmail.com>
@devnexen

devnexen commented Aug 6, 2026

Copy link
Copy Markdown
Member

Having a case that uses this pattern would be nice

$patterns = [
    "'at' HH:mm",
    "y年M月d日",
    "yyyy-MM-dd",
    "MMMMM",
    "MMM",
    "HH:mm:ss zzz dd/MM/y",
    "'at HH:mm",
];
foreach ($patterns as $p) {
    printf("%-22s %-12s %s\n", $p,
        IntlDatePatternGenerator::getSkeleton($p),
        IntlDatePatternGenerator::getBaseSkeleton($p));
}

Co-Authored-By: David CARLIER <devnexen@gmail.com>
RETVAL_STR(u8str);
}

static void dtpg_get_skeleton(INTERNAL_FUNCTION_PARAMETERS, bool base)

@devnexen devnexen Aug 6, 2026

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.

nit: we re not constrained by C limitations we can do here like has been done for spoofchecker earlier:

template <typename F>
static void dtpg_get_skeleton(INTERNAL_FUNCTION_PARAMETERS, F&& skeletonfn, const char *errmsg)...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I personally dont like template syntax. But its fine anyways.

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.

understandable, albeit template usage here is very basic I do not think we would need to go much further anytime soon.

Co-Authored-By: David CARLIER <devnexen@gmail.com>

@devnexen devnexen left a comment

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.

lgtm

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.

2 participants