Skip to content
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.

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

Format generated config files using the short array syntax #12499

Merged

Conversation

cykirsch
Copy link
Contributor

@cykirsch cykirsch commented Nov 30, 2017

Description

  1. Magento is following a standard of new code using short array syntax (Coding standards: arrays #758).
  2. Magento has an automated checker to enforce the short array syntax precommit (https://github.com/magento/magento2/blob/2.2/.php_cs.dist#L31).
  3. Magento recommends checking in app/etc/config.php (http://devdocs.magento.com/guides/v2.2/config-guide/config/config-php.html)

Check this file in to source control and use it in your development, staging, and production systems.

  1. Magento's auto-generated app/etc/config.php uses the long array syntax.

Put that all together and the auto-generated config doesn't match the required standards to be checked in. The change made in this PR is to modify PhpFormatter to recursively return the array representation using short array syntax [] instead of long array(). If the given variable is not an array, it uses the standard var_export behavior.

Manual testing scenarios

  1. bin/magento setup:upgrade
  2. app/etc/config.php should use the short array syntax

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@cykirsch
Copy link
Contributor Author

As an added note, I did attempt to do this using Zend\Code\Generator\ValueGenerator instead of var_export, but the array depth wasn't being tracked properly. The unit test array would look like this

[
  'ns1' => [
        's1' => [
            's11',
            's12',
        ],
        's2' => [
            's21',
            's22',
        ],
    ],
  'ns2' => [
        's1' => [
            's11',
        ],
    ],
  'ns3' => 'just text',
  'ns4' => 'just text',
]

{
if (gettype($var) === 'array') {
$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.

* @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)

*/
private function varExportShort($var, $depth = 0)
{
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
...

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

- const documenation
- reverse condition for readability
- better variable name
- type and return hints
@okorshenko okorshenko merged commit e66bea8 into magento:2.2-develop Dec 1, 2017
@cykirsch
Copy link
Contributor Author

@ishakhsuvarov @okorshenko I was more or less expecting these changes would be in 2.2.2. Is it odd to you that it is on the 2.2.2-preview branch but not on the 2.2.2 tag? Or does that happen often?

@magento-engcom-team magento-engcom-team added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Dec 13, 2017
@clemblanco
Copy link

clemblanco commented Mar 26, 2018

What if we want 4 spaces tab indentation? Shouldn't this follow a PSR?

@cykirsch
Copy link
Contributor Author

@clementblanco That did cross my mind, and I would prefer it as well, but I neglected to do it...I think because I was continuing to use var_export and I didn't want to mix 4 and 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Progress: accept Release Line: 2.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants