Page MenuHomePhabricator

Addressing PHP8 incompatibilities - Conduit
ClosedPublic

Authored by cspeckmim on May 29 2023, 11:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 5:53 PM
Unknown Object (File)
Tue, Dec 17, 2:30 AM
Unknown Object (File)
Mon, Dec 16, 8:13 AM
Unknown Object (File)
Fri, Dec 13, 11:39 AM
Unknown Object (File)
Tue, Dec 10, 6:47 PM
Unknown Object (File)
Fri, Dec 6, 3:18 PM
Unknown Object (File)
Thu, Dec 5, 7:17 PM
Unknown Object (File)
Thu, Dec 5, 7:54 AM
Subscribers
Restricted Owners Package

Details

Summary

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

Repository
rP Phabricator
Branch
cspeck-php8-conduit
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 25829
Build 35657: arc lint + arc unit

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.
src/applications/phriction/conduit/PhrictionCreateConduitAPIMethod.php
26–31

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

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

src/applications/files/conduit/FileUploadConduitAPIMethod.php
35

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().

src/applications/phriction/conduit/PhrictionCreateConduitAPIMethod.php
26–31

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

src/applications/remarkup/conduit/RemarkupProcessConduitAPIMethod.php
44

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.
src/applications/files/conduit/FileUploadConduitAPIMethod.php
35

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