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

PHP CS Fix #216

Closed
wants to merge 11 commits into from
Closed

PHP CS Fix #216

wants to merge 11 commits into from

Conversation

charescape
Copy link

No description provided.

@charescape
Copy link
Author

This PR related to #215

@codecov
Copy link

codecov bot commented Sep 21, 2019

Codecov Report

Merging #216 into master will not change coverage.
The diff coverage is 92.39%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #216   +/-   ##
=========================================
  Coverage     93.26%   93.26%           
  Complexity      466      466           
=========================================
  Files            77       77           
  Lines          1425     1425           
=========================================
  Hits           1329     1329           
  Misses           96       96
Impacted Files Coverage Δ Complexity Δ
src/Http/Diactoros/StreamFactory.php 70% <ø> (ø) 3 <0> (ø) ⬇️
src/Http/Diactoros/ResponseFactory.php 100% <ø> (ø) 3 <0> (ø) ⬇️
src/Http/Middleware/SessionMiddleware.php 100% <ø> (ø) 13 <0> (ø) ⬇️
src/Http/Middleware/JsonPayloadMiddleware.php 100% <ø> (ø) 4 <0> (ø) ⬇️
src/Bootloader/Http/RouterBootloader.php 100% <ø> (ø) 1 <0> (ø) ⬇️
src/GRPC/ServiceLocator.php 92.3% <ø> (ø) 6 <0> (ø) ⬇️
src/Http/ErrorHandler/PlainRenderer.php 100% <ø> (ø) 2 <0> (ø) ⬇️
src/Console/CommandLocator.php 90% <ø> (ø) 4 <0> (ø) ⬇️
src/Bootloader/SnapshotsBootloader.php 100% <ø> (ø) 1 <0> (ø) ⬇️
src/Http/Diactoros/UriFactory.php 100% <ø> (ø) 1 <0> (ø) ⬇️
... and 67 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0d10b2...bbd4980. Read the comment docs.

@wolfy-j
Copy link
Member

wolfy-j commented Sep 21, 2019

Hi, can I get your cs settings or script? There are few corrections I have to make.

@charescape
Copy link
Author

<?php

declare(strict_types=1);

return PhpCsFixer\Config::create()
    ->setCacheFile(__DIR__ . '/php_cs.cache')
    ->setRiskyAllowed(true)
    ->setRules([
        '@Symfony' => true,
        '@Symfony:risky' => true,
        '@PHP71Migration' => true,
        '@PHP71Migration:risky' => true,
        '@PHPUnit60Migration:risky' => true,
        'trailing_comma_in_multiline_array' => false,
        'native_constant_invocation' => false,
        'native_function_invocation' => false,
        'protected_to_private' => false,
        'no_alias_functions' => false,
        'set_type_to_cast' => false,
        'phpdoc_inline_tag' => false,
        'function_to_constant' => false,
        'self_accessor' => false,
        'increment_style' => false,
        'phpdoc_var_without_name' => false,
        'binary_operator_spaces' => [
            'default' => 'align',
        ],
        'array_syntax' => [
            'syntax' => 'short',
        ],
        'is_null' => false,
        'cast_spaces' => [
            'space' => 'none',
        ],
        'declare_strict_types' => true,
        'ordered_imports' => [
            'sortAlgorithm' => 'alpha',
            'importsOrder' => [
                'const',
                'function',
                'class',
            ],
        ],
        'no_useless_else' => true,
        'no_useless_return' => true,
        'php_unit_construct' => true,
        'php_unit_strict' => true,
        'phpdoc_separation' => false,
        'concat_space' => [
            'spacing' => 'one'
        ],
        'phpdoc_align' => [
            'tags' => []
        ],
        'phpdoc_annotation_without_dot' => true,
        'phpdoc_no_alias_tag' => false,
        'yoda_style' => false,
        'phpdoc_to_comment' => false,
    ])
    ->setFinder(
        PhpCsFixer\Finder::create()
            ->in(__DIR__)
            ->exclude([
                'vendor',
                'bin',
                'tests',
            ])
            ->notPath('functions.php')
    );

@charescape
Copy link
Author

@wolfy-j I followed https://cs.symfony.com/ and created .php_cs file, then run php php-cs-fixer-v2.phar fix

@charescape
Copy link
Author

charescape commented Sep 21, 2019

To avoid changing too many code styles, I disabled some rules, although I think they make sense.

@wolfy-j
Copy link
Member

wolfy-j commented Sep 22, 2019

Thank you, considering the impact and 60+ repos we have to apply to I can't guarantee quick merge. I need to talk to all the teams to make sure that CS is aligned.

@wolfy-j
Copy link
Member

wolfy-j commented Oct 5, 2019

FYI, we did not forget about it, @alexndr-novikov is working on the full-size analysis of the best approach.

@talentant
Copy link

Thank you for your excellent work. 👍

@alexndr-novikov
Copy link
Member

Thank you @charescape , we highly appreciate your contribution.

Unfortunately, this PR requires some modifications to be merged. It does not fit PSR-12, and has some changes that are not required by it, but do some unnecessary code changes. (like php-cs-fix 'binary_operator_spaces' alignment with '=' sign and etc.)
Also it has conflicts with current codebase.

