Page MenuHomePhorge

Fix DifferentialCommitMessageField renderFieldValue PHP 8.1 strlen(null) error
ClosedPublic

Authored by Sten on Jul 5 2023, 10:34.

Details

Summary

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

Test Plan

Make a change in a git repo with remote a Phorge server running PHP 8.1
Run:

arc diff

See exception thrown as per T15530

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Sten requested review of this revision.Jul 5 2023, 10:34

Handle renderFieldValue in DifferentialRevisionIDCommitMessageField as well

Thanks. Some tips:

  1. Probably we can avoid to create a "dummy" DifferentialTestCommitMessageField and just use something like DifferentialSubscribersCommitMessageField in the test case
  2. we can share some steps to reproduce in the test plan

Soft +1 in the meanwhile

src/applications/differential/field/DifferentialCommitMessageField.php
63

OH NO, AN EXTRA NEWLINE

  1. Probably we can avoid to create a "dummy" DifferentialTestCommitMessageField and just use something like DifferentialSubscribersCommitMessageField in the test case

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.

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)?

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.

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.

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)

Gentle reminder that this has been waiting review for a while now...

This revision is now accepted and ready to land.Aug 11 2023, 07:30

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.

src/applications/differential/field/DifferentialTestCommitMessageField.php
4

Turns out we can't do that.