Skip to content

Commit d97c788

Browse files
kocsismatekocsismate
committed
Address code review
1 parent b8137ca commit d97c788

File tree

6 files changed

+78
-62
lines changed

6 files changed

+78
-62
lines changed

‎ext/intl/converter/converter.stub.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ public static function getStandards() {}
4545
/** @return string|false|null */
4646
public function getSubstChars() {}
4747

48-
/** @return string|false */
48+
/** @return string */
4949
public static function reasonText(int $reason) {}
5050

5151
/** @return bool */

‎ext/intl/converter/converter_arginfo.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/* This is a generated file, edit the .stub.php file instead.
2-
* Stub hash: 9eef3fe293c07ab77f4c8b6d8d53a3798f8a9865 */
2+
* Stub hash: e33e2614c969c59b79c6062f7a347a8e8e486d85 */
33

44
ZEND_BEGIN_ARG_INFO_EX(arginfo_class_UConverter___construct, 0, 0, 0)
55
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, destination_encoding, IS_STRING, 1, "null")

‎ext/intl/grapheme/grapheme_string.c

Lines changed: 12 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,8 @@ PHP_FUNCTION(grapheme_strpos)
114114
}
115115

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

121121
/* we checked that it will fit: */
@@ -124,11 +124,6 @@ PHP_FUNCTION(grapheme_strpos)
124124

125125
/* the offset is 'grapheme count offset' so it still might be invalid - we'll check it later */
126126

127-
if (needle_len == 0) {
128-
zend_argument_value_error(2, "cannot be empty");
129-
RETURN_THROWS();
130-
}
131-
132127
if (offset >= 0) {
133128
/* quick check to see if the string might be there
134129
* I realize that 'offset' is 'grapheme count offset' but will work in spite of that
@@ -154,7 +149,6 @@ PHP_FUNCTION(grapheme_strpos)
154149
} else {
155150
RETURN_FALSE;
156151
}
157-
158152
}
159153
/* }}} */
160154

@@ -174,20 +168,15 @@ PHP_FUNCTION(grapheme_stripos)
174168
}
175169

176170
if ( OUTSIDE_STRING(loffset, haystack_len) ) {
177-
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_stripos: Offset not contained in string", 1 );
178-
RETURN_FALSE;
171+
zend_argument_value_error(3, "must be contained in argument #1 ($haystack)");
172+
RETURN_THROWS();
179173
}
180174

181175
/* we checked that it will fit: */
182176
offset = (int32_t) loffset;
183177

184178
/* the offset is 'grapheme count offset' so it still might be invalid - we'll check it later */
185179

186-
if (needle_len == 0) {
187-
zend_argument_value_error(2, "cannot be empty");
188-
RETURN_THROWS();
189-
}
190-
191180
is_ascii = ( grapheme_ascii_check((unsigned char*)haystack, haystack_len) >= 0 );
192181

193182
if ( is_ascii ) {
@@ -240,20 +229,15 @@ PHP_FUNCTION(grapheme_strrpos)
240229
}
241230

242231
if ( OUTSIDE_STRING(loffset, haystack_len) ) {
243-
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_strpos: Offset not contained in string", 1 );
244-
RETURN_FALSE;
232+
zend_argument_value_error(3, "must be contained in argument #1 ($haystack)");
233+
RETURN_THROWS();
245234
}
246235

247236
/* we checked that it will fit: */
248237
offset = (int32_t) loffset;
249238

250239
/* the offset is 'grapheme count offset' so it still might be invalid - we'll check it later */
251240

252-
if (needle_len == 0) {
253-
zend_argument_value_error(2, "cannot be empty");
254-
RETURN_FALSE;
255-
}
256-
257241
is_ascii = grapheme_ascii_check((unsigned char *)haystack, haystack_len) >= 0;
258242

259243
if ( is_ascii ) {
@@ -300,20 +284,15 @@ PHP_FUNCTION(grapheme_strripos)
300284
}
301285

302286
if ( OUTSIDE_STRING(loffset, haystack_len) ) {
303-
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_strpos: Offset not contained in string", 1 );
304-
RETURN_FALSE;
287+
zend_argument_value_error(3, "must be contained in argument #1 ($haystack)");
288+
RETURN_THROWS();
305289
}
306290

307291
/* we checked that it will fit: */
308292
offset = (int32_t) loffset;
309293

310294
/* the offset is 'grapheme count offset' so it still might be invalid - we'll check it later */
311295

312-
if (needle_len == 0) {
313-
zend_argument_value_error(2, "cannot be empty");
314-
RETURN_THROWS();
315-
}
316-
317296
is_ascii = grapheme_ascii_check((unsigned char *)haystack, haystack_len) >= 0;
318297

319298
if ( is_ascii ) {
@@ -377,8 +356,8 @@ PHP_FUNCTION(grapheme_substr)
377356
}
378357

379358
if ( OUTSIDE_STRING(lstart, str_len)) {
380-
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_substr: start not contained in string", 1 );
381-
RETURN_FALSE;
359+
zend_argument_value_error(2, "must be contained in argument #1 ($string)");
360+
RETURN_THROWS();
382361
}
383362

384363
/* we checked that it will fit: */
@@ -532,10 +511,9 @@ PHP_FUNCTION(grapheme_substr)
532511

