ext/intl: IntlDateFormatter::parse adds new optional argument. by devnexen · Pull Request #13779 · php/php-src

devnexen · 2024-03-21T19:45:59Z

New option to update its internal calendar.

ext/intl/dateformat/dateformat.stub.php

kocsismate · 2024-03-25T08:25:41Z

ext/intl/dateformat/dateformat.stub.php


    /**
     * @param int $offset
+     * @param bool $updateCalendar


this shouldn't be added since it doesn't have more information than the param type declaration

kocsismate · 2024-03-25T08:26:50Z

ext/intl/php_intl.stub.php

-function datefmt_parse(IntlDateFormatter $formatter, string $string, &$offset = null): int|float|false {}
+/**
+ * @param int $offset
+ * @param bool $updateCalendar


kocsismate · 2024-03-25T08:42:55Z

ext/intl/dateformat/dateformat.stub.php

     * @alias datefmt_parse
     */
-    public function parse(string $string, &$offset = null): int|float|false {}
+    public function parse(string $string, &$offset = null, bool $updateCalendar = false): int|float|false {}


I don't want to bikeshed this much, but I'd still want to throw one more idea to think about:

public function parseToCalendar(string $string, &$offset = null): void {}

Maybe a signature like this would convey the message better that it is intended to modify the internal state. Let's wait for someone else's review and go with the signature which has more support :) I'm also fine with yours even though it's not my preference

Oh, and I almost forgot: having a completely new method would also minimize the BC break, since IntlDateformatter::parse() is not final, so people may have been overriding it.

valid points, thanks. Will update later.

kocsismate · 2024-03-25T20:51:26Z

ext/intl/php_intl.stub.php

 function datefmt_parse(IntlDateFormatter $formatter, string $string, &$offset = null): int|float|false {}

+/** @param int $offset */
+function datefmt_parse_tocalendar(IntlDateFormatter $formatter, string $string, &$offset = null): int|float|false {}


lately, the procedural interface is not extended anymore 🤔 See https://externals.io/message/114473#114673

devnexen · 2024-04-24T22:40:43Z

ping :)

Girgias

What does this new method actually do?

Girgias · 2024-04-24T22:45:44Z

ext/intl/dateformat/dateformat_parse.c

