-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[feature] add static analysis tool #2664
Changes from 40 commits
9ac70cc
9bc7245
2f182cf
314cd14
e985e8e
1aad84a
4e7d4e0
0168dce
7349d4d
6b7d047
b19813a
f2ea91f
d80979d
7c4340a
e3e5d2e
8b2b05e
829c410
28436c1
5885486
51b0bb9
29101df
aa05e9c
b6c48c2
ef81fd7
d6e8a85
e061452
b221fd2
9148217
306476c
c4755b1
6d0e49e
1cc60c2
8bce1bf
9b4de89
a7b572e
58d7c63
6c8f41a
02b7acb
48adaeb
395ed5c
d73f9d9
0db7601
6bfe719
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,6 @@ name: CI | |
|
||
on: | ||
push: | ||
Treggats marked this conversation as resolved.
Show resolved
Hide resolved
|
||
branches: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. redundant, and not according to the Github Workflow schema |
||
tags: | ||
pull_request: | ||
|
||
jobs: | ||
|
@@ -55,7 +53,7 @@ jobs: | |
- name: Show Docker version | ||
run: if [[ "$DEBUG" == "true" ]]; then docker version && env; fi | ||
env: | ||
DEBUG: ${{secrets.DEBUG}} | ||
DEBUG: ${{ secrets.DEBUG }} | ||
- name: Download Composer cache dependencies from cache | ||
id: composer-cache | ||
run: echo "dir=$(composer config cache-files-dir)" >> $GITHUB_OUTPUT | ||
|
@@ -66,14 +64,8 @@ jobs: | |
key: ${{ matrix.os }}-composer-${{ hashFiles('**/composer.json') }} | ||
restore-keys: ${{ matrix.os }}-composer- | ||
- name: Install dependencies | ||
run: | | ||
composer update --no-interaction $([[ "${{ matrix.mode }}" == low-deps ]] && echo ' --prefer-lowest --prefer-stable') | ||
run: composer update --no-interaction $([[ "${{ matrix.mode }}" == low-deps ]] && echo ' --prefer-lowest --prefer-stable') | ||
- name: Run tests | ||
run: | | ||
./vendor/bin/phpunit --coverage-clover coverage.xml | ||
run: ./vendor/bin/phpunit --coverage-clover coverage.xml | ||
env: | ||
MONGODB_URI: 'mongodb://127.0.0.1/?replicaSet=rs' | ||
- uses: codecov/codecov-action@v3 | ||
with: | ||
token: ${{ secrets.CODECOV_TOKEN }} | ||
fail_ci_if_error: false | ||
GromNaN marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,6 @@ name: "Coding Standards" | |
|
||
on: | ||
push: | ||
branches: | ||
tags: | ||
pull_request: | ||
|
||
env: | ||
|
@@ -15,6 +13,11 @@ jobs: | |
name: "phpcs" | ||
runs-on: "ubuntu-22.04" | ||
|
||
permissions: | ||
# Give the default GITHUB_TOKEN write permission to commit and push the | ||
# added or changed files to the repository. | ||
contents: write | ||
|
||
steps: | ||
- name: "Checkout" | ||
uses: "actions/checkout@v4" | ||
|
@@ -50,6 +53,67 @@ jobs: | |
with: | ||
composer-options: "--no-suggest" | ||
|
||
- name: "Format the code" | ||
continue-on-error: true | ||
run: | | ||
mkdir .cache | ||
./vendor/bin/phpcbf | ||
|
||
Treggats marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# The -q option is required until phpcs v4 is released | ||
- name: "Run PHP_CodeSniffer" | ||
run: "vendor/bin/phpcs -q --no-colors --report=checkstyle | cs2pr" | ||
|
||
- name: "Commit the changes" | ||
uses: stefanzweifel/git-auto-commit-action@v5 | ||
with: | ||
commit_message: "apply phpcbf formatting" | ||
|
||
analysis: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
runs-on: "ubuntu-22.04" | ||
continue-on-error: true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
strategy: | ||
matrix: | ||
php: | ||
- '8.1' | ||
- '8.2' | ||
steps: | ||
- name: Checkout | ||
uses: actions/checkout@v4 | ||
|
||
- name: Setup PHP | ||
uses: shivammathur/setup-php@v2 | ||
with: | ||
php-version: ${{ matrix.php }} | ||
extensions: curl, mbstring | ||
tools: composer:v2 | ||
coverage: none | ||
|
||
- name: Cache dependencies | ||
id: composer-cache | ||
uses: actions/cache@v3 | ||
with: | ||
path: ./vendor | ||
key: composer-${{ hashFiles('**/composer.lock') }} | ||
|
||
- name: Install dependencies | ||
run: composer install | ||
|
||
- name: Restore cache PHPStan results | ||
id: phpstan-cache-restore | ||
uses: actions/cache/restore@v3 | ||
with: | ||
path: .cache | ||
key: "phpstan-result-cache-${{ github.run_id }}" | ||
restore-keys: | | ||
phpstan-result-cache- | ||
|
||
- name: Run PHPStan | ||
run: ./vendor/bin/phpstan analyse --no-interaction --no-progress --ansi | ||
|
||
- name: Save cache PHPStan results | ||
id: phpstan-cache-save | ||
if: always() | ||
uses: actions/cache/save@v3 | ||
with: | ||
path: .cache | ||
key: ${{ steps.phpstan-cache-restore.outputs.cache-primary-key }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,9 @@ | |
*.sublime-workspace | ||
.DS_Store | ||
.idea/ | ||
.phpunit.cache/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I introduced the |
||
.phpcs-cache | ||
/vendor | ||
composer.lock | ||
composer.phar | ||
phpunit.xml | ||
phpstan.neon | ||
/.cache/ |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,31 @@ | ||||||||||||||||||||||
parameters: | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The baseline provides PHPStan with a list of errors to ignore, kind of saying
|
||||||||||||||||||||||
ignoreErrors: | ||||||||||||||||||||||
- | ||||||||||||||||||||||
message: "#^Variable \\$value on left side of \\?\\? always exists and is always null\\.$#" | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we remove the null coalescence operator in laravel-mongodb/src/Eloquent/Model.php Lines 297 to 304 in 82ddb83
This was introduced in #2653, could you check this error @hans-thomas? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Treggats We can remove UPDATE: To solve the second error, we can also remove the second The final method: public function fromJson($value, $asObject = false)
{
if (! is_string($value)) {
$value = Json::encode($value);
}
return Json::decode($value, ! $asObject);
} |
||||||||||||||||||||||
count: 1 | ||||||||||||||||||||||
path: src/Eloquent/Model.php | ||||||||||||||||||||||
|
||||||||||||||||||||||
- | ||||||||||||||||||||||
message: "#^Variable \\$value on left side of \\?\\? always exists and is not nullable\\.$#" | ||||||||||||||||||||||
count: 1 | ||||||||||||||||||||||
path: src/Eloquent/Model.php | ||||||||||||||||||||||
|
||||||||||||||||||||||
Treggats marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
- | ||||||||||||||||||||||
message: "#^Cannot call method modelKeys\\(\\) on array\\|Illuminate\\\\Support\\\\Collection\\.$#" | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like there is an hidden bug if laravel-mongodb/src/Relations/BelongsToMany.php Lines 129 to 132 in 82ddb83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. laravel-mongodb/src/Relations/BelongsToMany.php Lines 127 to 132 in 82ddb83
It's not even covered in the tests. In what case, the $this->parent->{$this->relatedPivotKey} could return a Collection ?I think in the past, it was possible but not now. The pivot column always is an array. UPDATE: removing this condition doesn't cause any error. |
||||||||||||||||||||||
count: 1 | ||||||||||||||||||||||
path: src/Relations/BelongsToMany.php | ||||||||||||||||||||||
|
||||||||||||||||||||||
- | ||||||||||||||||||||||
message: "#^Method Illuminate\\\\Database\\\\Eloquent\\\\Model\\:\\:push\\(\\) invoked with 3 parameters, 0 required\\.$#" | ||||||||||||||||||||||
GromNaN marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
count: 3 | ||||||||||||||||||||||
path: src/Relations/BelongsToMany.php | ||||||||||||||||||||||
|
||||||||||||||||||||||
- | ||||||||||||||||||||||
message: "#^Method Illuminate\\\\Database\\\\Eloquent\\\\Model\\:\\:push\\(\\) invoked with 3 parameters, 0 required\\.$#" | ||||||||||||||||||||||
count: 6 | ||||||||||||||||||||||
path: src/Relations/MorphToMany.php | ||||||||||||||||||||||
|
||||||||||||||||||||||
- | ||||||||||||||||||||||
message: "#^Method Illuminate\\\\Database\\\\Schema\\\\Blueprint\\:\\:create\\(\\) invoked with 1 parameter, 0 required\\.$#" | ||||||||||||||||||||||
count: 1 | ||||||||||||||||||||||
path: src/Schema/Builder.php |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
includes: | ||
- ./phpstan-baseline.neon | ||
|
||
parameters: | ||
tmpDir: .cache/phpstan | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will speedup subsequent runs, in a Github Workflow we'd need to use the cache action to store this cache |
||
|
||
paths: | ||
- src | ||
|
||
level: 2 | ||
|
||
editorUrl: 'phpstorm://open?file=%%file%%&line=%%line%%' | ||
|
||
ignoreErrors: | ||
- '#Unsafe usage of new static#' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These issues were the remaining errors from level 1, I wasn't sure how to fix them. So I chose to ignore them. |
||
- '#Call to an undefined method [a-zA-Z0-9\\_\<\>]+::[a-zA-Z]+\(\)#' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,13 @@ | ||
<?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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed options that had the default value. |
||
bootstrap="vendor/autoload.php" | ||
colors="true" | ||
processIsolation="false" | ||
stopOnFailure="false" | ||
cacheDirectory=".phpunit.cache" | ||
backupStaticProperties="false" | ||
> | ||
<coverage/> | ||
cacheDirectory=".cache/phpunit" | ||
executionOrder="depends,defects" | ||
beStrictAboutCoverageMetadata="true" | ||
beStrictAboutOutputDuringTests="true" | ||
failOnRisky="true" | ||
failOnWarning="true"> | ||
<testsuites> | ||
<testsuite name="Test Suite"> | ||
<directory>tests/</directory> | ||
|
@@ -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"/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
</php> | ||
<source> | ||
|
||
<source restrictDeprecations="true" | ||
restrictNotices="true" | ||
restrictWarnings="true"> | ||
<include> | ||
<directory suffix=".php">./src</directory> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the suffix was redundant so it could be removed |
||
<directory>./src</directory> | ||
</include> | ||
</source> | ||
</phpunit> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,8 +15,8 @@ | |
use function array_map; | ||
use function array_merge; | ||
use function array_values; | ||
use function assert; | ||
use function count; | ||
use function is_array; | ||
use function is_numeric; | ||
|
||
class BelongsToMany extends EloquentBelongsToMany | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The parent method argument name was changed, updated it to match it. |
||
{ | ||
$model->save(['touch' => false]); | ||
|
||
$this->attach($model, $joining, $touch); | ||
$this->attach($model, $pivotAttributes, $touch); | ||
|
||
return $model; | ||
} | ||
|
@@ -131,7 +131,7 @@ public function sync($ids, $detaching = true) | |
$current = $ids->modelKeys(); | ||
} | ||
|
||
$records = $this->formatSyncList($ids); | ||
$records = $this->formatRecordsList($ids); | ||
|
||
$current = Arr::wrap($current); | ||
|
||
|
@@ -171,6 +171,7 @@ public function sync($ids, $detaching = true) | |
public function updateExistingPivot($id, array $attributes, $touch = true) | ||
{ | ||
// Do nothing, we have no pivot table. | ||
return $this; | ||
} | ||
|
||
/** @inheritdoc */ | ||
|
@@ -229,6 +230,8 @@ public function detach($ids = [], $touch = true) | |
} | ||
|
||
// Remove the relation to the parent. | ||
assert($this->parent instanceof \MongoDB\Laravel\Eloquent\Model); | ||
assert($query instanceof \MongoDB\Laravel\Eloquent\Builder); | ||
$query->pull($this->foreignPivotKey, $this->parent->getKey()); | ||
|
||
if ($touch) { | ||
|
@@ -266,7 +269,7 @@ public function newPivotQuery() | |
/** | ||
* Create a new query builder for the related model. | ||
* | ||
* @return \Illuminate\Database\Query\Builder | ||
* @return Builder|Model | ||
*/ | ||
public function newRelatedQuery() | ||
{ | ||
|
@@ -295,28 +298,6 @@ public function getQualifiedRelatedPivotKeyName() | |
return $this->relatedPivotKey; | ||
} | ||
|
||
/** | ||
* Format the sync list so that it is keyed by ID. (Legacy Support) | ||
* The original function has been renamed to formatRecordsList since Laravel 5.3. | ||
* | ||
* @deprecated | ||
* | ||
* @return array | ||
*/ | ||
protected function formatSyncList(array $records) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removing in favor of |
||
{ | ||
$results = []; | ||
foreach ($records as $id => $attributes) { | ||
if (! is_array($attributes)) { | ||
[$id, $attributes] = [$attributes, []]; | ||
} | ||
|
||
$results[$id] = $attributes; | ||
} | ||
|
||
return $results; | ||
} | ||
|
||
/** | ||
* Get the name of the "where in" method for eager loading. | ||
* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Match the current indenting.