Paths

Table of Contentst

Fix DifferentialCommitMessageField renderFieldValue PHP 8.1 strlen(null) error
ClosedPublic
Actions

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

Details

Reviewers

valerio.bozzolan
avivey

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15530: arc diff throws PHP 8.1 strlen(null) error from DifferentialCommitMessageField renderFieldValue

Commits

rP4b3c384856da: Fix DifferentialCommitMessageField renderFieldValue PHP 8.1 strlen(null) error

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

Branch

DifferentialCommitMessageField (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 639
Build 639: arc lint + arc unit

Event Timeline

Sten created this revision.Jul 5 2023, 10:34

Owners added a reviewer: O1: Blessed Committers.Jul 5 2023, 10:34

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptJul 5 2023, 10:34

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

Harbormaster completed remote builds in B639: Diff 1074.Jul 5 2023, 10:34

Handle renderFieldValue in DifferentialRevisionIDCommitMessageField as well

Harbormaster completed remote builds in B640: Diff 1075.Jul 5 2023, 10:43

Thanks. Some tips:

Probably we can avoid to create a "dummy" DifferentialTestCommitMessageField and just use something like DifferentialSubscribersCommitMessageField in the test case
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

Remove blank line

Harbormaster completed remote builds in B642: Diff 1077.Jul 5 2023, 12:37

Sten edited the test plan for this revision. (Show Details)Jul 5 2023, 12:39

In D25334#9617, @valerio.bozzolan wrote:

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

avivey accepted this revision.Aug 11 2023, 07:30

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

Closed by commit rP4b3c384856da: Fix DifferentialCommitMessageField renderFieldValue PHP 8.1 strlen(null) error. · Explain WhyAug 11 2023, 09:38

This revision was automatically updated to reflect the committed changes.

Sten added a commit: rP4b3c384856da: Fix DifferentialCommitMessageField renderFieldValue PHP 8.1 strlen(null) error.

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.