Details

Reviewers

valerio.bozzolan
avivey

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15529: arc diff DifferentialDiff getFieldValuesForConduit PHP 8.1 strlen(null) errors

Commits

rP8310591523f5: Fix DifferentialDiff getFieldValuesForConduit PHP 8.1 strlen(null) errors

Summary

Fix DifferentialDiff getFieldValuesForConduit PHP 8.1 strlen(null) errors by replacing the strlen() calls with phutil_nonempty_string().

Also needed to update DifferentialDiffTestCase.php to being a PhabricatorTestCase, as it was throwing an exception prior to any code changes -

EXCEPTION (Exception): Trying to read configuration "policy.locked" before configuration has been initialized.

Fixes T15529

Test Plan

arc diff

Diff Detail

Repository

rP Phorge

Branch

arcpatch-D25333

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 725
Build 725: arc lint + arc unit

Event Timeline

Sten created this revision.Jul 5 2023, 09:47

Owners added a reviewer: O1: Blessed Committers.Jul 5 2023, 09:47

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

Sten requested review of this revision.Jul 5 2023, 09:47

Harbormaster completed remote builds in B638: Diff 1073.Jul 5 2023, 09:47

I'm reasonably sure that "branches" and "bookmarks" are always strings, but not so much about "base revision" and svn.

src/applications/differential/storage/DifferentialDiff.php
798–804	Can somebody check that this one isn't an integer under Subversion?

Explicitly check for null instead of using phutil_nonempty_string()

Harbormaster completed remote builds in B696: Diff 1164.Jul 28 2023, 13:55

Sten marked an inline comment as done.Jul 28 2023, 13:56

Sten added inline comments.

src/applications/differential/storage/DifferentialDiff.php
798–804	strlen() on an integer works (at least under PHP 8.1), so we can switch to an explicit null check.

Soft+1 but let's wait for another +1. Thanks!

src/applications/differential/storage/DifferentialDiff.php
798–804	Or, we can switch to `phutil_nonempty_scalar()` to avoid future pitfalls with `strlen(123)`

Just checked the getSourceControlBaseRevision() functions

Harbormaster completed remote builds in B724: Diff 1201.Aug 3 2023, 09:30

Sten planned changes to this revision.Aug 3 2023, 09:30

Sten added inline comments.

src/applications/differential/storage/DifferentialDiff.php
798–804	Just checked the getSourceControlBaseRevision() functions Git: returns trimmed output of an exec'ed command. Must be a string. Mercurial: returns output of an exec'ed comamnd. Must be a string. SVN: returns some string concatenated values including an '@'. Must be a string.

We do need the null check - but only for the unit test when the diff is not associated with a repository.
For live use, getSourceControlBaseRevision() will always return a string.

Harbormaster completed remote builds in B725: Diff 1202.Aug 3 2023, 09:45

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

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

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

Closed by commit rP8310591523f5: Fix DifferentialDiff getFieldValuesForConduit PHP 8.1 strlen(null) errors. · Explain WhyAug 11 2023, 09:40

This revision was automatically updated to reflect the committed changes.

Sten added a commit: rP8310591523f5: Fix DifferentialDiff getFieldValuesForConduit PHP 8.1 strlen(null) errors.

Sten mentioned this in T15600: arc patch broken - class "DifferentialTestCommitMessageField" must define a "FIELDKEY" constant.Aug 11 2023, 17:27