On Thu, Jul 16, 2020 at 9:21 AM Brent Roose <bre...@stitcher.io> wrote:
> Hey Nikita > > Thanks for the rebase. I just tested this on one of our most largest > projects (after verifying that the warning does show in a dummy test case), > and all is fine. So from my point of view, there is a theoretical chance of > breaking code, but I believe this won't have a large impact, at least not > on modern-day projects. > > Kind regards > Brent > Great to hear, thanks for testing! Regards, Nikita On 15 Jul 2020, at 11:10, Nikita Popov <nikita....@gmail.com> wrote: > > On Wed, Jul 15, 2020 at 10:56 AM Brent Roose <bre...@stitcher.io> wrote: > > Hi Nikita > > Yes that would be nice, if it's not too much of a hassle. I'm only able to > test this in one or two large Laravel projects, so it would still be a > limited test. > > Kind regards > Brent > > > Done! https://github.com/php/php-src/pull/3917 is now based on current 7.4 > HEAD. Note that it just unconditionally throws a warning, without a way to > disable it. > > Nikita > > On 15 Jul 2020, at 10:53, Nikita Popov <nikita....@gmail.com> wrote: > > On Wed, Jul 15, 2020 at 10:49 AM Brent Roose <bre...@stitcher.io> wrote: > > Hi Nikita > > Is the ini setting available in current 7.4 builds? Is it documented > somewhere? I'd like to test this change in some of our projects. > > > We did not introduce an ini setting in PHP 7.4, I only used it for my own > experiments. The implementation is available at > https://github.com/php/php-src/pull/3917. I could rebase that to current > 7.4 if that would be useful. > > Nikita > > On 15 Jul 2020, at 10:28, Nikita Popov <nikita....@gmail.com> wrote: > > On Tue, Jul 14, 2020 at 11:47 PM Björn Larsson <bjorn.x.lars...@telia.com > > > wrote: > > Den 2020-07-14 kl. 15:48, skrev Nikita Popov: > > On Thu, Jul 2, 2020 at 10:09 AM Nikita Popov <nikita....@gmail.com> > > wrote: > > > On Mon, Mar 4, 2019 at 6:00 PM Nikita Popov <nikita....@gmail.com> > > wrote: > > > On Wed, Feb 27, 2019 at 10:23 AM Zeev Suraski <z...@php.net> wrote: > > > On Tue, Feb 26, 2019 at 2:27 PM Nikita Popov <nikita....@gmail.com> > wrote: > > Hi internals, > > I think it is well known that == in PHP is a pretty big footgun. It > doesn't > have to be. I think that type juggling comparisons in a language like > PHP > have some merit, it's just that the particular semantics of == in PHP > make > it so dangerous. The biggest WTF factor is probably that 0 == > > "foobar" > > returns true. > > I'd like to bring forward an RFC for PHP 8 to change the semantics > > of == > > and other non-strict comparisons, when used between a number and a > string: > > https://wiki.php.net/rfc/string_to_number_comparison > > The tl;dr is that if you compare a number and a numeric string, > > they'll > > be > compared as numbers. Otherwise, the number is converted into a string > and > they'll be compared as strings. > > This is a very significant change -- not so much because the actual > > BC > > breakage is expected to be particularly large, but because it is a > silent > change in core language semantics, which makes it hard to determine > whether > or not code is affected by the change. There are things we can do > > about > > this, for example the RFC suggests that we might want to have a > transition > mode where we perform the comparison using both the old and the new > semantics and warn if the result differs. > > I think we should give serious consideration to making such a change. > I'd > be interested to hear whether other people think this is worthwhile, > > and > > how we could go about doing it, while minimizing breakage. > > I generally like the direction and think we should seriously consider > > it. > > > I think that before we make any decisions on this, or even dive too > > deep > > into the discussion - we actually need to implement this behavior, > including the proposed INI setting you mentioned we might add in 7.4 > > - and > > see what happens in some real world apps, at least in terms of > > potential > > danger (as you say, figuring out whether there's actual breakage would > require a full audit of every potentially problematic sample. > > Ultimately, > > I think there's no question that if we were to start from scratch, > > we'd be > > going for something along these lines. But since we're not starting > > from > > scratch - scoping the level of breakage is key here. > > Zeev > > Totally agree that assessing the amount of breakage in real code is key > here. I have now implemented a warning for PHP 7.4 (for now > > unconditional, > > no ini setting) that is thrown whenever the result of a comparison is > > going > > to change under the currently proposed rules: > https://github.com/php/php-src/pull/3917 > > I've done a few initial tests by running this against the Laravel, > Symfony and pear-core. The warning was thrown 2 times for Laravel, 1 > > times > > for Symfony and 2 times for pear-core. (See PR for the results.) > > Both of the warnings in pear-core pointed to implementation bugs. The > Symfony warning was due to trailing whitespace not being allowed in > > numeric > > strings (something we should definitely change). One of the Laravel > warnings is ultimately a false-positive (does not change behavior), > > though > > code could be improved to avoid it. I wasn't able to tell whether the > > other > > one is problematic, as it affects sorting order. > > I have to say that this is a lot less warnings than I expected. Makes > > me > > wonder if I didn't make an implementation mistake ^^ > > Regards, > Nikita > > As we're moving closer to PHP 8 feature freeze, I want to give this RFC > > a > > bump. I've updated the text to account for some changes that have > > happened > > in the meantime, such as the removal of locale-sensitivity for float to > string conversions. > > It's been quite a while since we discussed this last, and back then the > discussion was fairly positive. Some experiments with a warning mode > > also > > showed that the impact, at least in framework/library code, appears to > > be > > fairly low in practice, contrary to what one might intuitively expect. > > Now would be the time to decide whether or not we want to pursue this > change for PHP 8. > > And then there was silence... > > I think I'll just put this up for vote on Friday, and we'll see what > > people > > think :) > > Nikita > > > Seems like a very good idea!! Especially in conjunction with the RFC: > - https://wiki.php.net/rfc/saner-numeric-strings > > Btw, in the RFC there is a reference to the "Trailing whitespace in > numeric > strings" RFC. Update to reference "Saner numeric strings" RFC instead? > > > Thanks, I've updated the link to point to the new proposal! > > Nikita > > >
