Hello,
the _convert_to_string return signature should be changed i.e returning a
status;
hence, accordingly to this status, within a context caller, you may decide
to trigger
an exception or not ; that's not the role of a conversion function to
handle
those concerns; thus the realm is wider than what it is sold here.
Cheers!
On Wed, May 1, 2019 at 7:53 AM Bishop Bettini <bis...@php.net> wrote:
> 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.
>
