Re: [RFC] Allow throwing exceptions from __toString()

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.