Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Modified PhpArray config writer to generate better readable array format. #5259

Closed
wants to merge 13 commits into from

4 participants

@michaelmoussa

Saw this Twitter conversation and agreed.

This PR modified Zend\Config\Writer\PhpArray to generate better readable arrays. Specifically, it will (1) indent 4 spaces per level instead of 2 and (2) put the opening array( on the same line as the =>.

Tests pass, and this "shouldn't" affect BC because it's "just" whitespace.

library/Zend/Config/Writer/PhpArray.php
((9 lines not shown))
- return $arrayString;
+ foreach (explode("\n", var_export($config, true)) as $line) {
+ $line = trim($line);
+
+ if ($line === '),' || $line === ')') {
+ $indentLevel--;
+ } else if (preg_match('/^\s*array \(/', $line)) {
@DASPRiD Collaborator
DASPRiD added a note

What if the line contains array ( as string value? It's probably more safe to rely on token_get_all() here.

@DASPRiD Collaborator
DASPRiD added a note

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.

@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.

@DASPRiD Collaborator
DASPRiD added a note

Thanks for the benchmark, that's always a good thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/Writer/PhpArray.php
((16 lines not shown))
+ * Sets whether or not to use the PHP 5.4+ "[]" array syntax.
+ *
+ * @param bool $value
+ */
+ public function setUseBracketArraySyntax($value)
+ {
+ $this->useBracketArraySyntax = $value;
+ }
+
+ protected function processIndented(array $config, array $array, &$indentLevel = 1)
+ {
+ $arrayString = "";
+
+ foreach ($config as $key => $value) {
+ $arrayString .= str_repeat(' ', $indentLevel);
+ $arrayString .= (is_numeric($key) ? $key : "'" . addslashes($key) . "'") . ' => ';
@DASPRiD Collaborator
DASPRiD added a note

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()?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/Writer/PhpArray.php
((15 lines not shown))
+ /**
+ * Sets whether or not to use the PHP 5.4+ "[]" array syntax.
+ *
+ * @param bool $value
+ */
+ public function setUseBracketArraySyntax($value)
+ {
+ $this->useBracketArraySyntax = $value;
+ }
+
+ protected function processIndented(array $config, array $array, &$indentLevel = 1)
+ {
+ $arrayString = "";
+
+ foreach ($config as $key => $value) {
+ $arrayString .= str_repeat(' ', $indentLevel);
@DASPRiD Collaborator
DASPRiD added a note

I suggest to make the indent-string a constant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/Writer/PhpArray.php
((28 lines not shown))
+
+ foreach ($config as $key => $value) {
+ $arrayString .= str_repeat(' ', $indentLevel);
+ $arrayString .= (is_numeric($key) ? $key : "'" . addslashes($key) . "'") . ' => ';
+
+ if (is_array($value)) {
+ if ($value === array()) {
+ $arrayString .= $array['open'] . $array['close'] . ",\n";
+ } else {
+ $indentLevel++;
+ $arrayString .= $array['open'] . "\n"
+ . $this->processIndented($value, $array, $indentLevel)
+ . str_repeat(' ', --$indentLevel) . $array['close'] . ",\n";
+ }
+ } elseif (is_object($value)) {
+ $arrayString .= var_export($value, true) . ",\n";
@DASPRiD Collaborator
DASPRiD added a note

Will this actually work properly, coding standard wise? Please add a unit tests for this.

Can you be more specific? (there are several lines highlighted, so I'm not sure which part you're talking about). Are you talking about the is_object(...) portion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/Writer/PhpArray.php
((20 lines not shown))
+ public function setUseBracketArraySyntax($value)
+ {
+ $this->useBracketArraySyntax = $value;
+ }
+
+ protected function processIndented(array $config, array $array, &$indentLevel = 1)
+ {
+ $arrayString = "";
+
+ foreach ($config as $key => $value) {
+ $arrayString .= str_repeat(' ', $indentLevel);
+ $arrayString .= (is_numeric($key) ? $key : "'" . addslashes($key) . "'") . ' => ';
+
+ if (is_array($value)) {
+ if ($value === array()) {
+ $arrayString .= $array['open'] . $array['close'] . ",\n";
@DASPRiD Collaborator
DASPRiD added a note

This part is not covered by a unit test.

Several lines highlighted so I'm not clear on which part - are you talking about the setUseBracketArraySyntax() method? If so, it's covered by ZendTest\Config\Writer\PhpArrayTest::testRenderWithBracketArraySyntax() on line 55.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/Writer/PhpArray.php
((9 lines not shown))
+ );
+
+ return "<?php\n\n" .
+ "return " . $array['open'] . "\n" . $this->processIndented($config, $array) . $array['close'] . ";\n";
+ }
+
+ /**
+ * Sets whether or not to use the PHP 5.4+ "[]" array syntax.
+ *
+ * @param bool $value
+ */
+ public function setUseBracketArraySyntax($value)
+ {
+ $this->useBracketArraySyntax = $value;
+ }
+
@DASPRiD Collaborator
DASPRiD added a note

Add docblock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@DASPRiD DASPRiD commented on the diff
library/Zend/Config/Writer/PhpArray.php
((30 lines not shown))
+ $arrayString .= str_repeat(' ', $indentLevel);
+ $arrayString .= (is_numeric($key) ? $key : "'" . addslashes($key) . "'") . ' => ';
+
+ if (is_array($value)) {
+ if ($value === array()) {
+ $arrayString .= $array['open'] . $array['close'] . ",\n";
+ } else {
+ $indentLevel++;
+ $arrayString .= $array['open'] . "\n"
+ . $this->processIndented($value, $array, $indentLevel)
+ . str_repeat(' ', --$indentLevel) . $array['close'] . ",\n";
+ }
+ } elseif (is_object($value)) {
+ $arrayString .= var_export($value, true) . ",\n";
+ } else {
+ $arrayString .= "'" . addslashes($value) . "',\n";
@DASPRiD Collaborator
DASPRiD added a note

How do namespaced classes turn out with this? I suppose that they'll end up like Zend\\Foo\\Bar, while they'd be likely more readable as Zend\Foo\Bar. Unit test please :)

The existing implementation turns them into Zend\\Foo\\Bar as well. :)

This could probably be done by checking for class_exists(...) on each of these so that we don't end up modifying non-namespaced strings that happen to contain a \\, which will likely slow things down (though perhaps not enough to matter).

The aim of this PR, though, was to improve indentation and structure for readability. If merging of this PR is contingent upon prettying up namespaced classes as well then I'll see about doing so sometime early next week. Otherwise feel free to create a separate issue and refer me to it if there's no other reason to hold this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/Writer/PhpArray.php
@@ -19,8 +24,48 @@ class PhpArray extends AbstractWriter
*/
public function processConfig(array $config)
{
- $arrayString = "<?php\n"
- . "return " . var_export($config, true) . ";\n";
+ $array = array(
@DASPRiD Collaborator
DASPRiD added a note

Probably rename to $arrayStyle or $arraySyntax to make it more clear?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/Writer/PhpArray.php
@@ -19,8 +24,48 @@ class PhpArray extends AbstractWriter
*/
public function processConfig(array $config)
{
- $arrayString = "<?php\n"
- . "return " . var_export($config, true) . ";\n";
+ $array = array(
+ 'open' => $this->useBracketArraySyntax ? '[' : 'array(',
+ 'close' => $this->useBracketArraySyntax ? ']' : ')'
+ );
+
+ return "<?php\n\n" .
+ "return " . $array['open'] . "\n" . $this->processIndented($config, $array) . $array['close'] . ";\n";
+ }
+
+ /**
+ * Sets whether or not to use the PHP 5.4+ "[]" array syntax.
+ *
+ * @param bool $value
@DASPRiD Collaborator
DASPRiD added a note

Please add @return void for consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michaelmoussa

Thanks for the attention and your comments @DASPRiD. If there's anything else I'll take of it over the next few days.

library/Zend/Config/Writer/PhpArray.php
@@ -19,8 +29,58 @@ class PhpArray extends AbstractWriter
*/
public function processConfig(array $config)
{
- $arrayString = "<?php\n"
- . "return " . var_export($config, true) . ";\n";
+ $arraySyntax = array(
+ 'open' => $this->useBracketArraySyntax ? '[' : 'array(',
+ 'close' => $this->useBracketArraySyntax ? ']' : ')'
+ );
+
+ return "<?php\n\n" .
@DASPRiD Collaborator
DASPRiD added a note

Why this double linebreak here? :)

Because whoops! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@DASPRiD DASPRiD commented on the diff
library/Zend/Config/Writer/PhpArray.php
((26 lines not shown))
+
+ /**
+ * Recursively processes a PHP config array structure into a readable format.
+ *
+ * @param array $config
+ * @param array $arraySyntax
+ * @param int $indentLevel
+ * @return string
+ */
+ protected function processIndented(array $config, array $arraySyntax, &$indentLevel = 1)
+ {
+ $arrayString = "";
+
+ foreach ($config as $key => $value) {
+ $arrayString .= str_repeat(self::INDENT_STRING, $indentLevel);
+ $arrayString .= (is_int($key) ? $key : "'" . addslashes($key) . "'") . ' => ';
@DASPRiD Collaborator
DASPRiD added a note

This should contain is_float() additionally.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/Writer/PhpArray.php
((5 lines not shown))
- . "return " . var_export($config, true) . ";\n";
+ $arraySyntax = array(
+ 'open' => $this->useBracketArraySyntax ? '[' : 'array(',
+ 'close' => $this->useBracketArraySyntax ? ']' : ')'
+ );
+
+ return "<?php\n" .
+ "return " . $arraySyntax['open'] . "\n" . $this->processIndented($config, $arraySyntax) .
+ $arraySyntax['close'] . ";\n";
+ }
+
+ /**
+ * Sets whether or not to use the PHP 5.4+ "[]" array syntax.
+ *
+ * @param bool $value
+ * @return void
@Maks3w Collaborator
Maks3w added a note

Add a fluent interface here (return $this)

Done. See updated PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Maks3w Maks3w commented on the diff
library/Zend/Config/Writer/PhpArray.php
((38 lines not shown))
+
+ foreach ($config as $key => $value) {
+ $arrayString .= str_repeat(self::INDENT_STRING, $indentLevel);
+ $arrayString .= (is_int($key) ? $key : "'" . addslashes($key) . "'") . ' => ';
+
+ if (is_array($value)) {
+ if ($value === array()) {
+ $arrayString .= $arraySyntax['open'] . $arraySyntax['close'] . ",\n";
+ } else {
+ $indentLevel++;
+ $arrayString .= $arraySyntax['open'] . "\n"
+ . $this->processIndented($value, $arraySyntax, $indentLevel)
+ . str_repeat(self::INDENT_STRING, --$indentLevel) . $arraySyntax['close'] . ",\n";
+ }
+ } elseif (is_object($value)) {
+ $arrayString .= var_export($value, true) . ",\n";
@Maks3w Collaborator
Maks3w added a note

I would like to see a unit test for this type of value. I think that the only safe way to export an object is serializing it.

@Maks3w Collaborator
Maks3w added a note

Forgive my dubts about the export format. However the tests should cover this type

Using var_export(...) for an object when it is encountered produces the same result as the existing implementation of the PhpArray writer (which just calls var_export(...) on the whole thing).

It uses the magic __set_state(...) static method which is described here. While using serialize is probably safer, changing to use it instead of var_export would break BC, as anybody presently using this config writer would not be expecting a serialized object.

That said, this is really a "bad practices edge case" IMO. Configuration files should contain configuration settings, not full-blown objects. This is only here for backwards compatibility.

I hope that all makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michaelmoussa

@DASPRiD or @Maks3w is there anything else you'd like to see done before this can be merged?

@michaelmoussa

Also, the broken Travis build is due to coding standards in files having nothing to do with what I changed. :)

@Maks3w Maks3w was assigned
@Maks3w Maks3w closed this
@gianarb gianarb referenced this pull request from a commit in zendframework/zend-config
@Maks3w Maks3w Merge pull request zendframework/zf2#5259 648b498
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
65 library/Zend/Config/Writer/PhpArray.php
@@ -12,6 +12,16 @@
class PhpArray extends AbstractWriter
{
/**
+ * @var string
+ */
+ const INDENT_STRING = ' ';
+
+ /**
+ * @var bool
+ */
+ protected $useBracketArraySyntax = false;
+
+ /**
* processConfig(): defined by AbstractWriter.
*
* @param array $config
@@ -19,8 +29,59 @@ class PhpArray extends AbstractWriter
*/
public function processConfig(array $config)
{
- $arrayString = "<?php\n"
- . "return " . var_export($config, true) . ";\n";
+ $arraySyntax = array(
+ 'open' => $this->useBracketArraySyntax ? '[' : 'array(',
+ 'close' => $this->useBracketArraySyntax ? ']' : ')'
+ );
+
+ return "<?php\n" .
+ "return " . $arraySyntax['open'] . "\n" . $this->processIndented($config, $arraySyntax) .
+ $arraySyntax['close'] . ";\n";
+ }
+
+ /**
+ * Sets whether or not to use the PHP 5.4+ "[]" array syntax.
+ *
+ * @param bool $value
+ * @return self
+ */
+ public function setUseBracketArraySyntax($value)
+ {
+ $this->useBracketArraySyntax = $value;
+ return $this;
+ }
+
+ /**
+ * Recursively processes a PHP config array structure into a readable format.
+ *
+ * @param array $config
+ * @param array $arraySyntax
+ * @param int $indentLevel
+ * @return string
+ */
+ protected function processIndented(array $config, array $arraySyntax, &$indentLevel = 1)
+ {
+ $arrayString = "";
+
+ foreach ($config as $key => $value) {
+ $arrayString .= str_repeat(self::INDENT_STRING, $indentLevel);
+ $arrayString .= (is_int($key) ? $key : "'" . addslashes($key) . "'") . ' => ';
@DASPRiD Collaborator
DASPRiD added a note

This should contain is_float() additionally.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ if (is_array($value)) {
+ if ($value === array()) {
+ $arrayString .= $arraySyntax['open'] . $arraySyntax['close'] . ",\n";
+ } else {
+ $indentLevel++;
+ $arrayString .= $arraySyntax['open'] . "\n"
+ . $this->processIndented($value, $arraySyntax, $indentLevel)
+ . str_repeat(self::INDENT_STRING, --$indentLevel) . $arraySyntax['close'] . ",\n";
+ }
+ } elseif (is_object($value)) {
+ $arrayString .= var_export($value, true) . ",\n";
@Maks3w Collaborator
Maks3w added a note

I would like to see a unit test for this type of value. I think that the only safe way to export an object is serializing it.

@Maks3w Collaborator
Maks3w added a note

Forgive my dubts about the export format. However the tests should cover this type

Using var_export(...) for an object when it is encountered produces the same result as the existing implementation of the PhpArray writer (which just calls var_export(...) on the whole thing).

It uses the magic __set_state(...) static method which is described here. While using serialize is probably safer, changing to use it instead of var_export would break BC, as anybody presently using this config writer would not be expecting a serialized object.

That said, this is really a "bad practices edge case" IMO. Configuration files should contain configuration settings, not full-blown objects. This is only here for backwards compatibility.

I hope that all makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ } else {
+ $arrayString .= "'" . addslashes($value) . "',\n";
@DASPRiD Collaborator
DASPRiD added a note

How do namespaced classes turn out with this? I suppose that they'll end up like Zend\\Foo\\Bar, while they'd be likely more readable as Zend\Foo\Bar. Unit test please :)

The existing implementation turns them into Zend\\Foo\\Bar as well. :)

This could probably be done by checking for class_exists(...) on each of these so that we don't end up modifying non-namespaced strings that happen to contain a \\, which will likely slow things down (though perhaps not enough to matter).

The aim of this PR, though, was to improve indentation and structure for readability. If merging of this PR is contingent upon prettying up namespaced classes as well then I'll see about doing so sometime early next week. Otherwise feel free to create a separate issue and refer me to it if there's no other reason to hold this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+ }
return $arrayString;
}
View
13 tests/ZendTest/Config/FactoryTest.php
@@ -177,13 +177,12 @@ public function testFactoryWriteToFile()
// build string line by line as we are trailing-whitespace sensitive.
$expected = "<?php\n";
- $expected .= "return array (\n";
- $expected .= " 'test' => 'foo',\n";
- $expected .= " 'bar' => \n";
- $expected .= " array (\n";
- $expected .= " 0 => 'baz',\n";
- $expected .= " 1 => 'foo',\n";
- $expected .= " ),\n";
+ $expected .= "return array(\n";
+ $expected .= " 'test' => 'foo',\n";
+ $expected .= " 'bar' => array(\n";
+ $expected .= " 0 => 'baz',\n";
+ $expected .= " 1 => 'foo',\n";
+ $expected .= " ),\n";
$expected .= ");\n";
$this->assertEquals(true, $result);
View
50 tests/ZendTest/Config/Writer/PhpArrayTest.php
@@ -31,21 +31,55 @@ public function setUp()
*/
public function testRender()
{
- $config = new Config(array('test' => 'foo', 'bar' => array(0 => 'baz', 1 => 'foo')));
+ $config = new Config(array(
+ 'test' => 'foo',
+ 'bar' => array(0 => 'baz', 1 => 'foo'),
+ 'emptyArray' => array(),
+ 'object' => (object) array('foo' => 'bar')
+ ));
$configString = $this->writer->toString($config);
// build string line by line as we are trailing-whitespace sensitive.
$expected = "<?php\n";
- $expected .= "return array (\n";
- $expected .= " 'test' => 'foo',\n";
- $expected .= " 'bar' => \n";
- $expected .= " array (\n";
- $expected .= " 0 => 'baz',\n";
- $expected .= " 1 => 'foo',\n";
- $expected .= " ),\n";
+ $expected .= "return array(\n";
+ $expected .= " 'test' => 'foo',\n";
+ $expected .= " 'bar' => array(\n";
+ $expected .= " 0 => 'baz',\n";
+ $expected .= " 1 => 'foo',\n";
+ $expected .= " ),\n";
+ $expected .= " 'emptyArray' => array(),\n";
+ $expected .= " 'object' => stdClass::__set_state(array(\n";
+ $expected .= " 'foo' => 'bar',\n";
+ $expected .= ")),\n";
$expected .= ");\n";
$this->assertEquals($expected, $configString);
}
+
+ public function testRenderWithBracketArraySyntax()
+ {
+ $config = new Config(array('test' => 'foo', 'bar' => array(0 => 'baz', 1 => 'foo'), 'emptyArray' => array()));
+
+ $this->writer->setUseBracketArraySyntax(true);
+ $configString = $this->writer->toString($config);
+
+ // build string line by line as we are trailing-whitespace sensitive.
+ $expected = "<?php\n";
+ $expected .= "return [\n";
+ $expected .= " 'test' => 'foo',\n";
+ $expected .= " 'bar' => [\n";
+ $expected .= " 0 => 'baz',\n";
+ $expected .= " 1 => 'foo',\n";
+ $expected .= " ],\n";
+ $expected .= " 'emptyArray' => [],\n";
+ $expected .= "];\n";
+
+ $this->assertEquals($expected, $configString);
+ }
+
+ public function testSetUseBracketArraySyntaxReturnsFluentInterface()
+ {
+ $this->assertSame($this->writer, $this->writer->setUseBracketArraySyntax(true));
+ }
}
Something went wrong with that request. Please try again.