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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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