Add phpcs to build and apply automatic fixes #1743

alcaeus · 2018-02-09T07:55:56Z

Part of #1702. To help with reviews, this PR only applies automatic fixes and disables all other sniffs for later fixing.

Note: some tests fail, these will have to be fixed. Most of them are due to strict typing.

Note: if you want to expand all diffs by default (at your own risk), you can copy the following snippet into the dev console while on the "Files changed" tab:

document.querySelectorAll('.load-diff-button').forEach(item => item.click())

malarzm · 2018-02-09T08:59:05Z

lib/Doctrine/ODM/MongoDB/Aggregation/Builder.php

@@ -200,7 +205,7 @@ public function facet()
     * @see http://docs.mongodb.org/manual/reference/operator/aggregation/geoNear/
     *
     * @param float|array|Point $x
-     * @param float $y
+     * @param float             $y


We have a rule to align params?

Apparently. Personally, I dislike it but I tried to not deviate too much from our official coding standard (except for space before colon because what kind of monster would want that?!?)

except for space before colon because what kind of monster would want that?!?

Me for example. 🐲

malarzm · 2018-02-09T09:00:30Z

lib/Doctrine/ODM/MongoDB/Aggregation/Stage/AddFields.php

 namespace Doctrine\ODM\MongoDB\Aggregation\Stage;

 /**
 * Fluent interface for adding a $addFields stage to an aggregation pipeline.
 *
- * @author Boris Guéry <guery.b@gmail.com>


Nitpicking, but empty line above should be removed

@kukulich might know if that's easily added to the sniff in question - if not I'd leave it until the sniff can deal with it.

It's not the only place, is there any way to configure CS fixer to disallow empty lines in comments when it's last line?

EDIT: or first line as well

I'll improve/fix the fixer. I hope it will be done today in next bugfix version.

No rush - we'll have plenty more pull requests until we're done fixing phpcs in this project. Thank you for your help!

Have experienced that one in doctrine/doctrine2#7046 as well

You can try dev-master of Slevomat CS.

Hmm, been fixing these empty lines manually... :| You have it too easy @alcaeus. :P

@kukulich Btw same applies to TypeHintDeclaration sniff when @param/@return is removed. That really sounds like a candidate for DocCommentSpacingSniff? 😊

Yes, new sniff is the best solution. It should be easy so maybe in the weekend or next week :)

malarzm · 2018-02-09T09:08:26Z

lib/Doctrine/ODM/MongoDB/Event/PreUpdateEventArgs.php

-     * @param array           $changeSet
+     *
+     * @param object $document
+     * @param array  $changeSet


This is not treated as redundant with method definition?

Interesting, seems to be a bug. I'll revisit those anyways when requiring 7.2 and introducing object typehint along with scalar typehints.

object is not required because of enableObjectTypeHint option that is enabled by default only if you run phpcs on PHP 7.2+.

array is ok because there is <exclude name="SlevomatCodingStandard.TypeHints.TypeHintDeclaration.MissingTraversableParameterTypeHintSpecification" /> in phpcs.xml.

malarzm · 2018-02-09T09:16:32Z

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataFactory.php

     *
-     * @return void


This got removed even though there's no : void in the definition, is it desirable? I mean will some tool later infer that there's no return in the function so : void should be added?

On the other hand next methods don't have @return void at all so I guess it's not a big deal

malarzm · 2018-02-09T09:21:37Z

lib/Doctrine/ODM/MongoDB/MongoDBException.php

     * @return MongoDBException
     */
    public static function invalidValueForType($type, $expected, $got)
    {
        if (is_array($expected)) {
-            $expected = sprintf('%s or %s',
+            $expected = sprintf(
+                '%s or %s',
                join(', ', array_slice($expected, 0, -1)),


Forbidden functions are not changed automatically or it's not that version of our CS yet?

It doesn't fix them automatically (it allows free configuration, so function signatures may vary), which is why it's not fixed in this PR.

malarzm · 2018-02-09T09:28:04Z

lib/Doctrine/ODM/MongoDB/Utility/LifecycleEventManager.php

-                new PreUpdateEventArgs($document, $this->dm, $this->uow->getDocumentChangeSet($document))
-            ));
+        if (! empty($class->lifecycleCallbacks[Events::preUpdate])) {
+            $class->invokeLifecycleCallbacks(Events::preUpdate, $document, [new PreUpdateEventArgs($document, $this->dm, $this->uow->getDocumentChangeSet($document))]);


Are we ignoring line's length from now on?

PSR-2 does not include a hard limit. That said, I'm fine changing any lines you mark as "too long, could be split into multiples" 😛

No need, it just stood out, I don't mind scrolling sideways that much ;)

Majkl578 · 2018-02-19T19:52:00Z

.travis.yml

      script:
        - vendor/bin/phpstan analyse -l 1 lib

+    - stage: Code Quality
+      php: 7.2


This should be 7.1 since 7.1 is required in composer, otherwise you may end up with object typehint.

carusogabriel · 2018-03-04T08:20:54Z

@alcaeus I believe 60e293e can be surpassed by Doctrine CS v4 now :)

alcaeus · 2018-03-05T05:35:18Z

Yeah, have to catch up with the times...we're running an antique version of our coding-standard here 😉

carusogabriel · 2018-03-08T19:39:10Z

@alcaeus @malarzm May I update to CS v4?

alcaeus · 2018-03-08T19:40:01Z

Damn, I thought I had that in there as well. Yes, feel free to do so.

alcaeus added the BC Break label Feb 9, 2018

alcaeus added this to the 2.0.0 milestone Feb 9, 2018

alcaeus self-assigned this Feb 9, 2018

alcaeus added this to To Do in ODM 2.0 via automation Feb 9, 2018

alcaeus force-pushed the phpcs branch from e10af3f to 87b448a Compare February 9, 2018 07:56

alcaeus moved this from To Do to In progress in ODM 2.0 Feb 9, 2018

malarzm reviewed Feb 9, 2018

View reviewed changes

Majkl578 reviewed Feb 19, 2018

View reviewed changes

alcaeus force-pushed the phpcs branch 3 times, most recently from ff0d2c4 to 60e293e Compare March 2, 2018 19:01

alcaeus added 3 commits March 5, 2018 07:16

Add phpcs to build stages

fed7512

Apply automatic fixes

ef8670e

Fix type errors in tests

7c4496a

alcaeus force-pushed the phpcs branch 2 times, most recently from aee5bea to e8fafce Compare March 8, 2018 18:41

alcaeus added 3 commits March 8, 2018 20:02

Fix failing tests

d2105ea

Fix risky tests

7b91337

Drop useless test

35a4f2e

alcaeus force-pushed the phpcs branch from e8fafce to 35a4f2e Compare March 8, 2018 19:02

malarzm merged commit 04938ee into doctrine:master Mar 8, 2018

ODM 2.0 automation moved this from In progress to Done Mar 8, 2018

alcaeus deleted the phpcs branch March 8, 2018 19:39

alcaeus mentioned this pull request Mar 9, 2018

[2.0] Code cleanup #1702

Closed

alcaeus added the Task label Dec 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add phpcs to build and apply automatic fixes #1743

Add phpcs to build and apply automatic fixes #1743

alcaeus commented Feb 9, 2018 •

edited

Loading

malarzm Feb 9, 2018

alcaeus Feb 9, 2018

Majkl578 Feb 9, 2018

malarzm Feb 9, 2018

alcaeus Feb 9, 2018

malarzm Feb 9, 2018 •

edited

Loading

kukulich Feb 9, 2018

alcaeus Feb 9, 2018

carusogabriel Feb 9, 2018

kukulich Feb 9, 2018

Majkl578 Feb 9, 2018

Majkl578 Feb 9, 2018

kukulich Feb 9, 2018

malarzm Feb 9, 2018

alcaeus Feb 9, 2018

kukulich Feb 9, 2018

malarzm Feb 9, 2018

malarzm Feb 9, 2018 •

edited

Loading

malarzm Feb 9, 2018

alcaeus Feb 9, 2018

malarzm Feb 9, 2018

alcaeus Feb 9, 2018

malarzm Feb 9, 2018

Majkl578 Feb 19, 2018 •

edited

Loading

carusogabriel commented Mar 4, 2018

alcaeus commented Mar 5, 2018

carusogabriel commented Mar 8, 2018

alcaeus commented Mar 8, 2018

Add phpcs to build and apply automatic fixes #1743

Add phpcs to build and apply automatic fixes #1743

Conversation

alcaeus commented Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malarzm Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malarzm Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Majkl578 Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

carusogabriel commented Mar 4, 2018

alcaeus commented Mar 5, 2018

carusogabriel commented Mar 8, 2018

alcaeus commented Mar 8, 2018

alcaeus commented Feb 9, 2018 •

edited

Loading

malarzm Feb 9, 2018 •

edited

Loading

malarzm Feb 9, 2018 •

edited

Loading

Majkl578 Feb 19, 2018 •

edited

Loading