Everything

php

BC broken for strrpos()/strripos()

php.internals

Andrey Hristov

22 years ago

Hi devs,
this night i saw that the BC of strrpos()/strripos()
is broken by a patch commited by pollita 7 months ago :
http://cvs.php.net/diff.php/php-src/ext/standard/string.c?login=2&r1=1.370&r2=1.371&ty=u
The documenation of both function states if the second parameter is an
integer instead of string then its value is used as a ord of the character
to be used during the search. This no more true with HEAD.
A script to reproduce :
<?php
var_dump(strrpos("ABCDEF","DEF"));
var_dump(strrpos("ABCDEF","DAF"));
var_dump(strrpos("ABCDEF",ord("D")));
?>
HEAD :
int(3)
bool(false)
bool(false)
PHP 4.3.5-dev (cli) (built: Dec  2 2003 00:13:37) (DEBUG):
int(3)
int(3)
int(3)
What do you think? Is it ok to break the BC here or not? As you see the
other change introduced with the patch is to use the whole needle string
while searching, not only the first character.

Comments are welcome,
Andrey

Sara Golemon

22 years ago

> this night i saw that the BC of strrpos()/strripos()
> is broken by a patch commited by pollita 7 months ago :
>

http://cvs.php.net/diff.php/php-src/ext/standard/string.c?login=2&r1=1.370&r2=1.371&ty=u

> The documenation of both function states if the second parameter is an
> integer instead of string then its value is used as a ord of the character
> to be used during the search. This no more true with HEAD.
>

Good catch.  This should not have been broken, I'll fix this.

> What do you think? Is it ok to break the BC here or not? As you see the
> other change introduced with the patch is to use the whole needle string
> while searching, not only the first character.
>

That break was intentional.  Having strrpos() behave differently from
strpos() in regards to needle size was not a good thing.

-Sara

Andrey Hristov

22 years ago

Sara Golemon wrote:

>>The documenation of both function states if the second parameter is an
>>integer instead of string then its value is used as a ord of the character
>>to be used during the search. This no more true with HEAD.
>>
>>
>>
>Good catch.  This should not have been broken, I'll fix this.
>
>

okie

>>What do you think? Is it ok to break the BC here or not? As you see the
>>other change introduced with the patch is to use the whole needle string
>>while searching, not only the first character.
>>
>>
>>
>That break was intentional.  Having strrpos() behave differently from
>strpos() in regards to needle size was not a good thing.
>
>

In this case it should be stated somewhere maybe in the still imaginery
"the_thin_differences_between_PHP4_and_PHP5.txt"

>-Sara
>
>
>

Andrey

Sara Golemon

22 years ago

> >>What do you think? Is it ok to break the BC here or not? As you see the
> >>other change introduced with the patch is to use the whole needle string
> >>while searching, not only the first character.
> >>
> >That break was intentional.  Having strrpos() behave differently from
> >strpos() in regards to needle size was not a good thing.
> >
> In this case it should be stated somewhere maybe in the still imaginery
> "the_thin_differences_between_PHP4_and_PHP5.txt"
>

Well, it's in the manual page (down at the bottom).  (Note: Ignore the note
about converting integers to strings, I've taken that out in light of
undoing that particular BC breakage).  But you're absolutely right, a spot
in a yet-to-be-writen FAQ couldn't hurt.


While I was in that file I fixed an offset reporting issue (offset didn't
exist in 4.3 so it's not something to MFH) and introduced an optimized
version of the string search for single character needles.  In strrpos()
it'll only be a minor difference, but strripos() saves a pair of needless
emallocs.

-Sara