Fwd: Re: Fix for issue #72320 - compatibility breakage in 7.0.11

I'm sending the mail again, because it has been rejected due "Spammy URLs in your message" – I've removed the links to svn.php.net now. On 22.12.2016 at 16:13, Zeev Suraski wrote: > All, > > In 7.0.11, the behavior of iconv_substr() was changed so that if the length argument is equal > to the length of the string - an empty string is returned, as opposed to FALSE. > > While I think there's a case to be made that this makes sense - what worries me is that > we've introduced a behavioral change for a function in a bugfix release - one that has bitten > us in Zend Framework - and I think it's safe to assume that it will affect plenty of other > codebases. > > Personally, I think we should be *much* more selective about introducing behavioral changes in > maintenance releases, even if they can be considered bugfixes. Depending on their scope and impact, > they should either go into the next mini version (this one probably falls in that category) or the > next major version. Of course - there are plenty of bugfixes where the existing behavior is a > crash, or undefined/unpredictable - those are obviously fine to fix without further discussion. > I'm talking about the cases where the current behavior is very much defined and has been that > way for a long time, but for whatever reason, people think it should change. Well, in this case the behavior differed from the documentation. The documentation read[1]: | If <parameter>str</parameter> is shorter than | <parameter>offset</parameter> characters long, &false; will be | returned. That is analogous to the wording in the substr() documentation[2] after bug #62922 had been fixed: | If <parameter>string</parameter> is less than | <parameter>start</parameter> characters long, &false; will be | returned. Considering that mb_substr() worked also this way (at least as of PHP 5.0.0), I concluded that also changing the behavior of iconv_substr() had simply been overlooked for PHP 7.0.0, and so it seemed appropriate to change iconv_substr() in the PHP-7.0 branch. Sorry, if my judgement was wrong! :) > We've worked hard to establish trust that people can upgrade into the next maintenance > version with minimal testing - which greatly helps people keep current with the latest and greatest > fixes incl. security, and even the next minor version with relatively limited testing, when we > ensured downwards compatibility within these versions. It does mean that our finger needs to be a > lot more hesitant on the trigger when we evaluate behavioral changes such as these. > > It's arguably too late to revert this patch now, but I think it's a good opportunity > to discuss the guidelines for dealing with such issues. > > Thoughts? It appears hard to categorize different kinds of bugs, so the best I can think of would be to start a discussion in internals and/or to provide a pull request before committing a bug fix. -- Christoph M. Becker