[feature] add static analysis tool #2664

Treggats · 2023-11-02T23:49:31Z

Introduce PHPStan as static analyser. Continues the work done on #2471

resolves #1930

This PR does the following

add a basic config for PHPStan
resolve issues found for level 1
add a baseline for issues found in level 2
add a job to the coding standard Github Workflow

.editorconfig

Treggats · 2023-11-02T23:51:44Z

.github/workflows/build-ci.yml

@@ -2,8 +2,6 @@ name: CI

 on:
    push:
-        branches:


redundant, and not according to the Github Workflow schema

Treggats · 2023-11-02T23:55:44Z

.github/workflows/coding-standards.yml

+
+  analysis:
+    runs-on: "ubuntu-22.04"
+    continue-on-error: true


This will allow the job to continue even if PHPStan finds errors.

For now mostly a convenience to see the output of runs for both PHP 8.1 and 8.2

Treggats · 2023-11-02T23:56:28Z

.github/workflows/coding-standards.yml

@@ -53,3 +51,29 @@ jobs:
      # The -q option is required until phpcs v4 is released
      - name: "Run PHP_CodeSniffer"
        run: "vendor/bin/phpcs -q --no-colors --report=checkstyle | cs2pr"
+
+  analysis:


Borrowed from the changes that @divine made in #2471

.github/workflows/coding-standards.yml

composer.json

Treggats · 2023-11-02T23:59:36Z

phpstan-baseline.neon

@@ -0,0 +1,46 @@
+parameters:


The baseline provides PHPStan with a list of errors to ignore, kind of saying

I know these issues exist, will fix it later

Treggats · 2023-11-03T00:00:40Z

phpstan.neon.dist

+    editorUrl: 'phpstorm://open?file=%%file%%&line=%%line%%'
+
+    ignoreErrors:
+        - '#Unsafe usage of new static#'


These issues were the remaining errors from level 1, I wasn't sure how to fix them. So I chose to ignore them.

Treggats · 2023-11-03T00:00:58Z

phpunit.xml.dist

@@ -1,13 +1,9 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/10.4/phpunit.xsd"
-         backupGlobals="false"


removed options that had the default value.

Treggats · 2023-11-03T00:01:34Z

src/Relations/BelongsToMany.php

@@ -82,11 +82,11 @@ protected function setWhere()
    }

    /** @inheritdoc */
-    public function save(Model $model, array $joining = [], $touch = true)
+    public function save(Model $model, array $pivotAttributes = [], $touch = true)


The parent method argument name was changed, updated it to match it.

Treggats · 2023-11-03T00:04:14Z

.editorconfig

+
+[*.yml]
+indent_size = 2


Match the current indenting.

.github/workflows/build-ci.yml

Treggats · 2023-11-03T00:12:59Z

phpstan.neon.dist

+    - ./phpstan-baseline.neon
+
+parameters:
+    tmpDir: .cache/phpstan


Will speedup subsequent runs, in a Github Workflow we'd need to use the cache action to store this cache

.github/workflows/coding-standards.yml

composer.json

src/Relations/EmbedsMany.php

GromNaN · 2023-11-03T07:41:11Z

src/Relations/EmbedsOne.php

     * @return int
     */
-    public function delete()
+    public function delete($id = null)


Same here, can we remove this argument, since it's not used.

src/Relations/EmbedsOneOrMany.php

GromNaN · 2023-11-03T07:45:36Z

src/Relations/EmbedsOneOrMany.php

     * @return int
     */
-    public function count()
+    public function count($columns = '*')


composer.json

.github/workflows/coding-standards.yml

Treggats · 2023-11-03T23:46:43Z

src/Eloquent/Builder.php

+        $builder = $this->toBase();
+        assert($builder instanceof MongoDBQueryBuilder);
+
+        return $builder->update($this->addUpdatedAtColumn($values), $options);


So this is an interesting case. Both the IDE and PHPStan think that Illuminate\Database\Query\Builder is used, while actually it's MongoDB\Laravel\Query\Builder.
Adding the assertion is quickest/eastiest way to make sure the correct one is used.

You can overload the return type of toBase with an annotation on this class.

/** @method \MongoDB\Laravel\Query\Builder toBase() */ class Builder extends EloquentBuilder

src/Eloquent/Builder.php

Treggats · 2023-11-04T00:58:22Z

src/Relations/EmbedsOneOrMany.php

-        $this->query      = $query;
-        $this->parent     = $parent;
+        parent::__construct($query, $parent);
+
        $this->related    = $related;


