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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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