Modified PhpArray config writer to generate better readable array format. #5259
Conversation
|
|
||
| if ($line === '),' || $line === ')') { | ||
| $indentLevel--; | ||
| } else if (preg_match('/^\s*array \(/', $line)) { |
There was a problem hiding this comment.
What if the line contains array ( as string value? It's probably more safe to rely on token_get_all() here.
There was a problem hiding this comment.
Actually, it's probably better to just build the PHP code up yourself. Doing a var_export() and then parsing it is for once slow and also a little PITA.
There was a problem hiding this comment.
elseif instead of else if for PSR-2 : https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md#51-if-elseif-else
There was a problem hiding this comment.
@DASPRiD I've modified it to build the array by hand. A microtime() based benchmark that I ran locally shows the execution time on the same order of magnitude as the existing simple var_export(...) method.
I've also provided for optional PHP 5.4+ array style generation.
There was a problem hiding this comment.
Thanks for the benchmark, that's always a good thing.
|
|
||
| foreach ($config as $key => $value) { | ||
| $arrayString .= str_repeat(' ', $indentLevel); | ||
| $arrayString .= (is_numeric($key) ? $key : "'" . addslashes($key) . "'") . ' => '; |
There was a problem hiding this comment.
I'm not 100% certain if is_numeric() is correct here, as it would render a string number as integer then as well. Probably better use is_int() and is_float()?
There was a problem hiding this comment.
I changed it to use is_int() (push forthcoming).
The following...
var_dump(array(1.234 => 'hi'));... truncates the float for the array key, so we'll end up with an int anyway.
| @@ -19,8 +24,48 @@ class PhpArray extends AbstractWriter | |||
| */ | |||
| public function processConfig(array $config) | |||
| { | |||
| $arrayString = "<?php\n" | |||
| . "return " . var_export($config, true) . ";\n"; | |||
| $array = array( | |||
There was a problem hiding this comment.
Probably rename to $arrayStyle or $arraySyntax to make it more clear?
|
Thanks for the attention and your comments @DASPRiD. If there's anything else I'll take of it over the next few days. |
| 'close' => $this->useBracketArraySyntax ? ']' : ')' | ||
| ); | ||
|
|
||
| return "<?php\n\n" . |
There was a problem hiding this comment.
Why this double linebreak here? :)
There was a problem hiding this comment.
Because whoops! :)
|
|
||
| foreach ($config as $key => $value) { | ||
| $arrayString .= str_repeat(self::INDENT_STRING, $indentLevel); | ||
| $arrayString .= (is_int($key) ? $key : "'" . addslashes($key) . "'") . ' => '; |
There was a problem hiding this comment.
This should contain is_float() additionally.
There was a problem hiding this comment.
See #5259 (comment) and The PHP Manual
Floats are also cast to integers, which means that the fractional part will be truncated. E.g. the key 8.7 will actually be stored under 8.
is_float(...) will never be true in this scenario.
|
Also, the broken Travis build is due to coding standards in files having nothing to do with what I changed. :) |
ghost
assigned 
Saw this Twitter conversation and agreed.
This PR modified
Zend\Config\Writer\PhpArrayto generate better readable arrays. Specifically, it will (1) indent 4 spaces per level instead of 2 and (2) put the openingarray(on the same line as the=>.Tests pass, and this "shouldn't" affect BC because it's "just" whitespace.