Allow app check code for deprecated classes #16935

nickvergessen · 2015-06-15T13:39:16Z

Old behaviour (without parameter):

nickv@nickv-think ~/ownCloud/master/core $ ./occ app:check-code activity
App is compliant - awesome job!

With deprecated parameter:

nickv@nickv-think ~/ownCloud/master/core $ ./occ app:check-code activity --deprecated
Analysing /home/nickv/ownCloud/master/repos/activity/templates/stream.app.navigation.php
 1 errors
    line   14: == - is discouraged
Analysing /home/nickv/ownCloud/master/repos/activity/controller/settings.php
 2 errors
    line   31: OCP\IConfig - deprecated class must not be imported with a use statement
    line  112: == - is discouraged
Analysing /home/nickv/ownCloud/master/repos/activity/lib/data.php
 1 errors
    line   27: OCP\DB - deprecated class must not be imported with a use statement

Analysing /home/nickv/ownCloud/master/repos/activity/appinfo/update.php
 3 errors
    line   14: OCP\DB - Static method of deprecated class must not be called
    line   24: OCP\DB - Static method of deprecated class must not be called
    line   26: OCP\DB - Static method of deprecated class must not be called
App uses deprecated functionality

Requested at https://mailman.owncloud.org/pipermail/devel/2015-June/001301.html

I think it's quite useful.
Also fixing a bug (use statements could circumvent blacklisted classes)

@DeepDiver1975 @MorrisJobke @LukasReschke

Todo

Add support for deprecated classes
Add support for deprecated constants
Add support for deprecated functions
Add support for deprecated methods

rullzer · 2015-07-05T12:07:39Z

Awesome work.
We should also try to get a checker for out internal code. To make sure non deprecated functions/methods do not call deprecated functions/methods..

PVince81 · 2015-07-06T09:08:53Z

Needs rebase

DeepDiver1975 · 2015-07-06T09:19:09Z

@nickvergessen please rebase an let's move this in - THX

nickvergessen · 2015-07-06T13:15:19Z

Btw, does anyone know something that got deprecated in the last 21days?

DeepDiver1975 · 2015-07-06T13:23:18Z

Btw, does anyone know something that got deprecated in the last 21days?

Not that I'm aware of

nickvergessen · 2015-07-06T13:23:28Z

Rebased and added the open task as todo.

Should be okay to merge, it doesn't catch everything, but at least static methods are done.

RobinMcCorkell · 2015-07-06T18:48:07Z

