Page MenuHomePhabricator

Update PhabricatorLiskDAO::chunkSQL() for new %Q semantics

Authored by epriestley on Nov 7 2018, 1:06 AM.



Ref T13217. This method is slightly tricky:

  • We can't safely return a string: return an array instead.
  • It no longer makes sense to accept glue. All callers use ', ' as glue anyway, so hard-code that.

Then convert all callsites.

Test Plan

Browsed around, saw fewer "unsafe" errors in error log.

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Nov 7 2018, 1:06 AM
Owners added a subscriber: Restricted Owners Package.Nov 7 2018, 1:06 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Nov 7 2018, 1:07 AM
Harbormaster failed remote builds in B21099: Diff 47250!
epriestley requested review of this revision.Nov 7 2018, 1:07 AM

Tests are likely to fail since this depends on %LQ out of libphutil/ in D19783.

amckinley accepted this revision.Nov 7 2018, 9:12 PM
amckinley added inline comments.

Another place where a TODO might be appropriate.


And this is dead code because we're now doing this work inside the %LQ conversion, right?

This revision is now accepted and ready to land.Nov 7 2018, 9:12 PM
epriestley marked an inline comment as done.Nov 13 2018, 4:58 PM
epriestley added inline comments.

Right -- %LQ (or some other %L*) now do the joins safely, and implode(...) is "unsafe".

(We could make this "safe" and keep the semantics, but we'd have to whitelist acceptable glue, and we only ever use , as glue. I'm guessing third-party code has very few chunkSQL() calls since it's mostly an optimization / migration / workaround-for-low-mysql-packet-size thing.)

This revision was automatically updated to reflect the committed changes.