This property is also being set in the parent constructor, but when I tried to change it a few mocks broke and I'm not sure how to update those. So I left it alone 😅

Treggats · 2023-11-04T01:09:24Z

Putting in draft while I figure out why the tests are faling

.github/workflows/coding-standards.yml

.cache/.gitignore

.github/workflows/build-ci.yml

Treggats · 2023-11-15T21:47:17Z

.gitignore

@@ -3,9 +3,9 @@
 *.sublime-workspace
 .DS_Store
 .idea/
-.phpunit.cache/


I introduced the .cache directory for caching phpunit/phpcs/phpstan

Treggats · 2023-11-15T21:48:32Z

phpunit.xml.dist

@@ -20,10 +18,15 @@
        <env name="MONGODB_DATABASE" value="unittest"/>
        <env name="SQLITE_DATABASE" value=":memory:"/>
        <env name="QUEUE_CONNECTION" value="database"/>
+
+        <ini name="memory_limit" value="-1"/>


While running the tests locally in Docker I ran into the issue that there wasn't enough memory. Disabling the limit here, seems like the most logical place

Treggats · 2023-11-15T21:49:10Z

phpunit.xml.dist

        <include>
-            <directory suffix=".php">./src</directory>


the suffix was redundant so it could be removed

.cache/.gitignore

.github/workflows/build-ci.yml

src/Eloquent/Builder.php

src/Relations/EmbedsMany.php

src/Relations/EmbedsOneOrMany.php

Treggats · 2023-11-16T11:45:35Z

src/Relations/EmbedsMany.php

@@ -82,7 +86,7 @@ public function performUpdate(Model $model)
        // Get the correct foreign key value.
        $foreignKey = $this->getForeignKeyValue($model);

-        $values = $this->getUpdateValues($model->getDirty(), $this->localKey . '.$.');
+        $values = self::getUpdateValues($model->getDirty(), $this->localKey . '.$.');


The method is static, so using a static call here

The codecov.io service is no longer used.

phpstan-baseline.neon

src/Relations/BelongsToMany.php

Co-authored-by: Mohammad Mortazavi <39920372+hans-thomas@users.noreply.github.com>

phpstan-baseline.neon

Co-authored-by: Mohammad Mortazavi <39920372+hans-thomas@users.noreply.github.com>

Treggats · 2023-11-23T13:41:11Z

src/Relations/BelongsToMany.php

-     *
-     * @return array
-     */
-    protected function formatSyncList(array $records)


removing in favor of formatRecordList as mentioned in the comment

src/Relations/BelongsToMany.php

Co-authored-by: Mohammad Mortazavi <39920372+hans-thomas@users.noreply.github.com>

GromNaN · 2023-11-23T19:04:18Z

Thank you @Treggats

Treggats · 2023-11-23T19:05:40Z

Thank you @Treggats

You're welcome @GromNaN was a nice experience

Treggats commented Nov 3, 2023

View reviewed changes

.editorconfig

[*.yml]

indent_size = 2

Copy link

Contributor Author

Treggats Nov 3, 2023

Choose a reason for hiding this comment

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

Match the current indenting.

GromNaN reacted with thumbs up emoji

Treggats marked this pull request as ready for review November 3, 2023 00:05

Treggats commented Nov 3, 2023

View reviewed changes

.github/workflows/build-ci.yml Show resolved Hide resolved

Treggats commented Nov 3, 2023

View reviewed changes

.github/workflows/coding-standards.yml Outdated Show resolved Hide resolved

GromNaN reviewed Nov 3, 2023

View reviewed changes

Treggats force-pushed the feature/1930-add-phpstan branch from b4989e5 to 848bed3 Compare November 3, 2023 23:42

Treggats commented Nov 3, 2023

View reviewed changes

src/Eloquent/Builder.php Show resolved Hide resolved

Treggats requested a review from GromNaN November 3, 2023 23:50

Treggats force-pushed the feature/1930-add-phpstan branch 2 times, most recently from 9f5e849 to 06a405f Compare November 4, 2023 00:46

Treggats commented Nov 4, 2023

View reviewed changes

Treggats marked this pull request as draft November 4, 2023 01:09

GromNaN reviewed Nov 15, 2023

View reviewed changes

.github/workflows/coding-standards.yml Outdated Show resolved Hide resolved

Treggats force-pushed the feature/1930-add-phpstan branch 2 times, most recently from c48f18e to 6014fc6 Compare November 15, 2023 21:44

