Skip to content
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

Forbidden Classes Sniff #1360

Open
wants to merge 7 commits into
base: 3.0
Choose a base branch
from
Open

Forbidden Classes Sniff #1360

wants to merge 7 commits into from

Conversation

scheb
Copy link

@scheb scheb commented Feb 19, 2017

Hello, would you be interested in a code sniff to forbid usage of certain classes? I'm writing that one for a project where we want to keep developers from using certain classes (e.g. deprcated ones) and suggesting alternatives instead. It looks for class usages in:

  • New statements
  • Static calls
  • Type hints in methods/functions

Then resolves the fully qualified class name and checks it against the configurable forbidden classes list. To resolve the fully qualified name, the sniff is taking namespace, class imports and namespace/class aliases into account. So it heavily relies on the namespace features of PHP and therefore I target the 3.0 branch with this PR.

Current limitations are:

  • It cannot handle multi-import use-statements yet, but I'm confident to get it working.
  • There are some others class usage that is doesn't cover yet, e.g. extends/implements statements.

Also, and that's something where you could help me, I'm wondering how I can configure the $forbiddenClasses parameter for the unit tests. Currently the forbidden class used for testing is hardcoded in the sniff, which is certainly not the best solution.

I hope to get some feedback!

gsherwood added a commit that referenced this pull request Feb 21, 2017
@scheb
Copy link
Author

scheb commented Feb 27, 2017

Latest commit supports multi-import use-statements. Also, the usage types can be configured, in which contexts the sniff should apply to (e.g. if you don't want to apply the check to extends/implements statements).

Support for traits/extends/implements not finished yet.

@orlangur
Copy link

Do you plan to check for forbidden classes appearance in PHPDoc?

I'm wondering how I can configure the $forbiddenClasses parameter for the unit tests

From https://github.com/squizlabs/PHP_CodeSniffer/blob/2.8.0/CodeSniffer.php#L1529 it looks like you can call setSniffProperty in setUp method of your test, but I'm not sure this is the recommended way as I don't see such calls in existing tests.

@scheb
Copy link
Author

scheb commented Mar 5, 2017

Now supports trait imports, implements and extends.

@orlangur Haven't thought about PHPDoc yet. Will give it a try next.

@scheb
Copy link
Author

scheb commented Mar 5, 2017

PHPDoc support as requested. Do I need to check some more PHPDoc tags as the ones implemented?

Also added the check for function return types, which have been added in PHP7.

Would be nice to get some feedback from the devs, especially how to configure sniffs in unit-tests. The suggested setSniffProperty seems to be a solution, but I don't understand how I can call it from the test.

@orlangur
Copy link

orlangur commented Mar 6, 2017

See AbstractSniffUnitTest implementation: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/tests/Standards/AbstractSniffUnitTest.php#L64

In your class extended from this one you should do something like

protected function setUp()
{
    parent::setUp();
    self::$phpcs->setSniffProperty(...);

@scheb
Copy link
Author

scheb commented Mar 6, 2017

@orlangur Unfortunately not in the 3.0 version branch :(

@orlangur
Copy link

orlangur commented Mar 6, 2017

You're right!

On the first glance it looks to me that unlike the 2.x you can achieve the same using these observations:
https://github.com/squizlabs/PHP_CodeSniffer/blob/3.0/tests/Standards/AbstractSniffUnitTest.php#L33
https://github.com/squizlabs/PHP_CodeSniffer/blob/3.0/tests/Standards/AbstractSniffUnitTest.php#L131

So, technically you can specify those global keys in setUp method. Again, it's better to wait for project lead response just to be sure that we're trying to use testing framework as intended.

@nawarian
Copy link

Hey guys! I know this PR is a bit old already but seems to be still mergeable. What is missing for it? Can I support you somehow?

@kevinsmith
Copy link

Curious about this as well. Anything else needed before it can be merged in?

@gsherwood
Copy link
Member

Anything else needed before it can be merged in?

The biggest issue with merging in new sniffs is always ensuring they've covered all the edge cases so they don't get merged in and then immediately produce a series of bug reports that cause development to slow down.

If you (or anyone) can grab the sniff and test it on your projects, that would be very helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants