[stable10] Introduce phpstan by phil-davis · Pull Request #35774 · owncloud/core

phil-davis · 2019-07-07T13:25:33Z

Description

Backport of various commits from PR Introduce phpstan #31810
Then fix other stuff in stable10 that phpstan reports.

Also fixes #35773

Motivation and Context

When trying some things for "drop PHP 7.0" and for PHPunit7 I noticed some problems and ended up in some of the same code that was corrected/adjusted by master PR #31810

Issue

How Has This Been Tested?

Local run of:

make test-php-phpstan

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

phil-davis · 2019-07-07T13:40:42Z

apps/files_sharing/lib/Scanner.php

 		if ($sourceScanner instanceof NoopScanner) {
 			list(, $internalPath) = $this->storage->resolvePath($file);
-			return parent::scan($internalPath, $reuseExisting, $parentId, $cacheData, $lock);
+			return parent::scan($internalPath, self::SCAN_SHALLOW, $reuseExisting, $lock);


Does this code ever get called?
The params were wrong.

See #32377 (comment)
#32377 has touched/fixed this code in master but never got backported.

codecov · 2019-07-07T14:23:00Z

Codecov Report

Merging #35774 into stable10 will increase coverage by 0.25%.
The diff coverage is 76%.

@@              Coverage Diff               @@
##             stable10   #35774      +/-   ##
==============================================
+ Coverage       64.84%   65.09%   +0.25%     
+ Complexity      20243    20238       -5     
==============================================
  Files            1299     1300       +1     
  Lines           77220    77237      +17     
  Branches         1301     1301              
==============================================
+ Hits            50070    50278     +208     
+ Misses          26765    26574     -191     
  Partials          385      385

Flag	Coverage Δ	Complexity Δ
#javascript	`53.85% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.28% <76%> (+0.28%)`	`20238 <30> (-5)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Session/Session.php	`100% <ø> (ø)`	`5 <0> (ø)`	⬇️
lib/private/Files/Storage/Common.php	`85.15% <ø> (+3.33%)`	`141 <0> (ø)`	⬇️
lib/private/Session/CryptoWrapper.php	`47.05% <ø> (ø)`	`6 <0> (ø)`	⬇️
core/Command/Maintenance/DataFingerprint.php	`100% <ø> (ø)`	`4 <0> (ø)`	⬇️
lib/private/Files/Node/Folder.php	`96.15% <ø> (-0.03%)`	`45 <0> (ø)`
lib/private/Files/Cache/Wrapper/CacheWrapper.php	`89.06% <ø> (+2.69%)`	`29 <0> (-1)`	⬇️
apps/systemtags/appinfo/app.php	`14.28% <ø> (ø)`	`0 <0> (ø)`	⬇️
core/Command/Group/AddMember.php	`100% <ø> (ø)`	`9 <0> (ø)`	⬇️
lib/private/legacy/util.php	`70.18% <ø> (+2.84%)`	`239 <0> (-2)`	⬇️
lib/private/Console/TimestampFormatter.php	`0% <ø> (ø)`	`8 <0> (ø)`	⬇️
... and 93 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 305a8ec...1b368f7. Read the comment docs.

codecov · 2019-07-07T14:23:00Z

Codecov Report

Merging #35774 into stable10 will decrease coverage by 19.3%.
The diff coverage is 0%.

@@               Coverage Diff               @@
##             stable10   #35774       +/-   ##
===============================================
- Coverage       64.99%   45.68%   -19.31%     
===============================================
  Files            1302      116     -1186     
  Lines           77354    11519    -65835     
  Branches         1301     1301               
===============================================
- Hits            50274     5263    -45011     
+ Misses          26695     5871    -20824     
  Partials          385      385

Flag	Coverage Δ	Complexity Δ
#javascript	`53.85% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`31.14% <0%> (-35.03%)`	`0 <0> (-20279)`

Impacted Files	Coverage Δ	Complexity Δ
apps/files_external/lib/Lib/Storage/Swift.php	`66.16% <ø> (ø)`	`0 <0> (ø)`	⬇️
apps/files_external/lib/Lib/Storage/SFTP.php	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
apps/files_external/lib/Lib/Storage/AmazonS3.php	`7.73% <ø> (ø)`	`0 <0> (ø)`	⬇️
...external/lib/Lib/LegacyDependencyCheckPolyfill.php	`47.36% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/Files/Storage/DAV.php	`59.45% <0%> (-21.64%)`	`0 <0> (ø)`
apps/updatenotification/templates/admin.php
lib/private/Encryption/Keys/Storage.php
lib/private/App/CodeChecker/NodeVisitor.php
lib/private/RedisFactory.php
... and 1104 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 45046f3...686aa34. Read the comment docs.

phil-davis · 2019-07-08T05:51:59Z

.gitignore

 /build/composer.phar
 /build/dist
 /build/phan.phar
+/build/phpstan.phar*


Note: this comes from the backport.
Similar to /build/phan.phar above it. these files should not even appear in the build directory these days - the tools are in vendor-bin

But we can remove these later. For now, this gets stable10 matching master

phil-davis · 2019-07-08T05:54:39Z

apps/encryption/lib/Session.php

 		$key = $this->session->get('privateKey');
 		if ($key === null) {
-			throw new Exceptions\PrivateKeyMissingException('please try to log-out and log-in again', 0);
+			throw new Exceptions\PrivateKeyMissingException('please try to log-out and log-in again');


This is a bit weird, but it happens in other places also - the first parameter is supposed to be the "text" version of the user name. Whatever text is passed ends up getting into the text of the exception.

This is encryption app code - so was not being analyzed by phpstan in master because it does not exist in master

phil-davis · 2019-07-08T05:58:24Z

lib/private/Files/External/InvalidStorage.php


 class InvalidStorage extends FailedStorage {
 	public function __construct($params) {
+		$params['exception'] = new StorageNotAvailableException();


php (and phpstan) wants the constructor to have a $params arg like FailedStorage
But then phpstan complains that $params is not used.
So this code uses it ;)

The mystery is that in master the constructor has no parameters, and phpstan does not complain in master ???

See for answer: #32051 (comment)

phil-davis · 2019-07-08T05:59:54Z

lib/private/Files/Storage/Temporary.php

+		if (!\is_array($arguments)) {
+			$arguments = [];
+		}
+		$arguments['datadir'] = \OC::$server->getTempManager()->getTemporaryFolder();


phpstan complained about unused $arguments - so this code uses $arguments

lib/private/Share/Share.php

lib/private/legacy/util.php

phil-davis · 2019-07-08T06:12:26Z

settings/Middleware/SubadminMiddleware.php

 	public function beforeController($controller, $methodName) {
 		if (!$this->reflector->hasAnnotation('NoSubadminRequired')) {
 			if (!$this->isSubAdmin) {
-				throw new NotAdminException('Logged in user must be a subadmin');


NotAdminException does not accepts any parameter. And it inherits from a parent that has no parameter.
I made a new NotSubadminException which can have this text.
Is that the reasonable thing to do?

This code went to the user_management app and disappeared from core master in core PR #30960
So that is why phpstan did not notice it in core master
The code in the user_management app has this same "feature"

micbar · 2019-07-08T11:42:54Z

Great! Passes CI

phil-davis · 2019-07-08T11:49:20Z

Great! Passes CI

yes, I fought with that for a bit - there s a list of small commits in this PR that are extra to what happened in master. I am currently convincing myself about why master passes without those. One of the obvious reasons is that encryption is not in master.

phil-davis · 2019-07-08T12:46:08Z

phpstan.neon

    - %currentWorkingDirectory%/apps/*/appinfo/routes.php
    - %currentWorkingDirectory%/apps/*/composer/*
    - %currentWorkingDirectory%/apps/*/3rdparty/*
+    - %currentWorkingDirectory%/apps/files/appinfo/update.php


This is because apps/files/appinfo/update.php was deleted from master by #33209 so it never had to be considered in the master PHPstan.

The code is not a class, it is just some old script. It gives:

------ -------------------------------------------------------------------------------------------------------------------------------- Line apps/files/appinfo/update.php ------ -------------------------------------------------------------------------------------------------------------------------------- 52 Function owncloud_reset_encrypted_flag not found while trying to analyse it - autoloading is probably not configured properly. 72 Function owncloud_reset_encrypted_flag not found. 79 Function owncloud_reset_encrypted_flag not found. ------ --------------------------------------------------------------------------------------------------------------------------------

and I couldn't work out how to "autoload" it. Seems not worth more time, so ignore it, and maybe delete it in #35781

phil-davis · 2019-07-08T12:48:13Z

apps/encryption/lib/Session.php

 		$key = $this->session->get('privateKey');
 		if ($key === null) {
-			throw new Exceptions\PrivateKeyMissingException('please try to log-out and log-in again', 0);
+			throw new Exceptions\PrivateKeyMissingException('please try to log-out and log-in again');


This is encryption app code - so was not being analyzed by phpstan in master because it does not exist in master

phil-davis · 2019-07-08T12:50:25Z

apps/files_external/lib/Lib/Storage/AmazonS3.php

+	/**
+	 * @var string
+	 */
+	private $id;


This whole file does not exist any more in master so that is why phpstan did not report about it in master

…ch parent in stable10

…ed without any parameters'

…nused parameter '

…parameter '

…eption constructor invoked with 1 parameter, 0 required'

phil-davis · 2019-07-09T02:38:14Z

@DeepDiver1975 @micbar @patrickjahns I don't see anything more that I can tidy up here.
It would be nice to fix these things in stable10 and also get phpstan running.

Please review.

phil-davis added the 2 - Developing label Jul 7, 2019

phil-davis self-assigned this Jul 7, 2019

phil-davis commented Jul 7, 2019

View reviewed changes

phil-davis force-pushed the stable10-add-phpstan branch from e7a0568 to 686aa34 Compare July 7, 2019 14:22

phil-davis force-pushed the stable10-add-phpstan branch 2 times, most recently from 645b3d7 to b42a0e9 Compare July 7, 2019 15:19

phil-davis mentioned this pull request Jul 7, 2019

Introduce phpstan #31810

Merged

10 tasks

phil-davis requested review from DeepDiver1975, micbar and patrickjahns July 8, 2019 00:30

phil-davis added 3 - To Review and removed 2 - Developing labels Jul 8, 2019

phil-davis marked this pull request as ready for review July 8, 2019 00:31

phil-davis commented Jul 8, 2019

View reviewed changes

lib/private/Share/Share.php Show resolved Hide resolved

phil-davis commented Jul 8, 2019

View reviewed changes

lib/private/legacy/util.php Show resolved Hide resolved

phil-davis commented Jul 8, 2019

View reviewed changes

phil-davis force-pushed the stable10-add-phpstan branch from b42a0e9 to 8559248 Compare July 8, 2019 06:13

This was referenced Jul 8, 2019

Remove deprecated interface OCP\Files\Storage in favor of OCP\Files\S… #32051

Merged

[stable10] Remove not longer valid update script - pre-8.1 upgrade #35781

Merged

phil-davis commented Jul 8, 2019

View reviewed changes

phil-davis mentioned this pull request Jul 8, 2019

With objectstore, scan from sourcescanner for shared files #32377

Merged

patrickjahns and others added 5 commits July 9, 2019 05:31

fix acessing undefined propertoes

38041a4

Fix phpstan errors

24a9b0d

Remove xcache remainings

4b53120

Ignore aws-sdk-php FooBar.php for phpstan

f801b6c

Revert constructor parameters InvalidStorage.php Temporary.php to mat…

3d35fb0

…ch parent in stable10

phil-davis force-pushed the stable10-add-phpstan branch from be69acd to 28d8ccf Compare July 8, 2019 23:47

phil-davis added 9 commits July 9, 2019 05:38

add phpstan extra autoload_directories for stable10

c8f9041

Fix throw PrivateKeyMissingException in apps/encryption/lib/Session.php

11204bc

declare in apps/files_external/lib/Lib/Storage/AmazonS3.php

90f5e81

Fix 'Scanner::scan() invoked with 5 parameters, 1-4 required'

758a776

Fix 'Class OCP\API does not have a constructor and must be instantiat…

fdff19f

…ed without any parameters'

Avoid 'Constructor of class OC\Files\External\InvalidStorage has an u…

80486a7

…nused parameter '

Avoid 'Constructor of class OC\Files\Storage\Temporary has an unused …

bb68154

…parameter '

Fix 'Call to sprintf contains 1 placeholder, 2 values given'

834814f

fix 'Class OC\AppFramework\Middleware\Security\Exceptions\NotAdminExc…

ebcbb3d

…eption constructor invoked with 1 parameter, 0 required'

phil-davis force-pushed the stable10-add-phpstan branch from 28d8ccf to c1b5129 Compare July 8, 2019 23:53

Fix case of BackgroundJob call

1b368f7

phil-davis force-pushed the stable10-add-phpstan branch from c1b5129 to 1b368f7 Compare July 8, 2019 23:54

DeepDiver1975 approved these changes Jul 10, 2019

View reviewed changes

DeepDiver1975 merged commit 2d2dbc1 into stable10 Jul 10, 2019

delete-merged-branch bot deleted the stable10-add-phpstan branch July 10, 2019 13:45

phil-davis added the release:10.3 label Aug 2, 2019

patrickjahns added the changelog-ignored label Aug 12, 2019

Conversation

phil-davis commented Jul 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

phil-davis Jul 7, 2019 • 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!

codecov bot commented Jul 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov bot commented Jul 7, 2019

Codecov Report

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phil-davis Jul 8, 2019 • 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!

micbar commented Jul 8, 2019

Uh oh!

phil-davis commented Jul 8, 2019

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!

phil-davis commented Jul 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

phil-davis commented Jul 7, 2019 •

edited

Loading

phil-davis Jul 7, 2019 •

edited

Loading

codecov bot commented Jul 7, 2019 •

edited

Loading

phil-davis Jul 8, 2019 •

edited

Loading