On Thu, May 27, 2021 at 3:54 PM Sara Golemon <poll...@php.net> wrote:
> On Thu, May 27, 2021 at 8:42 AM Nikita Popov <nikita....@gmail.com> wrote: > >> The most prudent and BC-safe thing would be to add another function >>> `var_dump_escaped()` or even to prefer/advise json_encode() when content >>> safety is relevant, but additional type information is not (most of the >>> uses of var_dump we have). Unfortunately, this doesn't actually fix the >>> initial problem you stated, which is just getting useful data out of CI >>> failures. >>> >> >> Well, we had https://wiki.php.net/rfc/readable_var_representation a few >> weeks ago, which is basically a better var_dump() with escaping and all -- >> but you can tell how it went ;) >> >> > > I don't specifically remember that thread, but suddenly I feel like I can > guess everything I need to know about the outcome. :( > > > Rather than change the output at all, why not just have a post-process >>> step in our test runner that transforms the output in the test report? >>> Then we could be as aggressive as we want, going as far as escaping all >>> non-printables plus backslash since at that point we're in it for the human >>> readability (and knowing specific byte sequences is essential) and we break >>> zero BC for anyone else? >>> >> >> Transforming the test output (such that the transformed output is checked >> in EXPECT) would indeed be a way to address the original motivation. >> There's two caveats though: >> 1. It does not address the issue for other uses of var_dump(). People >> being confused by var_dump((array) $object) output because of the null >> bytes in the property names is practically a PHP classic. Of course, fixing >> this would take away from the authentic PHP experience :) >> > > True, but neither (IMO) would just escaping null byte, which again I > regard as a half-measure. It has all the negative impact of breaking BC > (your point about extension phpts is a good one), without going far enough > to address the human-readability problem for many common cases (CR/NL chief > among them). > > Insert Karate Kid "squash like a bug" reference here. Escape it all, or > escape none of it. > > >> 2. Changes to run-tests.php behavior also affect extensions, and I think >> this kind of change would be even harder to address for PECL extensions >> than a simple var_dump() behavior change (where you can at least easily >> fork two versions of the file for the tests that need it). >> >> > Would it? The report at the end of the run is for human eyes, generally > speaking. A human can interpret escaping (or not) as needed. > Also, we could make the post-processing optional. > > run-test.php --escape-test-output > > Then we use that mode in CI where we need git-friendly diffs, and it works > "normally" elsewhere. > 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: diff --git a/Zend/tests/bug60569.phpt b/Zend/tests/bug60569.phpt index 480c9c8f8a1285956616a119825520471bbe88b5..c658fa28a00017412a3a840498605efa7f8fe10b 100644 GIT binary patch delta 25 gcmZ3<w3=zcAEp?CiGLM&Vhj{=Q;Ule(^HkW0DWKx>i_@% delta 23 ecmZ3@w32DUA4Z0W{}j0y6mnCGixbmRmAC+9#0Sy< To avoid this issue, we need the transformation to apply the actual EXPECTed output, otherwise our test files will still be considered as binary files. Regards, Nikita
