Convert Phabricator to handle "%s" / "%B" properly
ClosedPublic
Actions

Authored by epriestley on Feb 23 2014, 9:54 PM.

Details

Reviewers

arice
btrahan

Maniphest Tasks

T1191: Comments are truncated at first non-base-plane character

Commits

Restricted Diffusion Commit
rPa298a79bdac6: Convert Phabricator to handle "%s" / "%B" properly

Summary

Ref T1191. I believe we only have three meaningful binary fields across all applications:

The general cache may contain gzipped content.
The file storage blob may contain arbitrary binary content.
The Passphrase secret can store arbitrary binary data (although it currently never does).

This adds Lisk config for binary fields, and uses %B where necessary.

Test Plan

Added and executed unit tests.
Forced file uploads to use MySQL, uploaded binaries.
Disabled the CONFIG_BINARY on the file storage blob and tried again, got an appropraite failure.
Tried to register with an account containing a G-Clef, and was stopped before the insert.

Diff Detail

Repository

rP Phabricator

Branch

pbmp2

Lint

Lint Passed

Unit

Tests Passed

Event Timeline

epriestley added inline comments.Feb 23 2014, 10:05 PM

src/aphront/console/DarkConsoleCore.php
77–84	This isn't precisely related, but was triggered during testing.
src/applications/cache/PhabricatorKeyValueDatabaseCache.php
22	This data may be compressed.
src/applications/files/storage/PhabricatorFileStorageBlob.php
17–44	This never worked, see discussion in D3392.

The PushLog and RefCursor tables also store ref names in the raw. Mark those fields as binary.

Accepting as this code looks good, but I haven't done any type of exhaustive search for columns that need to be switched to binary.

I identified binary columns using these strategies:

Remembering where we'd used binary columns using my mind.
Grepping quickstart.sql (which is a comprehensive, recent dump of all schema) for BINARY, LONGBLOB, and latin1_bin.

It's possible something escaped me, but even if I did miss something the consequences are probably not severe, and the resulting error should be easy to identify and resolve. (For example, the refNameRaw columns store binary data only if you've managed to create a non-utf8 branch name in Git or Mercurial, which is probably possible but very unlikely.)