Treggats commented Nov 15, 2023

View reviewed changes

Treggats marked this pull request as ready for review November 15, 2023 21:49

Treggats mentioned this pull request Nov 15, 2023

Update docker and test configs #2678

Merged

Treggats requested review from GromNaN and divine November 16, 2023 08:53

GromNaN reviewed Nov 16, 2023

View reviewed changes

Treggats force-pushed the feature/1930-add-phpstan branch 2 times, most recently from 5f48433 to d26a9cd Compare November 16, 2023 11:41

Treggats commented Nov 16, 2023

View reviewed changes

Treggats requested a review from GromNaN November 16, 2023 11:46

Treggats force-pushed the feature/1930-add-phpstan branch from d26a9cd to 31157c0 Compare November 16, 2023 12:11

Treggats added 11 commits November 22, 2023 16:40

use static call for static method

306476c

let early return return the expected data

c4755b1

unused, removing

6d0e49e

The codecov.io service is no longer used.

update PHPStan baseline (level 2)

1cc60c2

replace assert with docblock

8bce1bf

remove @throw docblock comment

9b4de89

fix PHPStan error; unknown method pull()

a7b572e

replace array_merge with array unpacking

58d7c63

add return type to match the userland use case

6c8f41a

ignore Eloquent magically passing arguments

02b7acb

replace with the suggested "new" method

48adaeb

Treggats force-pushed the feature/1930-add-phpstan branch from 42cf4a3 to 48adaeb Compare November 22, 2023 15:40

Treggats commented Nov 22, 2023

View reviewed changes

phpstan-baseline.neon Show resolved Hide resolved

hans-thomas reviewed Nov 23, 2023

View reviewed changes

src/Relations/BelongsToMany.php Outdated Show resolved Hide resolved

Apply suggestions from code review

395ed5c

Co-authored-by: Mohammad Mortazavi <39920372+hans-thomas@users.noreply.github.com>

hans-thomas reviewed Nov 23, 2023

View reviewed changes

phpstan-baseline.neon Outdated Show resolved Hide resolved

Treggats and others added 2 commits November 23, 2023 14:19

fix null coalescing

d73f9d9

Co-authored-by: Mohammad Mortazavi <39920372+hans-thomas@users.noreply.github.com>

remove Collection check

0db7601

Co-authored-by: Mohammad Mortazavi <39920372+hans-thomas@users.noreply.github.com>

Treggats commented Nov 23, 2023

View reviewed changes

Treggats requested review from GromNaN and hans-thomas November 23, 2023 13:41

GromNaN approved these changes Nov 23, 2023

View reviewed changes

hans-thomas reviewed Nov 23, 2023

View reviewed changes

src/Relations/BelongsToMany.php Show resolved Hide resolved

remove Collection check

6bfe719

Co-authored-by: Mohammad Mortazavi <39920372+hans-thomas@users.noreply.github.com>

Treggats requested a review from hans-thomas November 23, 2023 14:44

GromNaN merged commit 973b6b9 into mongodb:4.1 Nov 23, 2023
15 checks passed

Treggats deleted the feature/1930-add-phpstan branch November 23, 2023 19:05

hans-thomas mentioned this pull request Nov 27, 2023

Hybrid support for BelongsToMany relationship #2688

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] add static analysis tool #2664

[feature] add static analysis tool #2664

Treggats commented Nov 2, 2023 •

edited

Loading

Treggats Nov 2, 2023

Treggats Nov 2, 2023

Treggats Nov 2, 2023

Treggats Nov 2, 2023

Treggats Nov 3, 2023

Treggats Nov 3, 2023

Treggats Nov 3, 2023

Treggats Nov 3, 2023

Treggats Nov 3, 2023

GromNaN Nov 3, 2023

GromNaN Nov 3, 2023

Treggats Nov 3, 2023

GromNaN Nov 22, 2023

Treggats Nov 4, 2023

Treggats commented Nov 4, 2023

Treggats Nov 15, 2023

Treggats Nov 15, 2023

Treggats Nov 15, 2023

Treggats Nov 16, 2023

Treggats Nov 23, 2023

GromNaN commented Nov 23, 2023

Treggats commented Nov 23, 2023

[feature] add static analysis tool #2664

[feature] add static analysis tool #2664

Conversation

Treggats commented Nov 2, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Treggats commented Nov 4, 2023

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

GromNaN commented Nov 23, 2023

Treggats commented Nov 23, 2023

Treggats commented Nov 2, 2023 •

edited

Loading