Correct a PHP8 compatibility issue when running "arc diff" with no active branch
ClosedPublic
Actions

Authored by jkimbo on May 23 2023, 12:45.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15412: PHP 8.1 "strlen(null)" exception when running "arc diff" with no active branch

Commits

rARC0e32dbc1ac8f: Correct a PHP8 compatibility issue when running "arc diff" with no active branch

Summary

When there is no active branch name, arc diff currently fails under PHP8 when we try to strlen(null).

This change is also credited to Evan from upstream Phabricator that applied the same change:

https://secure.phabricator.com/rARCc39ab20eb3717a15aed2467842bd77d9addce96a

Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.

Closes T15412

Test Plan

Under PHP 8.1: ran git checkout <hash of head>, then arc diff to generate this revision.

Diff Detail

Repository

rARC Arcanist

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jkimbo created this revision.May 23 2023, 12:45

Owners added a reviewer: O1: Blessed Committers.May 23 2023, 12:45

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 23 2023, 12:45

Harbormaster completed remote builds in B444: Diff 792.May 23 2023, 12:45

jkimbo requested review of this revision.May 23 2023, 12:45

jkimbo mentioned this in Q57: PHP 8.1 "strlen(null)" exception when running "arc diff" with no active branch.May 23 2023, 12:46

valerio.bozzolan edited the summary of this revision. (Show Details)May 23 2023, 12:52

valerio.bozzolan awarded a token.

valerio.bozzolan edited the summary of this revision. (Show Details)

valerio.bozzolan mentioned this in Code Differences between Phabricator and Phorge.

Thanks :)

I blindly accept since I totally trust Evan and also it's reasonable to assume that a branch name will never become a native integer or a boolean or an array etc. for legitimate reasons.

The function phutil_nonempty_string() will explode as usual if it will find a non-null or non-string, and that is OK here to me.

Ready to land!

lgtm

This revision is now accepted and ready to land.May 23 2023, 12:57

@valerio.bozzolan I'm not familiar with the process here, is it up to me to land the revision or do I wait until a maintainer does it?

Also thanks for the review :)

To land this patch, you can download Arcanist and:

arc patch D25237
arc land

If you can't for some reasons, don't worry: I will land this in 8 days, hoping to be useful

Thanks again :)

In D25237#7155, @valerio.bozzolan wrote:
To land this patch, you can download Arcanist and:
arc patch D25237
arc land
If you can't for some reasons, don't worry: I will land this in 8 days, hoping to be useful

Thanks again :)

I think it's a pretty hallucinatory experience to be tested at least once in your life

But feel free to say "naaah, not today. Land for me".

Closed by commit rARC0e32dbc1ac8f: Correct a PHP8 compatibility issue when running "arc diff" with no active branch. · Explain WhyMay 24 2023, 19:06

This revision was automatically updated to reflect the committed changes.

Restricted Repository Identity added a commit: rARC0e32dbc1ac8f: Correct a PHP8 compatibility issue when running "arc diff" with no active branch.

Awesome, revision landed. Thanks for the help @valerio.bozzolan