Make the CS stricter; fix some bugs / issues by teohhanhui · Pull Request #1007 · api-platform/core

teohhanhui · 2017-03-24T10:15:18Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	N/A

Stricter CS = better code, especially declare(strict_types=1);

soyuka · 2017-03-24T10:42:32Z

tests/Bridge/Doctrine/Orm/Filter/DateFilterTest.php

            'after' => [
                [
-                    'dummyDate' => true,
+                    'dummyDate' => null,


why the change here?

Because this is $nullManagement, which can only be string or null, not bool.

soyuka · 2017-03-24T10:44:49Z

.php_cs.dist

        'ordered_imports' => true,
+        // 'phpdoc_add_missing_param_annotation' => [
+        //     'only_untyped' => false,
+        // ],


should we remove those?

I plan to get these in next... 😄

@dunglas I want to get this in too... But this is gonna require quite a bit of manual work. 😆

teohhanhui · 2017-03-24T10:55:28Z

.php_cs.dist

+        'modernize_types_casting' => true,
+        // 'native_function_invocation' => true,
+        'no_extra_consecutive_blank_lines' => [
+            'break',


This one may be a bit controversial, but I think it's good that it encourages us to not have complex logic nested inside a switch-case, or even avoid switch-case altogether.

I agree with the intent, but it makes the code (even simple statements) less readable (IMO).

Yeah, personally I prefer the blank line too, but for the sake of consistency...

Consistency with what?

We cannot enforce a blank line before/after break;, but we can enforce break; always sticking to the next case:

switch ($str) { case 'A': $this->doSomething(); $this->doSomethingElse(); break; case 'B': $this->doSomethingAwesome(); break; }

But we can ignore this rule too and check it manually (it's not a big deal after all).

Do you need me to revert this? I'll have to do it manually...

Don't worry about that, keep it as is.

dunglas · 2017-03-27T20:45:26Z

src/Bridge/Symfony/Routing/RouteNameResolver.php

            $methods = $route->getMethods();

-            if ($resourceClass === $currentResourceClass && null !== $operation && (empty($methods) || in_array('GET', $methods))) {
+            if ($resourceClass === $currentResourceClass && null !== $operation && (empty($methods) || in_array('GET', $methods, true))) {


The call to empty can be replaced by the ! operator.

I really don't like such implicit conversions... And I don't like empty too...

My preference would be 0 === count($methods), but I understand there is a performance overhead (how much?) for the function call.

I prefer to keep in sync with what we do in Symfony (we remove empty, count and so on).

dunglas · 2017-03-27T20:49:08Z

Awesome. Can you just fix StyleCI ? Putting the declare line before the license header should do the trick.

teohhanhui · 2017-03-28T06:48:26Z

Can you just fix StyleCI ? Putting the declare line before the license header should do the trick.

No. StyleCI is buggy here. See PHP-CS-Fixer/PHP-CS-Fixer#2590

Also, declare should go after file-level docblock, according to PSR-12 draft.

dunglas · 2017-03-28T11:36:29Z

So what do we do in the meantime? Disable StyleCI? Or disable the header check?

teohhanhui · 2017-03-28T11:43:49Z

Disabling the header check sounds reasonable. But it might escape our watchful eyes. 😄

dunglas · 2017-03-28T11:49:32Z

We'll restore the check when PHP CS Fixer will be updated.

dunglas · 2017-04-01T07:17:30Z

StyleCI config updated. Can you rebase?

teohhanhui · 2017-04-03T05:10:59Z

.php_cs.dist

+        ],
+        'declare_strict_types' => true,
+        'modernize_types_casting' => true,
+        // 'native_function_invocation' => true,


What do you guys think about this? I remember @dunglas talking about this before... We'll need to do Blackfire profiling for this to see the performance difference.

teohhanhui · 2017-04-03T05:17:53Z

Rebased. But StyleCI still has a problem with return null; when Symfony has already changed its CS in view of the introduction of the void return type. I suspect they are using outdated PHP-CS-Fixer? 😞

teohhanhui · 2017-04-03T05:19:39Z

It could make sense for us to disable StyleCI (at least for now?), because frankly I don't see what's the added value of having it.

dunglas · 2017-04-03T11:49:32Z

Are you sure Symfony has changed this CS?

dunglas · 2017-04-03T11:51:56Z

Anyway StyleCI is useless since we run our own PHP CS Fixer instance. I've just disabled it. Can you rebase again?

soyuka · 2017-04-03T11:53:44Z

Styleci would make sense if it prevented the other CI from running on failure.

teohhanhui · 2017-04-03T11:54:45Z

Yes, since symfony/symfony-docs#6614

Use return null; when a function explicitly returns null values and use return; when the function returns void values;

http://symfony.com/doc/current/contributing/code/standards.html

teohhanhui · 2017-04-03T11:55:49Z

The only merit I see for StyleCI is the nice output, and the ability to send a PR. 😆

dunglas · 2017-04-07T10:19:06Z

Thank you @teohhanhui

teohhanhui added enhancement [RFR] Ready for Review labels Mar 24, 2017

teohhanhui requested review from dunglas and soyuka March 24, 2017 10:15

soyuka reviewed Mar 24, 2017

View reviewed changes

teohhanhui force-pushed the feature/make-cs-strict-again branch from 4941c42 to 9d18991 Compare March 24, 2017 10:53

teohhanhui commented Mar 24, 2017

View reviewed changes

dunglas reviewed Mar 27, 2017

View reviewed changes

teohhanhui force-pushed the feature/make-cs-strict-again branch from 9d18991 to b4b3cc4 Compare April 3, 2017 05:07

teohhanhui commented Apr 3, 2017

View reviewed changes

teohhanhui force-pushed the feature/make-cs-strict-again branch from b4b3cc4 to b80f208 Compare April 3, 2017 05:15

teohhanhui added 3 commits April 3, 2017 19:57

Make CS stricter

bee3f28

Enable declare_strict_types in PHP-CS-Fixer

d9bd458

Install PHP-CS-Fixer v2.1.2 on Travis CI

766d106

teohhanhui force-pushed the feature/make-cs-strict-again branch from b80f208 to 766d106 Compare April 3, 2017 11:57

dunglas merged commit 8c0664a into api-platform:2.0 Apr 7, 2017

teohhanhui deleted the feature/make-cs-strict-again branch April 10, 2017 04:56

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Make the CS stricter; fix some bugs / issues (api-platform#1007)

d1ca483

Uh oh!

Comments

Conversation

teohhanhui commented Mar 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Mar 27, 2017

Uh oh!

teohhanhui commented Mar 28, 2017

Uh oh!

dunglas commented Mar 28, 2017

Uh oh!

teohhanhui commented Mar 28, 2017

Uh oh!

dunglas commented Mar 28, 2017

Uh oh!

dunglas commented Apr 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui commented Apr 3, 2017

Uh oh!

teohhanhui commented Apr 3, 2017

Uh oh!

dunglas commented Apr 3, 2017

Uh oh!

dunglas commented Apr 3, 2017

Uh oh!

soyuka commented Apr 3, 2017

Uh oh!

teohhanhui commented Apr 3, 2017

Uh oh!

teohhanhui commented Apr 3, 2017

Uh oh!

dunglas commented Apr 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

teohhanhui Mar 28, 2017 •

edited

Loading