Support for string parser #111
Conversation
…ld more easily extend class
…on to `getParser()`
|
Looks good to me overall, just waiting on @DavidePastore's review 👍 |
|
I've fixed documentation and added warning about arbitrary code execution. |
|
Thanks for all your work @filips123 👍 Does this PR supersede #109? |
|
Yes. |
|
I've had another look at the PR - I guess my main concern is that there seems to be some code duplication between the |
|
I've been thinking about this, but I don't know how exactly should I make this. Do you have some ideas? |
There was a problem hiding this comment.
@filips123 Thanks for all your work! 😮 Please, improve the code following @hassankhan feedback and my code reviews.
src/StringParser/Ini.php
Outdated
| * INI string parser | ||
| * | ||
| * @package Config | ||
| * @author Jesus A. Domingo <[email protected]> |
There was a problem hiding this comment.
Please, add yourself as author of this class, removing the others.
| * Abstract string parser | ||
| * | ||
| * @package Config | ||
| * @author Jesus A. Domingo <[email protected]> |
There was a problem hiding this comment.
Please, add yourself as author of this class, removing the others.
src/StringParser/Json.php
Outdated
| * JSON string parser | ||
| * | ||
| * @package Config | ||
| * @author Jesus A. Domingo <[email protected]> |
There was a problem hiding this comment.
Please, add yourself as author of this class, removing the others.
src/StringParser/Php.php
Outdated
| * PHP string parser | ||
| * | ||
| * @package Config | ||
| * @author Jesus A. Domingo <[email protected]> |
There was a problem hiding this comment.
Please, add yourself as author of this class, removing the others.
| * Config string parser interface | ||
| * | ||
| * @package Config | ||
| * @author Jesus A. Domingo <[email protected]> |
There was a problem hiding this comment.
Please, add yourself as author of this class, removing the others.
src/StringParser/Xml.php
Outdated
| * XML string parser | ||
| * | ||
| * @package Config | ||
| * @author Jesus A. Domingo <[email protected]> |
There was a problem hiding this comment.
Please, add yourself as author of this class, removing the others.
src/StringParser/Yaml.php
Outdated
| * YAML string parser | ||
| * | ||
| * @package Config | ||
| * @author Jesus A. Domingo <[email protected]> |
There was a problem hiding this comment.
Please, add yourself as author of this class, removing the others.
src/Config.php
Outdated
|
|
||
| /** | ||
| * Config | ||
| * Configuration reader and writer for PHP. |
There was a problem hiding this comment.
Please, add yourself as author in every file you edited.
src/StringParser/Ini.php
Outdated
| @@ -0,0 +1,73 @@ | |||
| <?php | |||
There was a problem hiding this comment.
This file is very similar to FileParser/Ini.php. We should share the similar functionalities in a common place. The same issue is present in Json, Xml, Yaml and in part in Php parsers.
There was a problem hiding this comment.
I've been thinking about this, but I don't know how exactly should I make this. Do you have some ideas?
|
@filips123, with regards to "write" support, do you mean writing config values back to a file, or are you talking about adding methods like |
|
I currently can't program until next Thursday so I will add changes then. |
|
@hassankhan I mean writing to files and also methods like |
|
Hi @filips123, thanks for replying. No worries, take your time 😄. So I'd just like to mention that we've discussed adding functionality similar to this (#23 and #76 ) and decided against it in the end, as we do not feel that configuration values should be changed during application runtime. Writing to files, however, is something that could definitely be handy for a variety of use-cases (concatenating various configs and writing the result to a file via a PHP CLI script). |
|
Writing to files (or strings) would also be usable for user friendly installation programs. |
|
@hassankhan @DavidePastore I've fixed some requested changes about authors, but I still don't know how to join file and string parser. Do you have some ideas? |
|
I think the best way would be as I mentioned here. It also may be worthwhile renaming any |
|
@hassankhan What function should be joined and how exactly? Maybe we could just have string parser and then pass file content to them if needed? |
|
@hassankhan @DavidePastore I've joined file and string parsers. |
|
I also updated PHPUnit to namespaced syntax and added PHP 7.2 to Travis CI (see #110). |
|
Can you please quickly merge this PR because I want to start working on configuration writing? |
|
Really sorry for the delay, is there any chance you could rebase your PR on the |
|
I changed branch. |
|
Just waiting for the build to pass (expect it should be fine), will tag a new version shortly after. |
|
Aaaaaand released as 2.0.0 🎉 !! Thanks @filips123 for your contribution, much appreciated 👍 |
It add support for parsing strings and specifying file and string parsers. It closes #108 and closes #51. It also closes #109.
It also fixes some styling and tests.
Documentation is also updated.