Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:104610
MIME-Version: 1.0
References: <CAF+90c821boRO6YsO0SH0JbDb8E8eTHfEK=e4Y5_z9P7+OOnUg@mail.gmail.com>
 <CAJHtzny=SX_ojsohApAY8Xe7c_EMAKGbNe110QQVh7i5yGFm_A@mail.gmail.com> <CAF+90c-6iEBPjWuzA6=bAr=85N=JNV0+GqL3_maak3yqVP-e=Q@mail.gmail.com>
In-Reply-To: <CAF+90c-6iEBPjWuzA6=bAr=85N=JNV0+GqL3_maak3yqVP-e=Q@mail.gmail.com>
Date: Wed, 6 Mar 2019 21:17:56 +0100
Message-ID: <CANUQDCgKTQ14rvONxGDNG2wake=uHFkWBcRfAsXATRGsHg75rg@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: Zeev Suraski <zeev@php.net>, PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] [RFC] Saner string to number comparisons
From: me@kelunik.com (Niklas Keller)

Am Mo., 4. M=C3=A4rz 2019 um 18:00 Uhr schrieb Nikita Popov <nikita.ppv@gma=
il.com>:
>
> On Wed, Feb 27, 2019 at 10:23 AM Zeev Suraski <zeev@php.net> wrote:
>
> >
> >
> > On Tue, Feb 26, 2019 at 2:27 PM Nikita Popov <nikita.ppv@gmail.com> wro=
te:
> >
> >> Hi internals,
> >>
> >> I think it is well known that =3D=3D in PHP is a pretty big footgun. I=
t
> >> 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 =3D=3D in =
PHP make
> >> it so dangerous. The biggest WTF factor is probably that 0 =3D=3D "foo=
bar"
> >> returns true.
> >>
> >> I'd like to bring forward an RFC for PHP 8 to change the semantics of =
=3D=3D
> >> and other non-strict comparisons, when used between a number and a str=
ing:
> >>
> >> https://wiki.php.net/rfc/string_to_number_comparison
> >>
> >> The tl;dr is that if you compare a number and a numeric string, they'l=
l 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 sil=
ent
> >> 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 abou=
t
> >> this, for example the RFC suggests that we might want to have a transi=
tion
> >> 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, a=
nd
> >> how we could go about doing it, while minimizing breakage.
> >>
> >
> > I generally like the direction and think we should seriously consider i=
t.
> >
> > I think that before we make any decisions on this, or even dive too dee=
p
> > 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 potentia=
l
> > danger (as you say, figuring out whether there's actual breakage would
> > require a full audit of every potentially problematic sample.  Ultimate=
ly,
> > 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 fr=
om
> > 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 goi=
ng
> 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, Symfon=
y
> 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 numer=
ic
> strings (something we should definitely change). One of the Laravel
> warnings is ultimately a false-positive (does not change behavior), thoug=
h
> code could be improved to avoid it. I wasn't able to tell whether the oth=
er
> 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

Most warnings will not come from framework code, but code interfacing
with browsers, I guess.

Regards, Niklas