Skip to content

Promote warnings to exceptions in ext/intl #5972

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

Closed
wants to merge 5 commits into from
Closed

Promote warnings to exceptions in ext/intl #5972

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f296aad
Promote warnings to exceptions in ext/intl
kocsismate Aug 10, 2020
21d4f76
Address code review
kocsismate Aug 11, 2020
87f4698
Revert support for empty grapheme needles
kocsismate Sep 7, 2020
e8fd07a
Fix test
kocsismate Sep 7, 2020
e1013ce
Try to fix test
kocsismate Sep 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ext/intl/converter/converter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,8 +693,8 @@ PHP_METHOD(UConverter, reasonText) {
UCNV_REASON_CASE(CLOSE)
UCNV_REASON_CASE(CLONE)
default:
php_error_docref(NULL, E_WARNING, "Unknown UConverterCallbackReason: " ZEND_LONG_FMT, reason);
RETURN_FALSE;
zend_argument_value_error(1, "must be a UConverter::REASON_* constant");
RETURN_THROWS();
}
}
/* }}} */
Expand Down
2 changes: 1 addition & 1 deletion ext/intl/converter/converter.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public static function getStandards() {}
/** @return string|false|null */
public function getSubstChars() {}

/** @return string|false */
/** @return string */
public static function reasonText(int $reason) {}

/** @return bool */
Expand Down
2 changes: 1 addition & 1 deletion ext/intl/converter/converter_arginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* This is a generated file, edit the .stub.php file instead.
* Stub hash: 9eef3fe293c07ab77f4c8b6d8d53a3798f8a9865 */
* Stub hash: e33e2614c969c59b79c6062f7a347a8e8e486d85 */

ZEND_BEGIN_ARG_INFO_EX(arginfo_class_UConverter___construct, 0, 0, 0)
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, destination_encoding, IS_STRING, 1, "null")
Expand Down
5 changes: 2 additions & 3 deletions ext/intl/formatter/formatter_format.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,8 @@ PHP_FUNCTION( numfmt_format )
break;

default:
php_error_docref(NULL, E_WARNING, "Unsupported format type " ZEND_LONG_FMT, type);
RETURN_FALSE;
break;
zend_argument_value_error(3, "must be a NumberFormatter::TYPE_* constant");
RETURN_THROWS();
}

INTL_METHOD_RETVAL_UTF8( nfo, formatted, formatted_len, ( formatted != format_buf ) );
Expand Down
19 changes: 11 additions & 8 deletions ext/intl/formatter/formatter_parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,13 @@ PHP_FUNCTION( numfmt_parse )
FORMATTER_METHOD_INIT_VARS;

/* Parse parameters. */
if( zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "Os|lz!",
if (zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "Os|lz!",
&object, NumberFormatter_ce_ptr, &str, &str_len, &type, &zposition ) == FAILURE )
{
RETURN_THROWS();
}

if(zposition) {
if (zposition) {
position = (int32_t) zval_get_long(zposition);
position_p = &position;
}
Expand Down Expand Up @@ -86,17 +86,20 @@ PHP_FUNCTION( numfmt_parse )
RETVAL_DOUBLE(val_double);
break;
default:
php_error_docref(NULL, E_WARNING, "Unsupported format type " ZEND_LONG_FMT, type);
RETVAL_FALSE;
break;
zend_argument_value_error(3, "must be a NumberFormatter::TYPE_* constant");
goto cleanup;
}

if (zposition) {
ZEND_TRY_ASSIGN_REF_LONG(zposition, position);
}

cleanup:

#if ICU_LOCALE_BUG && defined(LC_NUMERIC)
setlocale(LC_NUMERIC, oldlocale);
efree(oldlocale);
#endif
if(zposition) {
ZEND_TRY_ASSIGN_REF_LONG(zposition, position);
}

if (sstr) {
efree(sstr);
Expand Down
66 changes: 33 additions & 33 deletions ext/intl/grapheme/grapheme_string.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ PHP_FUNCTION(grapheme_strpos)
}

if ( OUTSIDE_STRING(loffset, haystack_len) ) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_strpos: Offset not contained in string", 1 );
RETURN_FALSE;
zend_argument_value_error(3, "must be contained in argument #1 ($haystack)");
RETURN_THROWS();
}

/* we checked that it will fit: */
Expand All @@ -125,8 +125,8 @@ PHP_FUNCTION(grapheme_strpos)
/* the offset is 'grapheme count offset' so it still might be invalid - we'll check it later */

if (needle_len == 0) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_strpos: Empty delimiter", 1 );
RETURN_FALSE;
zend_argument_value_error(2, "cannot be empty");
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, there is no such warning in strpos(), so maybe we could get rid of this error? On the other hand, the offset is not contained... case has been promoted to an exception in strpos(), so I'll do it here as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should add support for empty string here.

RETURN_THROWS();
}

if (offset >= 0) {
Expand Down Expand Up @@ -154,7 +154,6 @@ PHP_FUNCTION(grapheme_strpos)
} else {
RETURN_FALSE;
}

}
/* }}} */

Expand All @@ -174,8 +173,8 @@ PHP_FUNCTION(grapheme_stripos)
}

if ( OUTSIDE_STRING(loffset, haystack_len) ) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_stripos: Offset not contained in string", 1 );
RETURN_FALSE;
zend_argument_value_error(3, "must be contained in argument #1 ($haystack)");
RETURN_THROWS();
}

/* we checked that it will fit: */
Expand All @@ -184,8 +183,8 @@ PHP_FUNCTION(grapheme_stripos)
/* the offset is 'grapheme count offset' so it still might be invalid - we'll check it later */

if (needle_len == 0) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_stripos: Empty delimiter", 1 );
RETURN_FALSE;
zend_argument_value_error(2, "cannot be empty");
RETURN_THROWS();
}

is_ascii = ( grapheme_ascii_check((unsigned char*)haystack, haystack_len) >= 0 );
Expand Down Expand Up @@ -240,8 +239,8 @@ PHP_FUNCTION(grapheme_strrpos)
}

if ( OUTSIDE_STRING(loffset, haystack_len) ) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_strpos: Offset not contained in string", 1 );
RETURN_FALSE;
zend_argument_value_error(3, "must be contained in argument #1 ($haystack)");
RETURN_THROWS();
}

/* we checked that it will fit: */
Expand All @@ -250,8 +249,8 @@ PHP_FUNCTION(grapheme_strrpos)
/* the offset is 'grapheme count offset' so it still might be invalid - we'll check it later */

if (needle_len == 0) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_strpos: Empty delimiter", 1 );
RETURN_FALSE;
zend_argument_value_error(2, "cannot be empty");
RETURN_THROWS();
}

is_ascii = grapheme_ascii_check((unsigned char *)haystack, haystack_len) >= 0;
Expand Down Expand Up @@ -300,8 +299,8 @@ PHP_FUNCTION(grapheme_strripos)
}

if ( OUTSIDE_STRING(loffset, haystack_len) ) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_strpos: Offset not contained in string", 1 );
RETURN_FALSE;
zend_argument_value_error(3, "must be contained in argument #1 ($haystack)");
RETURN_THROWS();
}

/* we checked that it will fit: */
Expand All @@ -310,8 +309,8 @@ PHP_FUNCTION(grapheme_strripos)
/* the offset is 'grapheme count offset' so it still might be invalid - we'll check it later */

if (needle_len == 0) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_strpos: Empty delimiter", 1 );
RETURN_FALSE;
zend_argument_value_error(2, "cannot be empty");
RETURN_THROWS();
}

is_ascii = grapheme_ascii_check((unsigned char *)haystack, haystack_len) >= 0;
Expand Down Expand Up @@ -377,8 +376,8 @@ PHP_FUNCTION(grapheme_substr)
}

if ( OUTSIDE_STRING(lstart, str_len)) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_substr: start not contained in string", 1 );
RETURN_FALSE;
zend_argument_value_error(2, "must be contained in argument #1 ($string)");
RETURN_THROWS();
}

/* we checked that it will fit: */
Expand Down Expand Up @@ -532,10 +531,9 @@ PHP_FUNCTION(grapheme_substr)

if ( UBRK_DONE == sub_str_end_pos) {
if(length < 0) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_substr: length not contained in string", 1 );

zend_argument_value_error(3, "must be contained in argument #1 ($string)");
efree(ustr);
RETURN_FALSE;
RETURN_THROWS();
} else {
sub_str_end_pos = ustr_len;
}
Expand Down Expand Up @@ -582,13 +580,10 @@ static void strstr_common_handler(INTERNAL_FUNCTION_PARAMETERS, int f_ignore_cas
}

if (needle_len == 0) {

intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_strpos: Empty delimiter", 1 );

RETURN_FALSE;
zend_argument_value_error(2, "cannot be empty");
RETURN_THROWS();
}


if ( !f_ignore_case ) {

/* ASCII optimization: quick check to see if the string might be there
Expand Down Expand Up @@ -783,20 +778,25 @@ PHP_FUNCTION(grapheme_extract)
}

if ( extract_type < GRAPHEME_EXTRACT_TYPE_MIN || extract_type > GRAPHEME_EXTRACT_TYPE_MAX ) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR,
"grapheme_extract: unknown extract type param", 0 );
RETURN_FALSE;
zend_argument_value_error(3, "must be either GRAPHEME_EXTR_COUNT, GRAPHEME_EXTR_MAXBYTES, or GRAPHEME_EXTR_MAXCHARS");
RETURN_THROWS();
}

if ( lstart > INT32_MAX || lstart < 0 || (size_t)lstart >= str_len ) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_extract: start not contained in string", 0 );
RETURN_FALSE;
}

if ( size > INT32_MAX || size < 0) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_extract: size is invalid", 0 );
RETURN_FALSE;
if (size < 0) {
zend_argument_value_error(2, "must be greater than or equal to 0");
RETURN_THROWS();
}

if ( size > INT32_MAX) {
zend_argument_value_error(2, "is too large");
RETURN_THROWS();
}

if (size == 0) {
RETURN_EMPTY_STRING();
}
Expand Down
7 changes: 3 additions & 4 deletions ext/intl/locale/locale_methods.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -882,10 +882,9 @@ PHP_FUNCTION(locale_compose)
/* Not grandfathered */
result = append_key_value(loc_name, hash_arr , LOC_LANG_TAG);
if( result == LOC_NOT_FOUND ){
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR,
"locale_compose: parameter array does not contain 'language' tag.", 0 );
zend_argument_value_error(1, "must contain a \"%s\" key", LOC_LANG_TAG);
smart_str_free(loc_name);
RETURN_FALSE;
RETURN_THROWS();
}
if( !handleAppendResult( result, loc_name)){
RETURN_FALSE;
Expand Down Expand Up @@ -1348,7 +1347,7 @@ static zend_string* lookup_loc_range(const char* loc_range, HashTable* hash_arr,
/* convert the array to lowercase , also replace hyphens with the underscore and store it in cur_arr */
if(Z_TYPE_P(ele_value)!= IS_STRING) {
/* element value is not a string */
intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR, "lookup_loc_range: locale array element is not a string", 0);
zend_argument_type_error(2, "must only contain string values");
LOOKUP_CLEAN_RETURN(NULL);
}
cur_arr[cur_arr_len*2] = estrndup(Z_STRVAL_P(ele_value), Z_STRLEN_P(ele_value));
Expand Down
10 changes: 4 additions & 6 deletions ext/intl/normalizer/normalizer_normalize.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,8 @@ PHP_FUNCTION( normalizer_normalize )
#endif
break;
default:
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR,
"normalizer_normalize: illegal normalization form", 0 );
RETURN_FALSE;
zend_argument_value_error(2, "must be a a valid normalization form");
RETURN_THROWS();
}

/*
Expand Down Expand Up @@ -248,9 +247,8 @@ PHP_FUNCTION( normalizer_is_normalized )
#endif
break;
default:
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR,
"normalizer_normalize: illegal normalization form", 0 );
RETURN_FALSE;
zend_argument_value_error(2, "must be a a valid normalization form");
RETURN_THROWS();
}


Expand Down
8 changes: 3 additions & 5 deletions ext/intl/resourcebundle/resourcebundle_class.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,7 @@ static int resourcebundle_ctor(INTERNAL_FUNCTION_PARAMETERS)
}

if (bundlename_len >= MAXPATHLEN) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "Bundle name too long", 0 );
zval_ptr_dtor(return_value);
ZVAL_NULL(return_value);
zend_argument_value_error(2, "is too long");
return FAILURE;
}

Expand Down Expand Up @@ -296,8 +294,8 @@ PHP_FUNCTION( resourcebundle_locales )
}

if (bundlename_len >= MAXPATHLEN) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "resourcebundle_locales: bundle name too long", 0 );
RETURN_FALSE;
zend_argument_value_error(1, "is too long");
RETURN_THROWS();
}

if(bundlename_len == 0) {
Expand Down
17 changes: 13 additions & 4 deletions ext/intl/tests/bug61487.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,18 @@ if (PHP_INT_SIZE != 8) die('skip 64-bit only');
?>
--FILE--
<?php
var_dump(grapheme_stripos(1,1,2147483648));
var_dump(grapheme_strpos(1,1,2147483648));
try {
grapheme_stripos(1,1,2147483648);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

try {
grapheme_strpos(1,1,2147483648);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}
?>
--EXPECT--
bool(false)
bool(false)
grapheme_stripos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
grapheme_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
8 changes: 6 additions & 2 deletions ext/intl/tests/bug62083.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ if (!extension_loaded('intl'))
--FILE--
<?php
$arr1 = array();
var_dump(grapheme_extract(-1, -1, -1,-1, $arr1));
try {
grapheme_extract(-1, -1, -1,-1, $arr1);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}
?>
--EXPECT--
bool(false)
grapheme_extract(): Argument #3 ($extract_type) must be either GRAPHEME_EXTR_COUNT, GRAPHEME_EXTR_MAXBYTES, or GRAPHEME_EXTR_MAXCHARS
24 changes: 20 additions & 4 deletions ext/intl/tests/grapheme.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,11 +400,19 @@ function ut_main()
$arg0 = urlencode($test[0]);
$res_str .= "substring of \"$arg0\" from \"$test[1]\" - grapheme_substr";
if ( 3 == count( $test ) ) {
$result = grapheme_substr($test[0], $test[1]);
try {
$result = grapheme_substr($test[0], $test[1]);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}
}
else {
$res_str .= " with length $test[2]";
$result = grapheme_substr($test[0], $test[1], $test[2]);
try {
$result = grapheme_substr($test[0], $test[1], $test[2]);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}
}
$res_str .= " = ";
if ( $result === false ) {
Expand Down Expand Up @@ -465,11 +473,19 @@ function ut_main()
$arg0 = urlencode($test[0]);
$res_str .= "find \"$arg1\" in \"$arg0\" - grapheme_strstr";
if ( 3 == count( $test ) ) {
$result = grapheme_strstr($test[0], $test[1]);
try {
$result = grapheme_strstr($test[0], $test[1]);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}
}
else {
$res_str .= " before flag is " . ( $test[2] ? "TRUE" : "FALSE" );
$result = grapheme_strstr($test[0], $test[1], $test[2]);
try {
$result = grapheme_strstr($test[0], $test[1], $test[2]);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}
}
$res_str .= " = ";
if ( $result === false ) {
Expand Down
Loading