Skip to content

Format generated config files using the short array syntax #12499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 1, 2017
Merged

Format generated config files using the short array syntax #12499

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe7e75f
Format generated config files using the short array syntax
cykirsch Nov 30, 2017
4597c4f
Fix the config formatter scenarios with $comments to also use short a…
cykirsch Nov 30, 2017
ed2886f
Fix code style issues
cykirsch Nov 30, 2017
e66bea8
Fixes per ishakhsuvarov code review:
cykirsch Dec 1, 2017
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
38 changes: 32 additions & 6 deletions lib/internal/Magento/Framework/App/DeploymentConfig/Writer/PhpFormatter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
*/
class PhpFormatter implements FormatterInterface
{
const INDENT = ' ';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a description for the constant (docBlock)


/**
* Format deployment configuration.
* If $comments is present, each item will be added
Expand All @@ -21,9 +23,9 @@ class PhpFormatter implements FormatterInterface
public function format($data, array $comments = [])
{
if (!empty($comments) && is_array($data)) {
return "<?php\nreturn array (\n" . $this->formatData($data, $comments) . "\n);\n";
return "<?php\nreturn [\n" . $this->formatData($data, $comments) . "\n];\n";
}
return "<?php\nreturn " . var_export($data, true) . ";\n";
return "<?php\nreturn " . $this->varExportShort($data, true) . ";\n";
}

/**
Expand Down Expand Up @@ -51,18 +53,42 @@ private function formatData($data, $comments = [], $prefix = ' ')
$elements[] = $prefix . " */";
}

$elements[] = $prefix . var_export($key, true) . ' => ' .
(!is_array($value) ? var_export($value, true) . ',' : '');
$elements[] = $prefix . $this->varExportShort($key) . ' => ' .
(!is_array($value) ? $this->varExportShort($value) . ',' : '');

if (is_array($value)) {
$elements[] = $prefix . 'array (';
$elements[] = $prefix . '[';
$elements[] = $this->formatData($value, [], ' ' . $prefix);
$elements[] = $prefix . '),';
$elements[] = $prefix . '],';
}
}
return implode("\n", $elements);
}

return var_export($data, true);
}

/**
* If variable to export is an array, format with the php >= 5.4 short array syntax. Otherwise use
* default var_export functionality.
*
* @param mixed $var
* @param integer $depth
* @return string
*/
private function varExportShort($var, $depth = 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please take advantage of PHP 7.0 syntax (type hints and return types for methods)

{
if (gettype($var) === 'array') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any specific reason not to use is_array?
Additionally, readability may be slightly improved by reversing the logic like

if (!is_array($var)) {
    return var_export($var, true);
}
/// Transforming to short array syntax here
...

$indexed = array_keys($var) === range(0, count($var) - 1);
$r = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid variables with unclear names. $r does not really describe what is it for.

foreach ($var as $key => $value) {
$r[] = str_repeat(self::INDENT, $depth)
. ($indexed ? '' : $this->varExportShort($key) . ' => ')
. $this->varExportShort($value, $depth + 1);
}
return sprintf("[\n%s\n%s]", implode(",\n", $r), str_repeat(self::INDENT, $depth - 1));
}

return var_export($var, true);
}
}
74 changes: 49 additions & 25 deletions lib/internal/Magento/Framework/App/Test/Unit/DeploymentConfig/Writer/PhpFormatterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,68 +55,68 @@ public function formatWithCommentDataProvider()
];
$expectedResult1 = <<<TEXT
<?php
return array (
return [
'ns1' =>
array (
[
's1' =>
array (
[
0 => 's11',
1 => 's12',
),
],
's2' =>
array (
[
0 => 's21',
1 => 's22',
),
),
],
],
/**
* For the section: ns2
* comment for namespace 2
*/
'ns2' =>
array (
[
's1' =>
array (
[
0 => 's11',
),
),
],
],
'ns3' => 'just text',
'ns4' => 'just text',
);
];

TEXT;
$expectedResult2 = <<<TEXT
<?php
return array (
return [
/**
* For the section: ns1
* comment for' namespace 1
*/
'ns1' =>
array (
[
's1' =>
array (
[
0 => 's11',
1 => 's12',
),
],
's2' =>
array (
[
0 => 's21',
1 => 's22',
),
),
],
],
/**
* For the section: ns2
* comment for namespace 2.
* Next comment for' namespace 2
*/
'ns2' =>
array (
[
's1' =>
array (
[
0 => 's11',
),
),
],
],
/**
* For the section: ns3
* comment for" namespace 3
Expand All @@ -127,15 +127,39 @@ public function formatWithCommentDataProvider()
* comment for namespace 4
*/
'ns4' => 'just text',
);
];

TEXT;

$expectedResult3 = <<<TEXT
<?php
return [
'ns1' => [
's1' => [
's11',
's12'
],
's2' => [
's21',
's22'
]
],
'ns2' => [
's1' => [
's11'
]
],
'ns3' => 'just text',
'ns4' => 'just text'
];

TEXT;
return [
['string', [], "<?php\nreturn 'string';\n"],
['string', ['comment'], "<?php\nreturn 'string';\n"],
[$array, [], "<?php\nreturn " . var_export($array, true) . ";\n"],
[$array, $comments1, $expectedResult1],
[$array, $comments2, $expectedResult2],
[$array, [], $expectedResult3],
];
}
}