533512
if ( UBRK_DONE == sub_str_end_pos) {
534513
if(length < 0) {
535-
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "grapheme_substr: length not contained in string", 1 );
536-
514+
zend_argument_value_error(3, "must be contained in argument #1 ($string)");
537515
efree(ustr);
538-
RETURN_FALSE;
516+
RETURN_THROWS();
539517
} else {
540518
sub_str_end_pos = ustr_len;
541519
}
@@ -581,12 +559,6 @@ static void strstr_common_handler(INTERNAL_FUNCTION_PARAMETERS, int f_ignore_cas
581559
RETURN_THROWS();
582560
}
583561

584-
if (needle_len == 0) {
585-
zend_argument_value_error(2, "cannot be empty");
586-
RETURN_THROWS();
587-
}
588-
589-
590562
if ( !f_ignore_case ) {
591563

592564
/* ASCII optimization: quick check to see if the string might be there

‎ext/intl/tests/bug61487.phpt

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,18 @@ if (PHP_INT_SIZE != 8) die('skip 64-bit only');
66
?>
77
--FILE--
88
<?php
9-
var_dump(grapheme_stripos(1,1,2147483648));
10-
var_dump(grapheme_strpos(1,1,2147483648));
9+
try {
10+
grapheme_stripos(1,1,2147483648);
11+
} catch (ValueError $exception) {
12+
echo $exception->getMessage() . "\n";
13+
}
14+
15+
try {
16+
grapheme_strpos(1,1,2147483648);
17+
} catch (ValueError $exception) {
18+
echo $exception->getMessage() . "\n";
19+
}
1120
?>
1221
--EXPECT--
13-
bool(false)
14-
bool(false)
22+
grapheme_stripos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
23+
grapheme_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)

‎ext/intl/tests/grapheme2.phpt

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -397,23 +397,27 @@ function ut_main()
397397
);
398398

399399
foreach( $tests as $test ) {
400-
$arg0 = urlencode($test[0]);
401-
$res_str .= "substring of \"$arg0\" from \"$test[1]\" - grapheme_substr";
402-
if ( 3 == count( $test ) ) {
403-
$result = grapheme_substr($test[0], $test[1]);
404-
}
405-
else {
406-
$res_str .= " with length $test[2]";
407-
$result = grapheme_substr($test[0], $test[1], $test[2]);
408-
}
409-
$res_str .= " = ";
410-
if ( $result === false ) {
411-
$res_str .= 'false';
412-
}
413-
else {
414-
$res_str .= urlencode($result);
400+
try {
401+
$arg0 = urlencode($test[0]);
402+
$res_str .= "substring of \"$arg0\" from \"$test[1]\" - grapheme_substr";
403+
if ( 3 == count( $test ) ) {
404+
$result = grapheme_substr($test[0], $test[1]);
405+
}
406+
else {
407+
$res_str .= " with length $test[2]";
408+
$result = grapheme_substr($test[0], $test[1], $test[2]);
409+
}
410+
$res_str .= " = ";
411+
if ( $result === false ) {
412+
$res_str .= 'false';
413+
}
414+
else {
415+
$res_str .= urlencode($result);
416+
}
417+
$res_str .= " == " . urlencode($test[count($test)-1]) . check_result($result, $test[count($test)-1]) . "\n";
418+
} catch (ValueError $exception) {
419+
echo $exception->getMessage() . "\n";
415420
}
416-
$res_str .= " == " . urlencode($test[count($test)-1]) . check_result($result, $test[count($test)-1]) . "\n";
417421
}
418422

419423

@@ -782,6 +786,15 @@ function check_result($result, $expected) {
782786

783787
?>
784788
--EXPECT--
789+
grapheme_substr(): Argument #2 ($start) must be contained in argument #1 ($string)
790+
grapheme_substr(): Argument #2 ($start) must be contained in argument #1 ($string)
791+
grapheme_substr(): Argument #2 ($start) must be contained in argument #1 ($string)
792+
grapheme_substr(): Argument #2 ($start) must be contained in argument #1 ($string)
793+
grapheme_substr(): Argument #3 ($length) must be contained in argument #1 ($string)
794+
grapheme_substr(): Argument #2 ($start) must be contained in argument #1 ($string)
795+
grapheme_substr(): Argument #3 ($length) must be contained in argument #1 ($string)
796+
grapheme_substr(): Argument #3 ($length) must be contained in argument #1 ($string)
797+
785798
function grapheme_strlen($string) {}
786799

787800
"hindi" in devanagari strlen 2

‎ext/intl/tests/grapheme_empty.phpt

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
--TEST--
2+
Test grapheme_strpos-alike functions with empty needle
3+
--SKIPIF--
4+
<?php if( !extension_loaded( 'intl' ) ) print 'skip'; ?>
5+
--FILE--
6+
<?php
7+
8+
var_dump(grapheme_strpos("abc", ""));
9+
var_dump(grapheme_stripos("abc", ""));
10+
var_dump(grapheme_strrpos("abc", ""));
11+
var_dump(grapheme_strripos("abc", ""));
12+
var_dump(grapheme_strstr("abc", ""));
13+
var_dump(grapheme_stristr("abc", ""));
14+
15+
?>
16+
--EXPECT--
17+
int(0)
18+
int(0)
19+
int(3)
20+
int(3)
21+
string(3) "abc"
22+
bool(false)

0 commit comments

Comments
 (0)