These's one problem : php-cs-fixer does not support PSR-12 yet (it is expected in v3, PHP-CS-Fixer/PHP-CS-Fixer#4502), so now you'll need 2 tools to fix the code in proper way and check it: code sniffer that already supports PSR-12 checks, and PHP-CS-Fixer.

So, please follow the instruction to update this PR (or optionally close this one, re-fork and apply from scratch and then create new one)

  1. Add to composer.json libraries:
composer require --dev  friendsofphp/php-cs-fixer:^v2.15.3
composer require --dev squizlabs/php_codesniffer:^3.5.0
  1. add .php_cs file to the root of the repo with the following content: (and attach it to PR later)
<?php

declare(strict_types=1);

return PhpCsFixer\Config::create()
    ->setCacheFile(__DIR__ . '/.php_cs.cache')
    ->setRiskyAllowed(true)
    ->setRules([
        '@PSR2' => true,
        'binary_operator_spaces' => [
            'default' => null,
            'operators' => [
                '|' => 'single_space',
                '!==' => 'single_space',
                '!=' => 'single_space',
                '==' => 'single_space',
                '===' => 'single_space',
            ]
        ],
        'ordered_class_elements' => true,
        'trailing_comma_in_multiline_array' => false,
        'declare_strict_types' => true,
        'linebreak_after_opening_tag' => true,
        'blank_line_after_opening_tag' => true,
        'single_quote' => true,
        'lowercase_cast' => true,
        'short_scalar_cast' => true,
        'no_leading_import_slash' => true,
        'declare_equal_normalize' => [
            'space' => 'none'
        ],
        'new_with_braces' => true,
        'no_blank_lines_after_phpdoc' => true,
        'single_blank_line_before_namespace' => true,
        'visibility_required' => ['property', 'method', 'const'],
        'ternary_operator_spaces' => true,
        'unary_operator_spaces' => true,
        'return_type_declaration' => true,
        'concat_space' => [
            'spacing' => 'one'
        ],
        'no_useless_else' => true,
        'no_useless_return' => true,
        'phpdoc_separation' => false,
        'yoda_style' => false,
        'void_return' => true,
    ])
    ->setFinder(
        PhpCsFixer\Finder::create()
            ->in(__DIR__)
            ->exclude([
                'vendor',
                'bin',
            ])
    );
  1. Run
./vendor/bin/php-cs-fixer fix
./vendor/bin/phpcbf --report=code --standard=PSR12 --colors tests src
./vendor/bin/phpcs -n --report=code --standard=PSR12 --colors tests src

If last check still alert any issues (usually about class docblock and declare order and blank lines) - fix it manually please

  1. Add to .travis.yml on line 30

vendor/bin/phpcs -n --report=code --standard=PSR12 --colors tests src

And then update the pull request.

Let me know if you have any questions about it

@charescape
Copy link
Author

@alexndr-novikov Thank you very much. I am going to follow your instruction and update this PR soon.

@charescape
Copy link
Author

  1. Add to composer.json libraries

done.

  1. add .php_cs file to the root of the repo with the following content: (and attach it to PR later)

done.

  1. Run auto fix

done.

  1. Do manually fix

done.

  1. Add to .travis.yml on line 30

done.

Copy link
Member

@alexndr-novikov alexndr-novikov left a comment

Choose a reason for hiding this comment

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

Almost perfect, need a minor change in .travis.yml, commented it separately

.travis.yml Outdated Show resolved Hide resolved
@wolfy-j
Copy link
Member

wolfy-j commented Oct 11, 2019

Hi,

we have implemented your standards as separate package. Unfortunately, it now causes conflicts on this merge. I'll close it.

@wolfy-j wolfy-j closed this Oct 11, 2019
@wolfy-j
Copy link
Member

wolfy-j commented Oct 11, 2019

@charescape if you want you can use spiral/code-style by yourself, so the PR will remain after you.

@wolfy-j wolfy-j reopened this Oct 11, 2019
@wolfy-j
Copy link
Member

wolfy-j commented Oct 11, 2019

@charescape vendon/bin/spiral-cs fix src tests functions.php

@charescape
Copy link
Author

@wolfy-j Thank you! I will try to update this PR today.

@wolfy-j
Copy link
Member

wolfy-j commented Oct 11, 2019

I'll push an update with enabled CS to it. Travis will fail for now, so we will see how the CS will work.

@alexndr-novikov
Copy link
Member

@charescape
so the flow should be like

  1. composer require --dev spiral/code-style
  2. ./vendor/bin/spiral-cs fix functions.php src/ tests
  3. ./vendor/bin/spiral-cs check functions.php src/ tests
  4. fix the rest issues manually (declare order, ini_set('display_errors', '1');, etc)

@charescape
Copy link
Author

@alexndr-novikov Thanks for your instruction. I will do it soon.

@wolfy-j
Copy link
Member

wolfy-j commented Oct 11, 2019

Hi, we are prepping big release, when do you think we can merge it? :|

@charescape
Copy link
Author

@wolfy-j @alexndr-novikov Sorry, I'm still busy at the moment, and I don't want to delay the progress of your project, so this PR should be closed.

@charescape charescape closed this Oct 11, 2019
@wolfy-j
Copy link
Member

wolfy-j commented Oct 11, 2019

Sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants