Page MenuHomePhorge

PHP8.1 fix for DiffusionServeController serveRequest()
ClosedPublic

Authored by Sten on Jul 4 2023, 11:12.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 10, 00:13
Unknown Object (File)
Fri, May 10, 00:13
Unknown Object (File)
Fri, May 10, 00:13
Unknown Object (File)
Wed, May 8, 07:58
Unknown Object (File)
Wed, May 8, 07:33
Unknown Object (File)
Apr 1 2024, 02:17
Unknown Object (File)
Mar 31 2024, 02:04
Unknown Object (File)
Mar 30 2024, 10:17
Tokens
"Yellow Medal" token, awarded by avivey.

Details

Summary

When a 'git pull' is done to an https git URL, the $_SERVER variables PHP_AUTH_USER and PHP_AUTH_PW will be unset, causing PHP 8.1 to throw strlen(null) errors.

This update fixes the issue by defaulting the values to '', which results in $have_user and $have_pass having false values as desired.

Fixes T15520

Test Plan

arc unit

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Sten requested review of this revision.Jul 4 2023, 11:12

Thanks, I love this minimal change.

But interestingly, the test fails for me with a totally unrelated message.

Assertion failed, expected 'true' (at DiffusionServeControllerTestCase.php:17): handleRequest threw an exception:Database isolation currently only supports some queries. You are trying to issue a query which does not begin with an allowed keyword (INSERT, UPDATE, DELETE, START, SAVEPOINT, COMMIT, ROLLBACK): 'SELECT <C>.* FROM <C> <C>  WHERE ((r.repositorySlug IN ('<S>')))   ORDER BY <C>.<C> DESC '.

Since the error is self-evident I can suggest: feel free to remove the test.

I like changes that have unit tests.

very strange about the test passing in some setup and failing in another one.

Try now - adding PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES has resolved the Database isolation exception in other test cases I've done.

I think we can save at least 1 CPU cycle in avoiding strlen() all the time and just using other things, but this is the minimal change to avoid such crash. Thanks!

This revision is now accepted and ready to land.Jul 5 2023, 14:23