Php config short array syntax #8029
Conversation
| * Similar to var_export() except it uses the Short Array Syntax; | ||
| * | ||
| * @param array $data Data to dump in Short Array Syntax | ||
| * @return string returns a parsable string representation of a short array syntax variable |
There was a problem hiding this comment.
@return string A parsable ...
| public function shortArrayVarExport($data) | ||
| { | ||
| $varExport = var_export($data, true); | ||
| $pattern = ['/array \(/', '/\)/']; |
There was a problem hiding this comment.
Simply replacing all ")" with "]" looks risky. It could be part of a string for e.g.
|
I agree with @ADmad. Sorry if I am wrong, but I think this change contains potential security risks. If it fails to replace some pair of parentheses, the generated php file would throw a parse error. |
|
I would use tokenizing, that will always work 100% and is safe. |
|
If it's only matching based on the return of var_export(), when would it not be safe to use? Since var_export is supposed to always return a valid PHP code string. That would mean the data for preg_replace is always correct? |
|
What happens if the variable contains an object? |
| { | ||
| $varExport = var_export($data, true); | ||
| $pattern = [ | ||
| '/array \(/', // Matches opening "array(" |
There was a problem hiding this comment.
Won't this also match string values like 'I am a silly array ( value'?
|
I share @chinpei215's concerns. This code could run into issues with exported objects, and formatting issues from with PHP. Another approach would be to partially re-implement var_export similar to how |
|
I've taken advice, and removed the search and replace. It now builds the array structure / indentation itself. Values/Objects are still passed to var_export. Hopefully this should be a better implementation. |
|
|
||
| $filename = $this->_getFilePath($key); | ||
| return file_put_contents($filename, $contents) > 0; | ||
| } | ||
|
|
||
| /** | ||
| * |
There was a problem hiding this comment.
A newline too much by the way.
|
@JustAMurph I respect your efforts. So It’s hard for me to say this but I don't think this is a good approach. I think @dereuromark's idea might be not bad. But I don't think that is safe because people often make a mistake. That would work if we could make a perfect code. It's very hard for me to explain abstract something by my English, but.. What about sending a request for this feature to PHP core team? I don't think this is an issue of CakePHP. |
|
@chinpei215 I had similar thoughts. Sometimes I wonder what the core PHP people think... Why didn't they provide a |
|
@dereuromark I've found.. https://bugs.php.net/bug.php?id=71057 |
Someone really didn't think things through :) The output is not PSR related, its PHP syntax related, thus is is a missing core PHP functionality. Well, Well... Oh and I like the |
|
@dereuromark I am not sure, but request for adding option |
| $key = (is_int($key)) ? (int)$key : "'$key'"; | ||
|
|
||
| if (is_array($value)) { | ||
| $value = PHP_EOL . $this->shortArrayVarExport($value, $indent + 1); |
There was a problem hiding this comment.
We could run out of stackframes if someone tries to dump a monster array.
We don't want to change the output semantics, we dont want to make it safer than the original output either. We just dont want to introduce new issues. |
|
I'm going to close this pull request. I'm not sure having PSR2 compatible generated coded is worth the complexity and risk we expose ourselves to. |
Changed PhpConfig::dump() to use the Short Array Syntax. Tests are included and passing.
This for issue: #7804
It uses a new method in the PhpConfig.php file. It could be made to be more accessible by moving the method to "functions.php". However I'm not sure how beneficial that would be.