Running through setting up and using Diffusion repositories and addressing PHP8 issues that come up.
Refs T13588
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T13588: PHP 8 Compatibility
- Commits
- rPce3d484b0ec7: Addressing some PHP8 incompatibilities - Diffusion & Differential
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
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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
Comment Actions
Fix the invoker of the conduit call to specify an appropriate offset input, also fix another null issue found while browsing through a repo
Comment Actions
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 | |
Comment Actions
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?
Comment Actions
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.