Page MenuHomePhorge

Fix a PHP 8.1 deprecated use of strlen with a NULL argument in commit parser
ClosedPublic

Authored by bob on Aug 28 2023, 13:04.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jun 13, 16:25
Unknown Object (File)
Wed, Jun 12, 23:37
Unknown Object (File)
Wed, Jun 12, 08:28
Unknown Object (File)
Sat, Jun 8, 16:22
Unknown Object (File)
Sat, Jun 8, 16:22
Unknown Object (File)
Sat, Jun 8, 07:34
Unknown Object (File)
Sat, Jun 8, 02:48
Unknown Object (File)
Sat, Jun 8, 02:45

Details

Summary

With PHP 8.1+ it is not possible to import a commit if the commiter field is not properly defined
Indeed, if the committer is not properly defined, strlen(null) is called, causing a deprecation warning, elevated to exception.
Using strlen() to check string validity is deprecated since PHP 8.1. Phorge adopts phutil_nonempty_string() as a replacement.

Fix T15629

Test Plan
  • Push a commit to an observed subversion repository
  • Import it via the phorge/bin/repository reparse COMMIT_ID --importing
  • The commit should be properly imported and available in Diffusion

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bob requested review of this revision.Aug 28 2023, 13:04
Sten subscribed.

Looks good to me.

valerio.bozzolan retitled this revision from Fix a PHP 8.1 deprecated use of strlen with a NULL argument to Fix a PHP 8.1 deprecated use of strlen with a NULL argument in commit parser.Aug 28 2023, 19:57

Thanks bob. Just to say that I care about your code proposals and sorry if this needed extra time.

There was still a very small margin of probability that the $committer sometime was an object that could be casted to string, and that before was allowed, and we should not introduce an accidental crash because of the well-known cool strictness of phutil_nonempty_string().

Basically, the DiffusionCommitRef.getCommitter is based on DiffusionCommitRef.formatUser() that may directly expose an object, for example, if DiffusionCommitRef allows to have an email that is not a string but an object like PhutilEmailAddress.

So, my doubt is: can we receive a DiffusionCommit with an email that is a PhutilEmailAddress, or a similar object?

My answer is: very probably this is not our problem 😎 The only usages of new DiffusionCommitRef are string based. Examples:

  • DiffusionLowLevelCommitQuery creates DiffusionCommitRef with emails that are parsed with PhutilEmailAddress but then are extracted as string (as far as I can understand)

So I'm 99.98% sure that this is safe since we only receive NULL or a string. Anything alien will cause a nuclear implosion, but Phorge loves that.

If somebody will be affected by such nuclear implosion, we will evaluate that case as usual to be less strict than this. Thanks bob.

Ready to land!

This revision is now accepted and ready to land.Nov 20 2023, 16:21