Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:105546
MIME-Version: 1.0
References: <CAF+90c-3TcEME_993vxvbhgCsKCrVVMSLobjrc=-EUZOmxYRYg@mail.gmail.com>
 <CA+kxMuSPDckN+YDN-PuqU-YXKm08fyTWh_7yK0YecTSJOFLfvA@mail.gmail.com>
 <2b141597-e68e-1843-51f5-16d6ed8c41fa@gmail.com> <CAEYWF=5Yay7SvL5wCe9WjwdWW+HD_Ht0a+TtFzeTB1R5wmE0xQ@mail.gmail.com>
 <CA+kxMuQhEiVO91PPm67DpjU-pXy0QmWdj6mZTVkMPbFeOtXEhA@mail.gmail.com>
In-Reply-To: <CA+kxMuQhEiVO91PPm67DpjU-pXy0QmWdj6mZTVkMPbFeOtXEhA@mail.gmail.com>
Reply-To: bishop@php.net
Date: Wed, 1 May 2019 10:52:38 -0400
Message-ID: <CAEYWF=6sXiUwhFQdnevuNh5edpncj6wTaqUzOpra=safN2uRVw@mail.gmail.com>
To: Dan Ackroyd <Danack@basereality.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000c345960587d4ac84"
Subject: Re: [PHP-DEV] [RFC] Allow throwing exceptions from __toString()
From: bishop@php.net (Bishop Bettini)

--000000000000c345960587d4ac84
Content-Type: text/plain; charset="UTF-8"

On Wed, May 1, 2019 at 7:36 AM Dan Ackroyd <Danack@basereality.com> wrote:

> On Wed, 1 May 2019 at 03:54, Bishop Bettini <bishop@php.net> wrote:
> >
> > But I'd still think this would be a "many eyes needed" kind of PR,
> especially from extension maintainers.
>
> Hypothetically, what should these extension maintainers be looking for?
>
> Other than "Mmm-hmm. Mmm-HMM. I know some of these words!" ?
>

Indeed. I'm by no means an expert, but here's what I did for phar and imap
extensions. I read the RFC and "got to know" the new helper methods for
resolving stringy content. I then looked at the PR for all changes to phar
and imap, verifying that I understood the changes Nikita made. Then I went
to master and PHP-7.4 branches and looked for any other cases that might
need to be changed, and while there took mental note of improvement
opportunities should this RFC pass.

When I was in there, I was basically looking for "convert_to_string" (and
friends) and if I found any that Nikita hadn't already found, replacing
those with one of the new helpers and making sure I understood the choice
Nikita made and commenting if I disagreed. I was also looking for any case
where the conversion touched data that persisted and made sure it wasn't
going to get trashed by a bad conversion.

I also went through code outside phar and imap looking for things I didn't
understand or surprised me or didn't look correct. Suffice to say, there's
a lot of code to look through. Some code surprised me, like that the (new
DateInterval(...))->{$badStr} unhappy path didn't adhere to the
has_property contract, so that was a bugfix Nikita made as an artifact of
this sweep, which is great and I think emblematic of the reason to get more
eyes on this: might be more of those lurking in the code base.

--000000000000c345960587d4ac84--