Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

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

Closed
wants to merge 13 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
dc75eea
Modified PhpArray config writer to generate better readable array for…
michaelmoussa Oct 12, 2013
14260d6
Modified to build out formatted PHP code by hand for greater speed
michaelmoussa Oct 12, 2013
99fa6cb
Optional support for PHP 5.4+ "[]" array syntax.
michaelmoussa Oct 12, 2013
43bb44f
Using is_int() instead of is_numeric() to avoid unintentional loss of…
michaelmoussa Oct 12, 2013
0af6a9c
Using a constant for the four-space indent string
michaelmoussa Oct 12, 2013
fcfd2ce
Added docblock for ::processIndented(...)
michaelmoussa Oct 12, 2013
35589ef
s/$array/$arraySyntax/g for clarity
michaelmoussa Oct 12, 2013
e489dd3
Added @return void for consistency. Aligned docblock variable names
michaelmoussa Oct 12, 2013
a9524cd
Removed the extra line break
michaelmoussa Oct 12, 2013
fd208c3
Added missing test case
michaelmoussa Oct 15, 2013
b117306
Added Fluent interface for the bracket array syntax setter
michaelmoussa Oct 15, 2013
c2a1efa
Added missing test case
michaelmoussa Oct 15, 2013
a28df59
Did not mean to commit this - removing.
michaelmoussa Oct 15, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 62 additions & 2 deletions library/Zend/Config/Writer/PhpArray.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,16 @@

class PhpArray extends AbstractWriter
{
/**
* @var string
*/
const INDENT_STRING = ' ';

/**
* @var bool
*/
protected $useBracketArraySyntax = false;

/**
* processConfig(): defined by AbstractWriter.
*
Expand All @@ -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" .
"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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a fluent interface here (return $this)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. See updated PR

*/
public function setUseBracketArraySyntax($value)
{
$this->useBracketArraySyntax = $value;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add docblock.

/**
* 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) . "'") . ' => ';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should contain is_float() additionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
$arrayString .= "'" . addslashes($value) . "',\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

return $arrayString;
}
Expand Down
13 changes: 6 additions & 7 deletions tests/ZendTest/Config/FactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
33 changes: 26 additions & 7 deletions tests/ZendTest/Config/Writer/PhpArrayTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,34 @@ public function testRender()

// 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($expected, $configString);
}

public function testRenderWithBracketArraySyntax()
{
$config = new Config(array('test' => 'foo', 'bar' => array(0 => 'baz', 1 => 'foo')));

$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 .= "];\n";

$this->assertEquals($expected, $configString);
}
}