core/command/app/checkcode.php

 			);
 	}

 	protected function execute(InputInterface $input, OutputInterface $output) {
 		$appId = $input->getArgument('app-id');
-		$codeChecker = new \OC\App\CodeChecker();
+		if ($input->getOption('deprecated')) {
+			$codeChecker = new DeprecationCodeChecker();


I would highly recommend use of the Decorator pattern here, instead of subclassing CodeChecker with the desired functionality. It means that if we introduce some other functionality at a later point, we just re-wrap the CodeChecker instead of needing 6 (!!!) subclasses. It quickly spirals out of control after that...

/me wraps Xenopathic into a decorator....

RobinMcCorkell · 2015-07-07T12:33:02Z

core/command/app/checkcode.php

+		if ($input->getOption('deprecated')) {
+			$list = new DeprecationList();
+		} else {
+			$list = new PrivateList();


🙈 This isn't actually the Decorator pattern... What happens if we introduce some more checking, that we might need to combine with DeprecationList and/or PrivateList? You have the same problem, needing many classes and subclasses to cover all possibilities.

Have a BasicList class, whose methods return empty arrays, then let DeprecationList and PrivateList take an ICheckList in the constructor, and forward the methods to that wrapped ICheckList. Then you create a list at the start ($list = new BasicList()), then if we want to test for deprecation replace the object with the wrapped DeprecationList ($list = new DeprecationList($list)), etc for all features.

Sorry to be such a pain, but I can see this needing a refactor in the (near?) future if it isn't done now. 🙊

Right, mixed it up a bit. Should be fixed now

RobinMcCorkell · 2015-07-07T14:49:31Z

Nice! Any reason why it's DeprecatedCheck or PrivateCheck? Can we not have PrivateCheck enabled by default (controllable by a new parameter would be cool) and enable DeprecatedCheck as well if the option is set? Or was the intention to do this?

I can see ./occ app:check-code --deprecated --no-private --check-foobar being really powerful when the app checker gets more features...

rullzer · 2015-07-08T10:16:23Z

I agree with @Xenopathic lets enable the extra checks by default.
Programmers are lazy so better enable as much by default as possible.

nickvergessen · 2015-07-08T15:22:04Z

My idea was to be able to fail travis on private class usage and make travis "allow failure" for deprecated usage... so as long as it is possible to do those two separatly I'm fine with it

RobinMcCorkell · 2015-07-08T15:38:20Z

@nickvergessen May I push to this branch? I'll implement the selectable features thing I mentioned above if you want 😄

MorrisJobke · 2015-07-08T16:25:49Z

@Xenopathic Do it. @nickvergessen is usually on vacation right now :P

RobinMcCorkell · 2015-07-10T21:20:47Z

You can now specify the checkers you want enabled, with --checker=... or -c.... So to only enable private class and strong comparison checks, use ./occ app:check-code -cprivate -cstrong-comparison. By default, all are enabled, so we need to tell Jenkins to only use the private checker at the moment.

MorrisJobke · 2015-07-13T06:14:48Z

Analysing /home/mjob/Projekte/owncloud/master/apps2/music/app/music.php
 1 errors
    line  236: OCP\User::getUser - Method of deprecated private class must not be called
Analysing /home/mjob/Projekte/owncloud/master/apps2/music/middleware/ampachemiddleware.php
 1 errors
    line   71: OC_Util - Static method of deprecated private class must not be called

❌ OCP\User is not a private class, but is correctly deprecated.
✅ OC_Utilprivate and deprecated

RobinMcCorkell · 2015-07-13T10:26:18Z

@MorrisJobke Ah, that's a side effect of the description concatenation. If both DeprecationCheck and PrivateCheck are enabled, any failures will be marked as deprecated private class, regardless of which checker actually found the error.

MorrisJobke · 2015-07-13T12:16:16Z

@Xenopathic Can we fix this?

RobinMcCorkell · 2015-07-13T13:01:04Z

@MorrisJobke I have an idea...

nickvergessen · 2015-07-15T08:49:26Z

core/command/app/checkcode.php

+				'checker',
+				'c',
+				InputOption::VALUE_REQUIRED | InputOption::VALUE_IS_ARRAY,
+				'enable the specified checker',


Because this is what people see as help, we should maybe add the available checkers here?

The help text displays the default value, which is currently all the available checkers (all are enabled by default). If we include some non-default-on checkers later in the future, this could become an issue. The problem is, if we list all the checkers here, the default also gets displayed, so you will see the checkers twice (not to mention the help text line will be really long)

This is the help text:

--checker (-c) enable the specified checker (default: ["private","deprecation","strong-comparison"]) (multiple values allowed)

What do you think?

Ah right, I didn't check the output. Seems okay when they output the default value.
Now the question is should we include all in the default (and I think we shouldn't, but private only)?

nickvergessen · 2015-07-15T08:53:19Z

Well, that is why I wouldn't add them all by default...
Will have a look whether I can find a way later.

nickvergessen · 2015-07-16T09:45:10Z

#17046 was merged, so I will add that when jenkins reported back

By default, all are enabled, but specific ones can be selected through command line options.

scrutinizer-notifier · 2015-07-17T10:35:16Z

A new inspection was created.

nickvergessen · 2015-07-17T10:35:40Z

Rebased to fix the oracle thing without timeout.

MorrisJobke · 2015-07-17T16:02:40Z

Jenkins PR #17720

MorrisJobke · 2015-07-17T20:37:45Z

Tested and works nicely 👍

nickvergessen · 2015-07-18T08:33:47Z

@Xenopathic want to review? ;)

RobinMcCorkell · 2015-07-18T19:25:10Z

Code looks good, haven't tested it however. 👍

…ecated-classes Allow app check code for deprecated classes

oparoz · 2015-10-04T11:23:58Z

You can now specify the checkers you want enabled, with --checker=... or -c.... So to only enable private class and strong comparison checks, use ./occ app:check-code -cprivate -cstrong-comparison. By default, all are enabled, so we need to tell Jenkins to only use the private checker at the moment.

Documentation/OP needs to be updated. It still has the old switch, which doesn't work and no explanation on the other valid options

nickvergessen · 2015-10-05T07:30:33Z

@oparoz posted in the docs repo: owncloud-archive/documentation#1736

oparoz · 2015-10-05T13:10:43Z

Thanks. Added a link to the doc from the list of features for 8.2.

nickvergessen added the 2 - Developing label Jun 15, 2015

DeepDiver1975 added this to the 8.2-next milestone Jun 15, 2015

nickvergessen force-pushed the allow-app-check-code-for-deprecated-classes branch from faef8de to ee7dd54 Compare July 6, 2015 13:22

nickvergessen added 3 - To Review and removed 2 - Developing labels Jul 6, 2015

nickvergessen mentioned this pull request Jul 6, 2015

Do not use OC*::mb_*_replace(), they are useless #17046

Merged

RobinMcCorkell reviewed Jul 6, 2015
View reviewed changes

nickvergessen force-pushed the allow-app-check-code-for-deprecated-classes branch from ee7dd54 to 6b35a60 Compare July 7, 2015 10:18

RobinMcCorkell reviewed Jul 7, 2015
View reviewed changes

MorrisJobke added 2 - Developing and removed 3 - To Review labels Jul 9, 2015

nickvergessen reviewed Jul 15, 2015
View reviewed changes

nickvergessen added technical debt and removed 2 - Developing labels Jul 16, 2015

nickvergessen and others added 12 commits July 17, 2015 12:34

Allow app:check-code to check for deprecated methods

4e95031

Correctly handle use statements

d2fc1b2

Add deprecation version to the list

483c886

Add support for deprecated constants

f228a3d

Allow checking for functions

2783a78

Check for methods as good as possible

eb1c437

Use non-zero exit code

9e46904

Restructor the code into different classes instead of extending

bba87a2

Use the decorator pattern

a0c6f2e

Allow specifying the checkers used in app:check-code

0de5c35

By default, all are enabled, but specific ones can be selected through command line options.

Only decorate the type when it was matched

8a64abf

PR #17046 deprecated OCP\Util::mb_(sub)str_replace()

3566dcf

nickvergessen force-pushed the allow-app-check-code-for-deprecated-classes branch from d05ecba to 3566dcf Compare July 17, 2015 10:35

MorrisJobke mentioned this pull request Jul 17, 2015

Jenkins allow app check code for deprecated classes #17720

Closed

nickvergessen added a commit that referenced this pull request Jul 20, 2015

Merge pull request #16935 from owncloud/allow-app-check-code-for-depr…

0dd1a78

…ecated-classes Allow app check code for deprecated classes

nickvergessen merged commit 0dd1a78 into master Jul 20, 2015

nickvergessen deleted the allow-app-check-code-for-deprecated-classes branch July 20, 2015 06:39

lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019

Allow app check code for deprecated classes #16935

Allow app check code for deprecated classes #16935

Conversation

nickvergessen commented Jun 15, 2015

Todo

rullzer commented Jul 5, 2015

PVince81 commented Jul 6, 2015

DeepDiver1975 commented Jul 6, 2015

nickvergessen commented Jul 6, 2015

DeepDiver1975 commented Jul 6, 2015

nickvergessen commented Jul 6, 2015

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

RobinMcCorkell commented Jul 7, 2015

rullzer commented Jul 8, 2015

nickvergessen commented Jul 8, 2015

RobinMcCorkell commented Jul 8, 2015

MorrisJobke commented Jul 8, 2015

RobinMcCorkell commented Jul 10, 2015

MorrisJobke commented Jul 13, 2015

RobinMcCorkell commented Jul 13, 2015

MorrisJobke commented Jul 13, 2015

RobinMcCorkell commented Jul 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Jul 15, 2015

nickvergessen commented Jul 16, 2015

scrutinizer-notifier commented Jul 17, 2015

nickvergessen commented Jul 17, 2015

MorrisJobke commented Jul 17, 2015

MorrisJobke commented Jul 17, 2015

nickvergessen commented Jul 18, 2015

RobinMcCorkell commented Jul 18, 2015

oparoz commented Oct 4, 2015

nickvergessen commented Oct 5, 2015

oparoz commented Oct 5, 2015