Page MenuHomePhorge

PHP8.1 fix for DiffusionServeController serveRequest()
ClosedPublic

Authored by Sten on Jul 4 2023, 11:12.

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
Branch
DiffusionServeController (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 630
Build 630: arc lint + arc unit

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