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

Add support for disallowing namespaces #51

Merged
merged 1 commit into from
Feb 19, 2021

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Feb 17, 2021

See #50

Currently it only triggers on FullyQualified nodes.

    disallowedNamespaces:
        -
            namespace: 'Symfony\Component\HttpFoundation\RequestStack'
            message: 'pass Request via controller instead'
            allowIn:
                - tests/*
        -
            namespace: 'Assert\*'
            message: 'use Webmozart\Assert instead'

@ruudk
Copy link
Contributor Author

ruudk commented Feb 17, 2021

You could think: Why not just use disallowedMethodCalls and use a wildcard like Symfony\Component\HttpFoundation\RequestStack::*'.

That would work, but only for method calls.

In my case I wanted to prevent the use of RequestStack like this:

class MyClass
{
    private $requestStack;
    
    public function __construct(RequestStack $requestStack) 
    {
        $this->requestStack = $requestStack;
    }

    public function doSomething()
    {
        // Pass the object to something else.... we didn't call anything
        $model = Model::createFromRequestStack($this->requestStack);
    }
}

@spaze
Copy link
Owner

spaze commented Feb 17, 2021

Thanks. I'm fine with a new config key. Can you please squash the commits or somehow fix the commits so I can review? The "Add tests" commit now changes other areas too, guess it shouldn't.

@ruudk
Copy link
Contributor Author

ruudk commented Feb 17, 2021

I'm not with my computer anymore. Please review from GH diff view. Before we merge it I'll squash it.

src/NamespaceUsages.php Outdated Show resolved Hide resolved
Copy link
Owner

@spaze spaze left a comment

Choose a reason for hiding this comment

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

Quick round 1 of review.

Please also squash the commits to remove unrelated changes, thanks.

.gitignore Outdated Show resolved Hide resolved
src/DisallowedNamespace.php Outdated Show resolved Hide resolved
src/DisallowedNamespaceHelper.php Show resolved Hide resolved
src/NamespaceUsages.php Outdated Show resolved Hide resolved
tests/NamespaceUsagesTest.php Outdated Show resolved Hide resolved
tests/NamespaceUsagesTest.php Show resolved Hide resolved
tests/NamespaceUsagesTest.php Show resolved Hide resolved
src/NamespaceUsages.php Outdated Show resolved Hide resolved
src/NamespaceUsages.php Outdated Show resolved Hide resolved
@ruudk ruudk force-pushed the disallowed-namespaces branch 3 times, most recently from 9ecf883 to 058dd4e Compare February 18, 2021 08:17
@ruudk
Copy link
Contributor Author

ruudk commented Feb 18, 2021

Updated the PR. Much better now.

@spaze spaze merged commit bb809a3 into spaze:master Feb 19, 2021
@spaze
Copy link
Owner

spaze commented Feb 19, 2021

Thank you, much better, agree 👍 Made minor changes (some whitespace removal, added namespaces to the docs, removed one @throws) directly and merged. Will probably release right away.

@spaze
Copy link
Owner

spaze commented Feb 19, 2021

Just released this in 1.3.0, thank you for your contribution.

@ruudk
Copy link
Contributor Author

ruudk commented Feb 20, 2021

Thanks for merging all the PR's and the kind words in the release notes 💙

@ruudk ruudk deleted the disallowed-namespaces branch February 20, 2021 08:19
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.

None yet

2 participants