hassankhan / config

Config is a lightweight configuration file loader that supports PHP, INI, XML, JSON, and YAML files
MIT License
971 stars 136 forks source link

[2.0] __DIR__ in PHP file gets wrong path #113

Closed jfcherng closed 5 years ago

jfcherng commented 5 years ago

In my PROJECT_ROOT/config/app.config.php:

<?php

return [ 
    'manifest' => __DIR__ . '/../public/build/manifest.json',
    // ...
];

Now __DIR__ is evaluated to PROJECT_ROOT/vendor/hassankhan/config/src/Parser due to the eval() in https://github.com/hassankhan/config/blob/f33748ba6b2d2353e4c93ad830bfb26a2a00d2b2/src/Parser/Php.php#L37

Previous in version 1.x, it's evaluated to PROJECT_ROOT/config which is expected because of using require $file; implementation.


I notice that all files are now always evaluated as strings.

https://github.com/hassankhan/config/blob/f33748ba6b2d2353e4c93ad830bfb26a2a00d2b2/src/Config.php#L97

Could we provide a way to just require a PHP file? I don't think most people would like to load a PHP FILE config as a string and then evaluate it since it could be simply loaded with require and provide better results.


https://github.com/hassankhan/config/blob/f33748ba6b2d2353e4c93ad830bfb26a2a00d2b2/src/Parser/Php.php#L30-L33

Just another concern. What if I have ?> in a string such as

<?php

return [
    'condition1' => '?>count',
    'condition2' => 'count<?',
    'php_starting_tag' => '<?php',
];

It could be simplified into $config = str_replace(['<?php', '<?', '?>'], '', $config); by the way.

filips123 commented 5 years ago

There could be two methods in the parser. One to read from file and one to read from string.

Quick example:

    public function parse($config, $string = false, $filename = null)
    {
        // Load file
        if ($string === false) {
            try {
                $temp = require $config;
            } catch (Exception $exception) {
                throw new ParseException(
                    [
                        'message'   => 'PHP file threw an exception',
                        'exception' => $exception,
                    ]
                );
            }

        // Load string
        } else {
            // Strip PHP start and end tags
            $config = str_replace('<?php', '', $config);
            $config = str_replace('<?', '', $config);
            $config = str_replace('?>', '', $config);
            // Eval the string, if it throws an exception, rethrow it
            try {
                $temp = eval($config);
            } catch (Exception $exception) {
                throw new ParseException(
                    [
                        'message'   => 'PHP string threw an exception',
                        'exception' => $exception,
                    ]
                );
            }
        }

        // If we have a callable, run it and expect an array back
        if (is_callable($temp)) {
            $temp = call_user_func($temp);
        }
        // Check for array, if its anything else, throw an exception
        if (!is_array($temp)) {
            throw new UnsupportedFormatException('PHP string does not return an array');
        }
        return $temp;
    }

Also, it problem with this only in PHP files or it is also in other formats?


Just another concern. What if I have ?> in a string such as ...

Maybe there could be check to remove only first and last line if there are <?php, <? or ?>.

jfcherng commented 5 years ago

@filips123

Also, it problem with this only in PHP files or it is also in other formats?

I guess PHP is the only special case.

Maybe there could be check to remove only first and last line if there are <?php, <? or ?>.

LGTM if people do not load string like the following, at least I wont.

<?php
// ...
?>

<?php
// ...
?>
hassankhan commented 5 years ago

@filips123 We should have two separate methods, loadFile() and loadString(), but other than that the example seems good 👍

filips123 commented 5 years ago

I think that program should remove PHP tags with this conditions:

Start tags (<?php, <?): Program should remove start tags only in blank lines in the beginning (until first non-blank line).

End tags (?>): Program should remove end tags only in blank lines in the end (after last non-blank line).


Blank line is a line which only have PHP tag, spaces, tabs and comments.

Example blank lines:

<?php
<?
?>

       <?php
<?         // Hello
/*
A
*/
/**
 * A
 */
     <?php      // Hello

Non-blank file is a line which have other things.

Example non-blank lines:

$a = null;
$b = 'Hello World'; // Set b to Hello World

Example of tags that should be removed:

// This ↓ because there is only a tag in a line
<?php
<?      // This ← because there is only tag, spaces and comment in a line
    <?php
// This ↑ because there are only spaces and tag in a line

?> // This ← should not be removed because it is end tag

return [
   'start-tag' => '<?php', // This ← should not be removed because it is not in blank line
];

// This ↓ because there is only a tag in a line
?>
?>      // This ← because there is only tag, spaces and comment in a line
    ?>
// This ↑ because there are only spaces and tag in a line

<?php // This ← should not be removed because it is start tag

I hope it is clear what I mean (I don't know how else can i tell this). How could I make this?

hassankhan commented 5 years ago

@filips123 Personally I'd stick with the simple approach as you outlined above, anything more would be extra complexity and would be outside the scope of this project (IMO).

/cc @DavidePastore thoughts?

jfcherng commented 5 years ago

@hassankhan I found an interesting way to utilize eval() without stripping PHP tags.

Simply prepend a ?> to the string.

image

Thus, $temp = eval('?>' . trim($config)); could be good enough. But better to add some comments since this is not common to see.

filips123 commented 5 years ago

@jfcherng This is OK, but it will not work when string not contain <?php, because code will be interpreted as HTML.

$str = 'return [\'a\' => \'b\'];';
eval('?>' . $str);
// eval('?>return [\'a\' => \'b\'];');
jfcherng commented 5 years ago

@filips123 In that case, if this repo were mine, I would define the input as either a string could be interpreted as PHP code, or a string that has no PHP tags, but not both. Just like eval() wont evaluate a string starting with <?php.

Or, just check whether trim($config) is starting with <? or not.

$config = trim($config);

if (substr($config, 0, 2) === '<?') {
    $config = "?>{$config}";
}