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

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
4 participants
@michaelmoussa
Contributor

michaelmoussa commented Oct 12, 2013

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.

@DASPRiD

View changes

library/Zend/Config/Writer/PhpArray.php
+
+ if ($line === '),' || $line === ')') {
+ $indentLevel--;
+ } else if (preg_match('/^\s*array \(/', $line)) {

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

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

@DASPRiD

DASPRiD Oct 12, 2013

Member

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

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

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

DASPRiD Oct 12, 2013

Member

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.

This comment has been minimized.

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

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

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

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

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

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

@DASPRiD

DASPRiD Oct 12, 2013

Member

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

@DASPRiD

View changes

library/Zend/Config/Writer/PhpArray.php
+
+ foreach ($config as $key => $value) {
+ $arrayString .= str_repeat(' ', $indentLevel);
+ $arrayString .= (is_numeric($key) ? $key : "'" . addslashes($key) . "'") . ' => ';

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

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

@DASPRiD

DASPRiD Oct 12, 2013

Member

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

This comment has been minimized.

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

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.

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

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.

@DASPRiD

View changes

library/Zend/Config/Writer/PhpArray.php
+ $arrayString = "";
+
+ foreach ($config as $key => $value) {
+ $arrayString .= str_repeat(' ', $indentLevel);

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

I suggest to make the indent-string a constant.

@DASPRiD

DASPRiD Oct 12, 2013

Member

I suggest to make the indent-string a constant.

@DASPRiD

View changes

library/Zend/Config/Writer/PhpArray.php
+ . str_repeat(' ', --$indentLevel) . $array['close'] . ",\n";
+ }
+ } elseif (is_object($value)) {
+ $arrayString .= var_export($value, true) . ",\n";

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

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

@DASPRiD

DASPRiD Oct 12, 2013

Member

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

This comment has been minimized.

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

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?

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

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?

@DASPRiD

View changes

library/Zend/Config/Writer/PhpArray.php
+
+ if (is_array($value)) {
+ if ($value === array()) {
+ $arrayString .= $array['open'] . $array['close'] . ",\n";

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

This part is not covered by a unit test.

@DASPRiD

DASPRiD Oct 12, 2013

Member

This part is not covered by a unit test.

This comment has been minimized.

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

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.

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

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.

@DASPRiD

View changes

library/Zend/Config/Writer/PhpArray.php
+ {
+ $this->useBracketArraySyntax = $value;
+ }
+

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

Add docblock.

@DASPRiD

DASPRiD Oct 12, 2013

Member

Add docblock.

+ } elseif (is_object($value)) {
+ $arrayString .= var_export($value, true) . ",\n";
+ } else {
+ $arrayString .= "'" . addslashes($value) . "',\n";

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

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 :)

@DASPRiD

DASPRiD Oct 12, 2013

Member

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 :)

This comment has been minimized.

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

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.

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

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.

@DASPRiD

View changes

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(

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

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

@DASPRiD

DASPRiD Oct 12, 2013

Member

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

@DASPRiD

View changes

library/Zend/Config/Writer/PhpArray.php
+ /**
+ * Sets whether or not to use the PHP 5.4+ "[]" array syntax.
+ *
+ * @param bool $value

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

Please add @return void for consistency.

@DASPRiD

DASPRiD Oct 12, 2013

Member

Please add @return void for consistency.

@michaelmoussa

This comment has been minimized.

Show comment
Hide comment
@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

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

Contributor

michaelmoussa commented Oct 12, 2013

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

@DASPRiD

View changes

library/Zend/Config/Writer/PhpArray.php
+ 'close' => $this->useBracketArraySyntax ? ']' : ')'
+ );
+
+ return "<?php\n\n" .

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

Why this double linebreak here? :)

@DASPRiD

DASPRiD Oct 12, 2013

Member

Why this double linebreak here? :)

This comment has been minimized.

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

Because whoops! :)

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

Because whoops! :)

+
+ foreach ($config as $key => $value) {
+ $arrayString .= str_repeat(self::INDENT_STRING, $indentLevel);
+ $arrayString .= (is_int($key) ? $key : "'" . addslashes($key) . "'") . ' => ';

This comment has been minimized.

@DASPRiD

DASPRiD Oct 12, 2013

Member

This should contain is_float() additionally.

@DASPRiD

DASPRiD Oct 12, 2013

Member

This should contain is_float() additionally.

This comment has been minimized.

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

See zendframework#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.

@michaelmoussa

michaelmoussa Oct 12, 2013

Contributor

See zendframework#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.

@Maks3w

View changes

library/Zend/Config/Writer/PhpArray.php
+ * Sets whether or not to use the PHP 5.4+ "[]" array syntax.
+ *
+ * @param bool $value
+ * @return void

This comment has been minimized.

@Maks3w

Maks3w Oct 15, 2013

Member

Add a fluent interface here (return $this)

@Maks3w

Maks3w Oct 15, 2013

Member

Add a fluent interface here (return $this)

This comment has been minimized.

@michaelmoussa

michaelmoussa Oct 15, 2013

Contributor

Done. See updated PR

@michaelmoussa

michaelmoussa Oct 15, 2013

Contributor

Done. See updated PR

+ . str_repeat(self::INDENT_STRING, --$indentLevel) . $arraySyntax['close'] . ",\n";
+ }
+ } elseif (is_object($value)) {
+ $arrayString .= var_export($value, true) . ",\n";

This comment has been minimized.

@Maks3w

Maks3w Oct 15, 2013

Member

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

Maks3w Oct 15, 2013

Member

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.

This comment has been minimized.

@Maks3w

Maks3w Oct 15, 2013

Member

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

@Maks3w

Maks3w Oct 15, 2013

Member

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

This comment has been minimized.

@michaelmoussa

michaelmoussa Oct 15, 2013

Contributor

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.

@michaelmoussa

michaelmoussa Oct 15, 2013

Contributor

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.

@michaelmoussa

This comment has been minimized.

Show comment
Hide comment
@michaelmoussa

michaelmoussa Oct 15, 2013

Contributor

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

Contributor

michaelmoussa commented Oct 15, 2013

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

@michaelmoussa

This comment has been minimized.

Show comment
Hide comment
@michaelmoussa

michaelmoussa Oct 15, 2013

Contributor

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

Contributor

michaelmoussa commented Oct 15, 2013

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

@ghost ghost assigned Maks3w Oct 20, 2013

Maks3w added a commit that referenced this pull request Oct 20, 2013

@Maks3w Maks3w closed this Oct 20, 2013

gianarb pushed a commit to zendframework/zend-config that referenced this pull request May 15, 2015

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