Ref T5105. This function behaves similarly to phutil_json_decode but for INI strings.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T5105: Ensure that `ArcanistTextLinter` respects `.editorconfig` settings
- Commits
- rPHU2acdbcbd19ef: Add a `phutil_ini_decode` function
Wrote unit tests.
Diff Detail
Diff Detail
- Repository
- rPHU libphutil
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| 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' => ''). | |
Comment Actions
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.
Comment Actions
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:
This part above it tells the real story:
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. | |
Comment Actions
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. | |
Comment Actions
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}