Page MenuHomePhorge

Fix DifferentialDiff getFieldValuesForConduit PHP 8.1 strlen(null) errors
ClosedPublic

Authored by Sten on Jul 5 2023, 09:47.

Details

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 requested review of this revision.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()

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)

Sten marked an inline comment as done.
  • Just checked the getSourceControlBaseRevision() functions
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.

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:02