Page MenuHomePhorge

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

Authored by bob on Aug 16 2023, 16:17.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 10, 23:15
Unknown Object (File)
Thu, Apr 4, 18:52
Unknown Object (File)
Mon, Apr 1, 06:12
Unknown Object (File)
Mon, Apr 1, 06:12
Unknown Object (File)
Mon, Apr 1, 05:40
Unknown Object (File)
Mon, Apr 1, 05:40
Unknown Object (File)
Mon, Apr 1, 02:39
Unknown Object (File)
Mon, Apr 1, 02:39

Details

Summary

This call prevents users to view a commit in subversion repositories
Indeed, if commiter and/or author field is not properly defined strlen is call with a NULL argument.
Using strlen to check string validity is deprecated since PHP 8.1, phorge adopts phutil_nonempty_string() as a replacement.

Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.

Fix T15610

Test Plan
  • Sign in (if needed)
  • Open a diffusion SVN repository
  • Open a commit without user name and or email
  • You should be able to view the commit

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bob requested review of this revision.Aug 16 2023, 16:17
valerio.bozzolan retitled this revision from Fix a PHP 8.1/8.2 deprecated use of strlen with a NULL argument to Fix a PHP 8.1 deprecated use of strlen with a NULL argument.Aug 19 2023, 02:52
valerio.bozzolan edited the summary of this revision. (Show Details)

Thanks

Can somebody else test this? I need more time, since I have issues with hosted repositories at the moment :(

Also bob: is "Sign in" really needed in the test plan in your opinion?

Maybe not. Just being able to see the repository is OK (e.g. with a public Phorge and a public repo)

You're right valerio, this sign in is an automatic reflex since my Phorge instance is private ;)

I was trying to test this, but I don't know how to do a Subversion commit without a committer name 🤔

An example repository would be greatly appreciated.

Premising that I 100% blindly trust the changes in PhabricatorRepositoryCommit. I would just to test PhabricatorRepositoryCommitData since it's not about any "raw" string. So I would like to see if getCommitter() could be an object. Because in case, there we need the "like" thing: phutil_nonempty_stringlike().

Honestly, I don't know why we have old commits improperly filled, you can maybe play with svnadmin command on a dummy repo as described here : Change author of SVN commit

Nobody replied for months. I basically like the disclaimer and I basically trust your test in your production. Thanks.

lgtm

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

Great, I'm a little bit busy, professionally speaking but, I'll try to land it this week !