Page MenuHomePhabricator

Addressing PHP8 incompatibilities - Conduit

Authored by cspeckmim on May 29 2023, 11:52 PM.
Referenced Files
F12202291: D21872.id52170.diff
Tue, Oct 3, 2:26 PM
F12193895: D21872.diff
Sun, Sep 24, 6:47 AM
F12192755: D21872.diff
Sat, Sep 23, 3:06 AM
F12162450: D21872.diff
Wed, Sep 6, 8:16 PM
F12153915: D21872.diff
Sep 4 2023, 3:48 AM
F12134759: D21872.id52175.diff
Aug 25 2023, 5:00 AM
F12134758: D21872.id52174.diff
Aug 25 2023, 5:00 AM
F12134757: D21872.id52170.diff
Aug 25 2023, 5:00 AM
Restricted Owners Package



Navigate through all the conduit pages and address PHP8 incompatibilities. Additionally test out empty form submissions on many of the "create" or "update" APIs and return more contextual errors rather than PHP8 errors.

Refs T13588

Test Plan
  • Clicked through all the conduit APIs.
  • Clicked submit on empty forms, or minimally filled-in forms.

Diff Detail

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

cspeckmim held this revision as a draft.
Owners added a subscriber: Restricted Owners Package.May 29 2023, 11:52 PM
cspeckmim added inline comments.

Looks like I could move this into PhrictionConduitAPIMethod rather than repeating it.

Fine as-is, but see inlines about Conduit error behavior.


It's probably vaguely preferable to just throw new Exception(...) instead of defining an error type in new code, unless you have a specific scenario in mind for client error handling behavior.

The error type stuff is mostly a legacy system (not usually used by newer API calls) that sounded good in theory but was bad in practice, and adding more of it probably just makes it harder to clean up without helping anyone implement any behavior. In practice the overwhelming majority of errors are effectively just "wrong/bad parameters", and essentially no client will reasonably do anything special with a "bad name parameter" error versus a "bad file data" parameter versus a "missing required parameter" error or whatever.

(If clients did care about this some day, it would probably be better to bundle them all into some kind of "parameter validation" meta-error that lists the fields in a machine-readable format, instead of having hundreds of "ERR-NO-WHATEVER" constants that clients would need to hard-code to handle.)

There are a small number of cases where clients reasonably care (the one I can remember offhand is authentication-related errors) but even these would probably be better exposed in some other meta-error-way.

The most-desirable form for all of this is probably a small set of machine-readable meta-errors (authentication, policy error, temporary/retryable service failure, parameter validation) that are easy to implement on the server side (don't require defining custom error codes for every error). And all the parameter validation should be made easy/automatic via a mechanism like PhutilTypeSpec::checkMap().


Per above, consider just reverting this and leaving a bare exception.


Boy howdy, the ol' error system ain't so great.

This revision is now accepted and ready to land.May 30 2023, 2:30 PM
cspeckmim added inline comments.

This makes sense.

Revert addition of custom error-types

This revision is now accepted and ready to land.May 30 2023, 3:17 PM

Fix new exception message to be phtranslated