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. -Sara
