Page MenuHomePhabricator

Add a `phutil_ini_decode` function
ClosedPublic

Authored by joshuaspence on Jan 20 2015, 10:45 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 26, 5:26 AM
Unknown Object (File)
Wed, Nov 23, 4:13 PM
Unknown Object (File)
Wed, Nov 23, 4:13 PM
Unknown Object (File)
Wed, Nov 23, 4:13 PM
Unknown Object (File)
Wed, Nov 23, 4:13 PM
Unknown Object (File)
Wed, Nov 23, 4:13 PM
Unknown Object (File)
Wed, Nov 23, 4:13 PM
Unknown Object (File)
Wed, Nov 23, 4:12 PM
Subscribers

Details

Summary

Ref T5105. This function behaves similarly to phutil_json_decode but for INI strings.

Test Plan

Wrote unit tests.

Diff Detail

Repository
rPHU libphutil
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

joshuaspence retitled this revision from to Add a `phutil_ini_decode` function.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence edited edge metadata.

Remove unrelated function

joshuaspence updated this object.

Work around PHP being dumb

joshuaspence added inline comments.
src/utils/utils.php
1070

I'm not sure what I should do here... without INI_SCANNER_RAW parse_ini_string("foo = true\nbar = false") returns array('foo' => '1', 'bar' => '').

I can probably write this to be more compatible with PHP 5.2, but it would come at a cost. Basically, if we drop the use of INI_SCANNER_RAW then this function should be backwards compatible, but we would not be able to differentiate between x = , x = false, x = null, x = off and x = no.

epriestley edited edge metadata.

One concrete inline.

What version of PHP has INI_SCANNER_RAW but not parse_ini_string()? It looks like they both came from PHP 5.3, so we should be able to remove the TempFile stuff? Or am I reading the docs wrong / are the docs wrong?

I think it's fine to make this require 5.3, since we don't plan to use it in core functionality. Maybe just throw an exception at the beginning if the PHP version is too old? The complexity to support 5.2 doesn't seem worthwhile, and the linter can easily just not respect .editorconfig under 5.2.

src/utils/utils.php
1067

The PhutilErrorTrap documentation is confusing. This part is misleading:

  • - Traps capture all errors until they are destroyed, usually by leaving
  • scope..

This part above it tells the real story:

  • NOTE: You must explicitly destroy traps because they register themselves with
  • PhutilErrorHandler, and thus will not be destroyed when unset().

That is, traps do not self-destruct when they leave scope. Specifically, if anything in this method throws, the trap will remain active. There should be a try/catch/destroy/rethrow around all of the trapped code.

1085

This feels suspicious, but I can't come up with a case offhand where it gets the wrong result (maybe dates, octal, scientific notation?). Maybe put a comment here explaining the cleverness, it took me a second read to get it.

This revision now requires changes to proceed.Jan 22 2015, 8:32 PM

Yeah you are correct about ini_parse_string. I left the conditional there because I was trying to find a way to support PHP 5.2 as well, but couldn't come up with anything.

I'm fine with this function requiring PHP 5.3.

src/utils/utils.php
1085

Yeah, I would agree with you on that. I started out by just doing:

if ($result == 'true') {
  $result = true;
} else if ($result == 'false') {
  $result = false;
}

But then I realized that we should convert integers and floating point numbers as well, and json_decode just felt cleaner.

joshuaspence edited edge metadata.
  • Drop PHP 5.2 (in)compatibility
  • Make sure the error trap is destroyed
epriestley edited edge metadata.
This revision is now accepted and ready to land.Jan 22 2015, 9:13 PM

One minor issue here is that the unit tests will fail on PHP 5.2.x. Seeing as this will only affect Phabricator developers, I don't think that this is a major issue.

   FAIL  PhutilUtilsTestCase::testPhutilINIDecode
EXCEPTION (PhutilMethodNotImplementedException): phutil_ini_decode is not compatible with your version of PHP (5.2.17). This function is only supported on PHP versions newer than 5.3.0.
#0 /home/joshua/workspace/github.com/phacility/libphutil/src/utils/__tests__/PhutilUtilsTestCase.php(566): phutil_ini_decode('')
#1 [internal function]: PhutilUtilsTestCase->testPhutilINIDecode()
#2 /home/joshua/dotfiles/modules/phabricator/arcanist/src/unit/engine/phutil/ArcanistPhutilTestCase.php(484): call_user_func_array(Array, Array)
#3 /home/joshua/dotfiles/modules/phabricator/arcanist/src/unit/engine/PhutilUnitTestEngine.php(58): ArcanistPhutilTestCase->run()
#4 /home/joshua/dotfiles/modules/phabricator/arcanist/src/workflow/ArcanistUnitWorkflow.php(171): PhutilUnitTestEngine->run()
#5 /home/joshua/dotfiles/modules/phabricator/arcanist/scripts/arcanist.php(364): ArcanistUnitWorkflow->run()
#6 {main}

Seems fine. You could explicitly skip the test, too.

Skip test on older versions of PHP

This revision was automatically updated to reflect the committed changes.