+	if (UNEXPECTED(update_calendar)) {
+		UCalendar      *parsed_calendar = (UCalendar *)udat_getCalendar(DATE_FORMAT_OBJECT(dfo));
+		udat_parseCalendar( DATE_FORMAT_OBJECT(dfo), parsed_calendar, text_utf16, text_utf16_len, parse_pos, &INTL_DATA_ERROR_CODE(dfo));
+		if( text_utf16 ){


Nit: CS

Suggested change

if( text_utf16 ){

if (text_utf16) {

Girgias · 2024-04-24T22:45:57Z

ext/intl/dateformat/dateformat_parse.c

+		timestamp = ucal_getMillis( parsed_calendar, &INTL_DATA_ERROR_CODE(dfo));
+	} else {
+		timestamp = udat_parse( DATE_FORMAT_OBJECT(dfo), text_utf16, text_utf16_len, parse_pos, &INTL_DATA_ERROR_CODE(dfo));
+		if( text_utf16 ){


Nit: CS

Suggested change

if( text_utf16 ){

if (text_utf16) {

Girgias · 2024-04-24T22:46:48Z

ext/intl/dateformat/dateformat_parse.c

+	char*           text_to_parse = NULL;
+	size_t          text_len =0;
+	zval*         	z_parse_pos = NULL;
+	int32_t		parse_pos = -1;


CS: Either align them properly or don't bother having any spaces, but this is just weird

Girgias · 2024-04-24T22:47:01Z

ext/intl/dateformat/dateformat_parse.c

+	DATE_FORMAT_METHOD_INIT_VARS;
+
+	/* Parse parameters. */
+	if( zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "Os|z!",


Nit: CS

Suggested change

if( zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "Os|z!",

if (zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "Os|z!",

Girgias · 2024-04-24T22:47:58Z

ext/intl/dateformat/dateformat_parse.c

+	zval*         	z_parse_pos = NULL;
+	int32_t		parse_pos = -1;
+
+	DATE_FORMAT_METHOD_INIT_VARS;


Blerg I hate those sorts of macros.... but that's an existing problem with the codebase.

Girgias · 2024-04-24T22:49:02Z

ext/intl/dateformat/dateformat_parse.c

+	DATE_FORMAT_METHOD_INIT_VARS;
+
+	/* Parse parameters. */
+	if( zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "Os|z!",


You don't need to use zend_parse_method_parameters() when just adding a new method, as the getThis() is pointless and the first param being an object is also unnecessary.

Girgias · 2024-04-24T22:50:20Z

ext/intl/dateformat/dateformat_parse.c

+		}
+	}
+	internal_parse_to_timestamp( dfo, text_to_parse, text_len, z_parse_pos?&parse_pos:NULL, true, return_value);
+	if(z_parse_pos) {


Nit: CS

Suggested change

if(z_parse_pos) {

if (z_parse_pos) {

Girgias · 2024-04-24T22:51:03Z

ext/intl/dateformat/dateformat_parse.c

+	if (z_parse_pos) {
+		zend_long long_parse_pos;
+		ZVAL_DEREF(z_parse_pos);
+		long_parse_pos = zval_get_long(z_parse_pos);


Use the new zval_try_get_long() API instead?

Girgias · 2024-04-24T22:51:16Z

ext/intl/dateformat/dateformat_parse.c

+			RETURN_FALSE;
+		}
+		parse_pos = (int32_t)long_parse_pos;
+		if((size_t)parse_pos > text_len) {


Nit: CS

Suggested change

if((size_t)parse_pos > text_len) {

if ((size_t)parse_pos > text_len) {

Girgias · 2024-04-24T22:51:51Z

ext/intl/dateformat/dateformat_parse.c

+			RETURN_FALSE;
+		}
+	}
+	internal_parse_to_timestamp( dfo, text_to_parse, text_len, z_parse_pos?&parse_pos:NULL, true, return_value);


Nit: CS

Suggested change

internal_parse_to_timestamp( dfo, text_to_parse, text_len, z_parse_pos?&parse_pos:NULL, true, return_value);

internal_parse_to_timestamp( dfo, text_to_parse, text_len, z_parse_pos ? &parse_pos : NULL, true, return_value);

Girgias · 2024-04-25T15:16:07Z

ext/intl/dateformat/dateformat_parse.c

 		}
 		parse_pos = (int32_t)long_parse_pos;
-		if((size_t)parse_pos > text_len) {
+		if((size_t)parse_pos > ZSTR_LEN(text_to_parse)) {


Are negative indexes supported?

Girgias · 2024-04-25T15:16:21Z

ext/intl/dateformat/dateformat_parse.c

+		Z_PARAM_ZVAL(z_parse_pos)
+	ZEND_PARSE_PARAMETERS_END();
+
+	object = getThis();


Use ZEND_THIS?

Girgias

Minor CS nits again, and adding test cases for the unhappy path

Girgias · 2024-04-25T21:21:16Z

ext/intl/dateformat/dateformat_parse.c

+		UCalendar      *parsed_calendar = (UCalendar *)udat_getCalendar(DATE_FORMAT_OBJECT(dfo));
+		udat_parseCalendar( DATE_FORMAT_OBJECT(dfo), parsed_calendar, text_utf16, text_utf16_len, parse_pos, &INTL_DATA_ERROR_CODE(dfo));
+		if(text_utf16) {


Nit: CS

Suggested change

UCalendar *parsed_calendar = (UCalendar *)udat_getCalendar(DATE_FORMAT_OBJECT(dfo));

udat_parseCalendar( DATE_FORMAT_OBJECT(dfo), parsed_calendar, text_utf16, text_utf16_len, parse_pos, &INTL_DATA_ERROR_CODE(dfo));

if(text_utf16) {

UCalendar *parsed_calendar = (UCalendar *)udat_getCalendar(DATE_FORMAT_OBJECT(dfo));

udat_parseCalendar(DATE_FORMAT_OBJECT(dfo), parsed_calendar, text_utf16, text_utf16_len, parse_pos, &INTL_DATA_ERROR_CODE(dfo));

if (text_utf16) {

Girgias · 2024-04-25T21:22:09Z

ext/intl/dateformat/dateformat_parse.c

+		timestamp = udat_parse( DATE_FORMAT_OBJECT(dfo), text_utf16, text_utf16_len, parse_pos, &INTL_DATA_ERROR_CODE(dfo));
+		if(text_utf16) {
+			efree(text_utf16);
+		}


Nit: CS

Suggested change

timestamp = udat_parse( DATE_FORMAT_OBJECT(dfo), text_utf16, text_utf16_len, parse_pos, &INTL_DATA_ERROR_CODE(dfo));

if(text_utf16) {

efree(text_utf16);

}

timestamp = udat_parse(DATE_FORMAT_OBJECT(dfo), text_utf16, text_utf16_len, parse_pos, &INTL_DATA_ERROR_CODE(dfo));

if (text_utf16) {

efree(text_utf16);

}

Girgias · 2024-04-25T21:22:57Z

ext/intl/dateformat/dateformat_parse.c

+	zend_string    *text_to_parse = NULL;
+	zval*         	z_parse_pos = NULL;
+	int32_t		parse_pos = -1;


I wouldn't bother aligning it

Suggested change

zend_string *text_to_parse = NULL;

zval* z_parse_pos = NULL;

int32_t parse_pos = -1;

zend_string *text_to_parse = NULL;

zval* z_parse_pos = NULL;

int32_t parse_pos = -1;

Girgias · 2024-04-25T21:23:53Z

ext/intl/dateformat/dateformat_parse.c

+			RETURN_FALSE;
+		}
+		parse_pos = (int32_t)long_parse_pos;
+		if(parse_pos != -1 && (size_t)parse_pos > ZSTR_LEN(text_to_parse)) {


Nit: CS

Suggested change

if(parse_pos != -1 && (size_t)parse_pos > ZSTR_LEN(text_to_parse)) {

if (parse_pos != -1 && (size_t)parse_pos > ZSTR_LEN(text_to_parse)) {

Girgias · 2024-04-25T21:24:46Z

ext/intl/dateformat/dateformat_parse.c

+		long_parse_pos = zval_try_get_long(z_parse_pos, &failed);
+		if (failed || ZEND_LONG_INT_OVFL(long_parse_pos)) {


If the parsing fails, I'm pretty sure it always throws an Error so having another intl specific extension being thrown seems odd? Also a test for this case?

kocsismate · 2024-04-28T20:37:11Z

ext/intl/dateformat/dateformat.stub.php


+    /**
+     * @param int $offset
+     * @tentative-return-type


no need for the tentative return type, as it's a brand new method :)

Girgias

Final comment, but LGTM otherwise

Girgias · 2024-04-29T13:20:54Z

ext/intl/dateformat/dateformat_parse.c

+		bool failed = false;
+		long_parse_pos = zval_try_get_long(z_parse_pos, &failed);
+		if (failed) {
+			zend_argument_type_error(2, "invalid offset");


Just to standardize the error message

Suggested change

zend_argument_type_error(2, "invalid offset");

zend_argument_type_error(2, "must be of type int, %s given", zend_zval_value_name(z_parse_pos));

New option to update its internal calendar.

iluuu1994 · 2024-04-30T09:25:57Z

@devnexen Please check this related failure: https://github.com/php/php-src/actions/runs/8887832558/job/24403750313

iluuu1994 · 2024-04-30T09:41:55Z

Nevermind, fixed here: 7f3fd30.

github-actions bot added the Extension: intl label Mar 21, 2024

jorgsowa reviewed Mar 21, 2024

View reviewed changes

ext/intl/dateformat/dateformat.stub.php Outdated Show resolved Hide resolved

devnexen force-pushed the intl_update_dateformat_parse branch 3 times, most recently from bacd94e to fbd4334 Compare March 21, 2024 23:16

devnexen marked this pull request as ready for review March 22, 2024 08:09

devnexen requested a review from kocsismate as a code owner March 22, 2024 08:09

kocsismate reviewed Mar 25, 2024

View reviewed changes

kocsismate requested a review from Girgias March 25, 2024 08:43

devnexen force-pushed the intl_update_dateformat_parse branch from fbd4334 to 4f3af87 Compare March 25, 2024 18:43

kocsismate reviewed Mar 25, 2024

View reviewed changes

devnexen force-pushed the intl_update_dateformat_parse branch from 4f3af87 to 10aeb6d Compare March 25, 2024 21:30

Girgias reviewed Apr 24, 2024

View reviewed changes

devnexen force-pushed the intl_update_dateformat_parse branch from 10aeb6d to f15e633 Compare April 25, 2024 06:26

Girgias reviewed Apr 25, 2024

View reviewed changes

devnexen force-pushed the intl_update_dateformat_parse branch from f15e633 to d69a34a Compare April 25, 2024 16:50

Girgias reviewed Apr 25, 2024

View reviewed changes

devnexen force-pushed the intl_update_dateformat_parse branch from d69a34a to bc58213 Compare April 25, 2024 21:40

kocsismate reviewed Apr 28, 2024

View reviewed changes

devnexen force-pushed the intl_update_dateformat_parse branch from bc58213 to 03d4443 Compare April 28, 2024 21:12

Girgias approved these changes Apr 29, 2024

View reviewed changes

devnexen added 3 commits April 29, 2024 18:15

ext/intl: IntlDateFormatter::parse adds new optional argument.

f54708b

New option to update its internal calendar.

alternative from feedback for comparison.

286533e

changes from feedback

ad4cfa9

devnexen force-pushed the intl_update_dateformat_parse branch from 03d4443 to ad4cfa9 Compare April 29, 2024 17:19

devnexen closed this in 1cf4cc3 Apr 29, 2024

	if( zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "Os\|z!",
	if (zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "Os\|z!",

	if((size_t)parse_pos > text_len) {
	if ((size_t)parse_pos > text_len) {

	internal_parse_to_timestamp( dfo, text_to_parse, text_len, z_parse_pos?&parse_pos:NULL, true, return_value);
	internal_parse_to_timestamp( dfo, text_to_parse, text_len, z_parse_pos ? &parse_pos : NULL, true, return_value);

	if(parse_pos != -1 && (size_t)parse_pos > ZSTR_LEN(text_to_parse)) {
	if (parse_pos != -1 && (size_t)parse_pos > ZSTR_LEN(text_to_parse)) {

		long_parse_pos = zval_try_get_long(z_parse_pos, &failed);
		if (failed \|\| ZEND_LONG_INT_OVFL(long_parse_pos)) {

	zend_argument_type_error(2, "invalid offset");
	zend_argument_type_error(2, "must be of type int, %s given", zend_zval_value_name(z_parse_pos));

Conversation

devnexen commented Mar 21, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kocsismate Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devnexen commented Apr 24, 2024

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iluuu1994 commented Apr 30, 2024

Uh oh!

iluuu1994 commented Apr 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kocsismate Mar 25, 2024 •

edited

Loading