Page MenuHomePhabricator

Addressing some PHP8 incompatibilities - Diffusion & Differential
ClosedPublic

Authored by cspeckmim on May 18 2023, 4:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Feb 13, 8:48 PM
Unknown Object (File)
Mon, Feb 12, 12:30 AM
Unknown Object (File)
Sun, Feb 11, 9:51 PM
Unknown Object (File)
Sun, Feb 4, 2:10 AM
Unknown Object (File)
Sun, Feb 4, 2:10 AM
Unknown Object (File)
Sun, Feb 4, 2:10 AM
Unknown Object (File)
Sun, Feb 4, 2:09 AM
Unknown Object (File)
Sun, Feb 4, 2:09 AM
Subscribers

Details

Summary

Running through setting up and using Diffusion repositories and addressing PHP8 issues that come up.

Refs T13588

Test Plan

I set up a hosted mercurial repository and pushed commits to it over ssh.
I set up a hosted git repository and pushed commits to it over ssh.
I set up a mirrored mercurial repository over ssh.
I set up a mirrored git repository over ssh.

I created a diff on a git repository and landed it.
I created a diff on a mercurial repository and landed it.

I cloned and pushed a commit to a mercurial repo over http.
I cloned and pushed a commit to a git repo over http.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

cspeckmim held this revision as a draft.

Digging up more issues using Diffusion.

  • set up a hosted repository and pushed commits over ssh
  • addressed exceptions in the daemon logs
  • navigated through any pages I could find related to the repository, addressing exceptions as they came up

Fix support for Git, also SVN though untested

Fix an error that came up during mirroring, of HTTPS URL. Not yet tested SSH URL.

Addressed a few issues that came up while mirroring repos and importing commits.

Remove try/catch used for testing, address lint

Revert change used for testing to retrieve full stack trace

Mark limit and offset fields as optional, fix for browsing /conduit

Fix the invoker of the conduit call to specify an appropriate offset input, also fix another null issue found while browsing through a repo

Fixes while testing out push/pull and making diffs

cspeckmim retitled this revision from Addressing some PHP8 incompatibilities - Diffusion to Addressing some PHP8 incompatibilities - Diffusion & Differential.May 24 2023, 11:18 PM
cspeckmim edited the test plan for this revision. (Show Details)

Additional fixes while testing git/hg over HTTP

Fix lint, update error logging to use pht()

No changes - re-running with updated local.json so tests run properly on my host

I probably have missed some code paths but this revision has touched a lot of files as-is

src/applications/differential/customfield/DifferentialBranchField.php
42–47

I don't quite like this but it did feel a little simpler than adding null checks to the conditions below

src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php
254

This prevents null value further down through phabricator_form. I looked through a number of call sites and I think this is the only place that wasn't passing in method. I didn't go through the call hierarchy though so there were a few sites that were passing in attribute arrays computed from other things.

src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php
40

This felt weird but I wasn't sure how else to guard the strlen. Maybe phutil_nonempty_string would be appropriate here

The two phlog() commands introduced in error handling seem unrelated, can we strip those out of this change? Everything else looks good.

src/applications/diffusion/controller/DiffusionServeController.php
652

Unrelated change.

851

Unrelated change.

This revision now requires changes to proceed.May 26 2023, 11:37 PM

The two phlog() commands introduced in error handling seem unrelated, can we strip those out of this change? Everything else looks good.

The underlying issue (D21867) did not result in any error information being available. I added the hg log statement to further identify what was happening, as mercurial (even with verbose and debug info printing) did not indicate anything other than receiving a 500 error and I think just the first line of stderr here. There was no additional error information in any logs on the server. I meant to leave an inline question about this. Would this point to an issue somewhere in the output of the PhabricatorVCSResponse?

My preferred behavior is that the error is reported to the user, but not sent to the error log.

Our ability to report these errors to the user varies by VCS, but was generally poor in the last version of Mercurial I worked with. Git is only marginally better: it has a GIT_CURL_VERBOSE environmental variable which will emit all response headers, so we encode the entire message in X-Phabricator-Response headers, but you have to know that this flag exists and that it is useful. If you can figure out a way to surface a useful error to the user in Mercurial (e.g., perhaps newer versions have provided a mechanism), that would be a desirable improvement. You could conceivably surface it with an HTTP proxy in older Mercurial, although this is quite involved.

In a production environment, writing "user made a bad request" sorts of errors to the log is almost never actionable to anyone who can see the log and tends to fill the log up with garbage once a couple users write poorly-behaved bots or whatever. It may be useful in a one-user development environment, but you can just add whatever debugging code you need to in a development environment anyway.

Phabricator has only ~73 calls to phlog() and most of them seem to be lazy legacy calls that shouldn't really exist (mostly old code sending exceptions to phlog() instead of handling them) or maybe leftover debugging stuff that slipped through. The "correct" number of committed calls is probably 0 or nearly zero -- that is, phlog() "should" be a development tool, not a production error handling mechanism. Compare to ~3,000 throw new.

This revision is now accepted and ready to land.May 28 2023, 11:02 PM