On Thu, May 27, 2021 at 6:11 PM Sara Golemon <poll...@php.net> wrote:
> On Thu, May 27, 2021 at 9:07 AM Nikita Popov <nikita....@gmail.com> wrote: > >> Ah, I think I explained the original issue badly: The test runner output >> isn't really a problem, or at least I never perceived it to be. >> >> The problem is that if you change a test that contains a null byte >> anywhere (read: in var_dump output), then github will not show a diff for >> that file. You can see this nicely in the proposed PR itself: >> https://github.com/php/php-src/pull/7059/files Most of the updated test >> files show up as "Binary file not shown." As you can imagine, this makes >> review hard. Using the .patch file doesn't help either, because it only >> contains entries like: >> >> > AH! Yeah, I completely misunderstood the problem space. I didn't imagine > for a second we had non-ASCII in any test because that's bad and it should > feel bad. > > Okay, then I still disagree with changing var_dump()'s output, but for > different reasons. > > So take Zend/tests/bug60569.phpt for example, which has this EXPECT > section (where ^@ is actually the null byte): > > --EXPECT-- > string(20) "Some error ^@ message" > > Instead of changing how var_dump() works in order to produce different > output, I'd change the EXPECT to and EXPECTF and add a new %0 pattern > > --EXPECTF-- > string(20) "Some error %0 message" > > This creates zero BC breaks, allows extensions to continue working just > fine with raw nulls or update to use %0 if they'd like, and creates no > ambiguities. > I like that approach. Implemented in https://github.com/php/php-src/pull/7069. Regards, Nikita
