ext/intl: SpoofChecker::setAllowedChars support. #14433

devnexen · 2024-06-02T11:49:26Z

To limit the acceptable range of acceptable unicode chars via individual ones or via a pattern.

ext/intl/spoofchecker/spoofchecker_main.c

kocsismate · 2024-06-21T21:21:53Z

ext/intl/spoofchecker/spoofchecker.stub.php

    /** @tentative-return-type */
    public function setRestrictionLevel(int $level): void {}
 #endif
+    public function setAllowedChars(string $pattern, int $pattern_options = 0): void {}


The parameter naming style should follow the extension's naming style, which is camelCase.

Suggested change

public function setAllowedChars(string $pattern, int $pattern_options = 0): void {}

public function setAllowedChars(string $pattern, int $patternOptions = 0): void {}

kocsismate · 2024-06-21T21:25:26Z

ext/intl/spoofchecker/spoofchecker_main.c

+	SPOOFCHECKER_METHOD_FETCH_OBJECT;
+
+	if (ZSTR_LEN(pattern) > INT32_MAX) {
+		zend_argument_value_error(1, "is larger than " ZEND_LONG_FMT, INT32_MAX);


This is the most common wording:

Suggested change

zend_argument_value_error(1, "is larger than " ZEND_LONG_FMT, INT32_MAX);

zend_argument_value_error(1, "must be less than or equal to " ZEND_LONG_FMT " bytes", INT32_MAX);

kocsismate · 2024-06-21T21:26:26Z

ext/intl/spoofchecker/spoofchecker_main.c

+
+	/* uset_applyPattern requires to start with a regex range char */
+	if (ZSTR_VAL(pattern)[0] != '[') {
+		zend_argument_value_error(1, "invalid pattern");


The sentence becomes correct this way:

Suggested change

zend_argument_value_error(1, "invalid pattern");

zend_argument_value_error(1, "is an invalid pattern");

kocsismate · 2024-06-21T21:26:44Z

ext/intl/spoofchecker/spoofchecker_main.c

+
+	intl_convert_utf8_to_utf16(&upattern, &upattern_len, ZSTR_VAL(pattern), ZSTR_LEN(pattern), SPOOFCHECKER_ERROR_CODE_P(co));
+	if (U_FAILURE(SPOOFCHECKER_ERROR_CODE(co))) {
+		zend_argument_value_error(1, "string conversion to utf-16 failed (%d) %s", SPOOFCHECKER_ERROR_CODE(co), u_errorName(SPOOFCHECKER_ERROR_CODE(co)));


this error message should also be improved

the same goes for the rest as well

nielsdos

Looks almost good

ext/intl/spoofchecker/spoofchecker_main.c

nielsdos · 2024-07-22T15:23:23Z

ext/intl/spoofchecker/spoofchecker_main.c

+	efree(upattern);
+
+	if (U_FAILURE(SPOOFCHECKER_ERROR_CODE(co))) {
+		php_error_docref(NULL, E_WARNING, "(%d) %s", SPOOFCHECKER_ERROR_CODE(co), u_errorName(SPOOFCHECKER_ERROR_CODE(co)));


A bit unfortunate of an API design, but at least consistent with what we have, so 🤷

ext/intl/spoofchecker/spoofchecker_main.c

nielsdos · 2024-07-22T17:49:57Z

ext/intl/spoofchecker/spoofchecker_main.c

+
+	/* uset_applyPattern requires to start with a regex range char */
+	if (ZSTR_VAL(pattern)[0] != '[' || ZSTR_VAL(pattern)[ZSTR_LEN(pattern) -1] != ']') {
+		zend_argument_value_error(1, "must be an invalid pattern");


Suggested change

zend_argument_value_error(1, "must be an invalid pattern");

zend_argument_value_error(1, "must be a valid pattern");

Or perhaps we should even hint in the error message that this should be a regex character range.

I also think that the "is a" vs "must be" error messages should be done consistently in this function.

nielsdos · 2024-07-22T18:11:52Z

ext/intl/spoofchecker/spoofchecker_main.c

+
+	uset_applyPattern(set, upattern, upattern_len, (uint32_t)pattern_option, SPOOFCHECKER_ERROR_CODE_P(co));
+	if (U_FAILURE(SPOOFCHECKER_ERROR_CODE(co))) {
+		zend_argument_value_error(1, "must be a invalid pattern (%d) %s", SPOOFCHECKER_ERROR_CODE(co), u_errorName(SPOOFCHECKER_ERROR_CODE(co)));


Suggested change

zend_argument_value_error(1, "must be a invalid pattern (%d) %s", SPOOFCHECKER_ERROR_CODE(co), u_errorName(SPOOFCHECKER_ERROR_CODE(co)));

zend_argument_value_error(1, "must be a valid regular expression character set pattern (%d) %s", SPOOFCHECKER_ERROR_CODE(co), u_errorName(SPOOFCHECKER_ERROR_CODE(co)));

nielsdos · 2024-07-22T18:12:37Z

ext/intl/tests/spoofchecker_008.phpt

+bool(true)
+bool(false)
+bool(false)
+Spoofchecker::setAllowedChars(): Argument #2 ($patternOptions) must be a valid pattern option, 0 or (SpoofChecker::IGNORE_SPACE|(<none> or SpoofChecker::USET_CASE_INSENSITIVE or SpoofChecker::USET_ADD_CASE_MAPPINGS or SpoofChecker::USET_SIMPLE_CASE_INSENSITIVE))


You lost the %s thingie here

nielsdos · 2024-07-22T18:13:14Z

ext/intl/spoofchecker/spoofchecker_main.c

+
+	/* uset_applyPattern requires to start with a regex range char */
+	if (ZSTR_VAL(pattern)[0] != '[' || ZSTR_VAL(pattern)[ZSTR_LEN(pattern) -1] != ']') {
+		zend_argument_value_error(1, "must be a valid pattern");


Suggested change

zend_argument_value_error(1, "must be a valid pattern");

zend_argument_value_error(1, "must be a valid regular expression character set pattern");

To limit the acceptable range of acceptable unicode chars via individual ones or via a pattern.

nielsdos

LGTM if CI is green.
I pulled this locally and played a bit with it, and I think it seems good.
Sorry it took so long for the final review, I thought someone else was gonna review it and then forgot that I initially commented on this.

devnexen · 2024-07-22T18:45:04Z

Thanks for your review

devnexen · 2024-07-22T19:10:41Z

Merge with 21418b5

github-actions bot added the Extension: intl label Jun 2, 2024

nielsdos reviewed Jun 2, 2024

View reviewed changes

ext/intl/spoofchecker/spoofchecker_main.c Outdated Show resolved Hide resolved

devnexen force-pushed the spoofchecker_update branch 2 times, most recently from 7f8885f to 4c4605f Compare June 2, 2024 13:14

TimWolla changed the title ~~ext/intl: SpooChecker::setAllowedChars support.~~ ext/intl: SpoofChecker::setAllowedChars support. Jun 2, 2024

devnexen force-pushed the spoofchecker_update branch 5 times, most recently from 9d25674 to b50fc05 Compare June 2, 2024 17:57

devnexen marked this pull request as ready for review June 2, 2024 19:57

devnexen requested a review from kocsismate as a code owner June 2, 2024 19:57

kocsismate reviewed Jun 21, 2024

View reviewed changes

devnexen force-pushed the spoofchecker_update branch from b50fc05 to c2815dc Compare June 21, 2024 22:08

nielsdos reviewed Jul 22, 2024

View reviewed changes

devnexen force-pushed the spoofchecker_update branch from c2815dc to 7b8ce1c Compare July 22, 2024 17:16

nielsdos reviewed Jul 22, 2024

View reviewed changes

devnexen force-pushed the spoofchecker_update branch from 7b8ce1c to 9f840a9 Compare July 22, 2024 18:06

nielsdos reviewed Jul 22, 2024

View reviewed changes

ext/intl: SpoofChecker::setAllowedChars support.

b29b569

To limit the acceptable range of acceptable unicode chars via individual ones or via a pattern.

devnexen force-pushed the spoofchecker_update branch from 9f840a9 to b29b569 Compare July 22, 2024 18:22

nielsdos approved these changes Jul 22, 2024

View reviewed changes

devnexen closed this Jul 22, 2024

	public function setAllowedChars(string $pattern, int $pattern_options = 0): void {}
	public function setAllowedChars(string $pattern, int $patternOptions = 0): void {}

	zend_argument_value_error(1, "is larger than " ZEND_LONG_FMT, INT32_MAX);
	zend_argument_value_error(1, "must be less than or equal to " ZEND_LONG_FMT " bytes", INT32_MAX);

	zend_argument_value_error(1, "invalid pattern");
	zend_argument_value_error(1, "is an invalid pattern");

	zend_argument_value_error(1, "must be an invalid pattern");
	zend_argument_value_error(1, "must be a valid pattern");

Uh oh!

ext/intl: SpoofChecker::setAllowedChars support. #14433

ext/intl: SpoofChecker::setAllowedChars support. #14433

Uh oh!

Conversation

devnexen commented Jun 2, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

devnexen commented Jul 22, 2024

Uh oh!

devnexen commented Jul 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants