Differential D25334
Fix DifferentialCommitMessageField renderFieldValue PHP 8.1 strlen(null) error Authored by Sten on Jul 5 2023, 10:34.
Details arc diff throws strlen(null) error from DifferentialCommitMessageField renderFieldValue when calling a Phorge server running PHP 8.1 Add unit test, which required a new DifferentialTestCommitMessageField class so as to be able to test the abstract DifferentialCommitMessageField class methods. Fixes T15530 Make a change in a git repo with remote a Phorge server running PHP 8.1 arc diff See exception thrown as per T15530
Diff Detail
Event TimelineComment Actions Thanks. Some tips:
Soft +1 in the meanwhile
Comment Actions My initial test did just that, tested renderFieldValue using an existing concrete class. As luck would have it, that class overrode renderFieldValue so I got back something completely unexpected! My next attempt was to instantiate an anonymous class, but Phorge wasn't having any of that. So - in order to be absolutely sure we're testing the DifferentialCommitMessageField methods we need to create a dedicated child class for the tests. I see this as a bit cleaner. It will also improve the unit test coverage for the other child classes. Comment Actions What do you think about the test plan here? Is this relevant also to the online web interface (Diff paste) or just to running arc diff? or visiting a Diff is enough (visiting stuff I think it's not relevant)? Comment Actions I triggered the exceptions just by running 'arc diff'. Visiting a diff on the web interface was fine. Please see T15530 for the exception stack trace. Comment Actions Yep thanks but can you please also try uploading a Diff from the web interface (without and with this patch)? think your patch fixes 2 bugs in one shot. Comment Actions Tested uploading a diff via the web interface, and it's fine with and without this patch. This bug only affects 'arc diff' done from a remote arcanist client (as per the stack trace showing it's conduit calls triggering the exception) Comment Actions The test-field gets picked up automatically because we pick all fields. It's missing FIELDKEY, so arc-diff doesn't work at all, and if we add FIELDKEY it shows up as the first field in the arc-diff form.
| ||||||||||||||||