Uh oh!
There was an error while loading. Please reload this page.
Promote warnings to exceptions in ext/intl - #5972
Conversation
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Yes, we should add support for empty string here.
eb894c5 to
d97c788CompareThere 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
d97c788 to
87f4698CompareUh oh!
There was an error while loading. Please reload this page.
nikic
commented
Sep 7, 2020
Travis still failing. |
kocsismate
commented
Sep 7, 2020
@nikic Yeah, I'll fix it too. It didn't run on my machine due to incompatible icu version. |
5af4591 to
fec3f5eComparefec3f5e to
e1013ceCompare
There is quite a few
intl_error_set()invocations remaining, out of which probably a few could also be promoted to an exception.