Page MenuHomePhabricator

Refactor `phutil_var_export`
ClosedPublic

Authored by joshuaspence on Jul 15 2014, 5:29 AM.
Tags
None
Referenced Files
F13883215: D9935.diff
Thu, Oct 3, 6:19 AM
Unknown Object (File)
Tue, Oct 1, 3:08 PM
Unknown Object (File)
Thu, Sep 26, 5:52 AM
Unknown Object (File)
Wed, Sep 25, 9:38 AM
Unknown Object (File)
Sat, Sep 14, 8:51 PM
Unknown Object (File)
Sat, Sep 14, 8:50 PM
Unknown Object (File)
Sat, Sep 14, 8:49 PM
Unknown Object (File)
Sat, Sep 14, 8:49 PM
Subscribers

Details

Summary

Using regular expressions in phutil_var_export to prettify the output is a pretty bad idea (albeit it is something that we have been using for a while). PHP's var_export function is pretty simple conceptually and we can probably just construct the "exported" variable representation ourselves in most cases (and just call out to var_export in other cases).

It's a little weird to make a prettiness/complexity tradeoff in favor of prettiness here, but I think humans end up interacting with the generated files often enough that it's somewhat valuable to have them follow conventions and look like other files.

Test Plan

arc unit

Diff Detail

Repository
rPHU libphutil
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence retitled this revision from to Refactor `phutil_var_export`.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
epriestley edited edge metadata.
This revision is now accepted and ready to land.Jul 15 2014, 4:14 PM

This seems reasonable to me -- we currently only use it in file generation, so it's fine if there are some iffy edge cases for the moment.

Do you think that this approach is preferable to the previous (regex-based) approach?

joshuaspence edited edge metadata.
  • Let var_export handle true and false values
  • Add a comment
  • We can get away with using var_export for array keys
  • Add some additional test cases

Don't perform preg_replace on var_export

I'm a little worried about modifying (what should be) valid PHP code with a regular expression. I'd rather settle for slightly uglier PHP code than risk invalid PHP code.

joshuaspence edited edge metadata.

(@epriestley, just bumping this back to your queue because I made a few changes and want to make sure you think this is a good idea in general)

epriestley edited edge metadata.

I think this is fine. It's a little weird to make a prettiness/complexity tradeoff so far in favor of prettiness here (that is, this is 100 lines of somewhat-complex code which does nothing but adjust whitespace in generated files), but I think humans end up interacting with the generated files often enough that it's somewhat valuable to have them follow conventions and look like other files. We also never use this stuff to serialize + eval data (and probably never will -- in applicable cases, we generally use serialize() + unserialize() instead) so I don't think this approach has any real risk.

src/utils/__tests__/PhutilUtilsTestCase.php
608–623

I think it would also be reasonable to not support serializing objects at all.

This revision is now accepted and ready to land.Jul 16 2014, 11:05 PM
joshuaspence edited edge metadata.
joshuaspence updated this object.
joshuaspence updated this object.
src/utils/__tests__/PhutilUtilsTestCase.php
608–623

Yeah, I agree... I put test cases in anyway to make sure things don't explode.

  • Let var_export handle true and false values
  • Add a comment
  • We can get away with using var_export for array keys
  • Add some additional test cases
  • Don't perform preg_replace on var_export
  • Add comments
  • Don't use a static variable
  • Minor change
  • Add some more test cases
  • Minor comment change
  • Change double quotes to single quotes