Promote warnings to exceptions in ext/intl #5972
Conversation
kocsismate
force-pushed
the
intl-warning
branch
from
1c9f0ca to
eb894c5
Compare
| @@ -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"); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, we should add support for empty string here.
| @@ -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"); | |||
There was a problem hiding this comment.
Yes, we should add support for empty string here.
kocsismate
force-pushed
the
intl-warning
branch
from
eb894c5 to
d97c788
Compare
ext/intl/tests/grapheme_empty.phpt
Outdated
| int(3) | ||
| int(3) | ||
| string(3) "abc" | ||
| bool(false) |
There was a problem hiding this comment.
Shouldn't this one also be string(3) "abc"?
There was a problem hiding this comment.
Ah, yes.. I thought that the removed check from strstr_common_handler() was inside if ( !f_ignore_case ) {}, thus it only applies to grapheme_strstr. And when I double-checked it on 3v4n, the intl.error_level tricked me, since there is no error shown by default.
That said, I'll try to fix this.
There was a problem hiding this comment.
The reason of the false return value is this check in grapheme_strpos_utf16():
src = usearch_open(uneedle, uneedle_len, uhaystack, uhaystack_len, "", bi, &status);
STRPOS_CHECK_STATUS(status, "Error creating search object");
To make things worse, grapheme_strpos_utf16() is called in the other grapheme string functions when the offset is below 0, so e.g. grapheme_strpos("abc", "", -1) also fails currently.
I don't see any easy solution for the problem because usearch_open() is third-party code, otherwise we could just add a similar check for empty needle what George added to zend_memnstr(). @nikic Do you know about any good solution that circumvents this problem? Is it a very big problem if grapheme_() functions can't handle empty needles?
There was a problem hiding this comment.
Looks like the relevant icu code is: https://github.com/unicode-org/icu/blob/f744742e036441cbfd54b535e722dc3801ff0f63/icu4c/source/i18n/usearch.cpp#L2675
I think we should add an explicit check for this case then. I do think it would be good to support empty strings here, as we also added support for them in ext/standard and ext/mbstring (I'm not sure about ext/iconv right now).
There was a problem hiding this comment.
I've reverted the change in question, but added a task
kocsismate
force-pushed
the
intl-warning
branch
from
d97c788 to
87f4698
Compare
|
Travis still failing. |
|
@nikic Yeah, I'll fix it too. It didn't run on my machine due to incompatible icu version. |
kocsismate
force-pushed
the
intl-warning
branch
from
5af4591 to
fec3f5e
Compare
kocsismate
force-pushed
the
intl-warning
branch
from
fec3f5e to
e1013ce
Compare
There is quite a few
intl_error_set()invocations remaining, out of which probably a few could also be promoted to an exception.