On Wed, May 1, 2019 at 7:36 AM Dan Ackroyd <dan...@basereality.com> wrote:
> On Wed, 1 May 2019 at 03:54, Bishop Bettini <bis...@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.
