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 disallowedMethodCalls.allowCount to allow method calls N times #87

Merged
merged 1 commit into from
Oct 22, 2021

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Oct 21, 2021

Close #86

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.

Hey, thanks for the PR. It looks cool and clean, I like it, good job :-)

If you could change just one more detail and maybe add documentation to README?

src/DisallowedHelper.php Outdated Show resolved Hide resolved
@ruudk
Copy link
Contributor Author

ruudk commented Oct 22, 2021

Updated the readme. Please suggest changes if you want.

@spaze spaze merged commit 855a8de into spaze:master Oct 22, 2021
spaze added a commit that referenced this pull request Oct 22, 2021
Add `disallowedMethodCalls.allowCount` to allow method calls N times
@spaze
Copy link
Owner

spaze commented Oct 22, 2021

Thanks, good job! I've just merged it and released 1.10.0.

ruudk added a commit to ruudk/phpstan-disallowed-calls that referenced this pull request Oct 26, 2021
Forgot to add this in spaze#87
Fixes:
```
Invalid configuration:
The option 'parameters › disallowedMethodCalls › 2 › allowCount' expects to be string|list|array, 1 given.
```
@ruudk ruudk deleted the allow-count branch October 26, 2021 14:58
@ruudk
Copy link
Contributor Author

ruudk commented Oct 26, 2021

I made a small error in my thinking while working on this PR.

This works fine:

parameters:
    disallowedMethodCalls:
        -
            method: 'Generated\MyNamedQuery::execute*()'
            message: 'do not reuse GraphQL queries.'
            allowCount: 1

It will only allow one call to Generated\MyNamedQuery::execute*.

But I want to go to a situation where I have 1 rule generic rule

parameters:
    disallowedMethodCalls:
        -
            method: 'Generated\*Query::execute*()'
            message: 'do not reuse GraphQL queries.'
            allowCount: 1

and it should track it's usages by FQCN instead of by rule.

For example, I have 2 generated classes:

  • Generated\MyNamedQuery
  • Generated\MyOtherQuery

MyNamedQuery::execute() should be allowed to called once, and MyOtherQuery::execute() should be allowed to call once.

With the current setup, it tracks its usage on the rule and therefore blocks the second call to a separate class.

@ruudk
Copy link
Contributor Author

ruudk commented Oct 26, 2021

To solve that, I would suggest to scope the allowCount by FQCN.

Do you think that makes sense? So wildcard on method does count, but wildcard on class name does not count.

Since the feature is not working (see #88) I don't think it's a breaking change.

@spaze
Copy link
Owner

spaze commented Oct 26, 2021

Hey, yeah, usually making things work with other things is the hardest part :-)

To solve that, I would suggest to scope the allowCount by FQCN.

I think this is probably the only option we're left with. It would be cool if this can be checked run-time so that method: 'Generated\*Query::execute*()' + allowCount: 1 won't work and will give a warning to users that it won't work so they don't expect it would work. Do you think it would be possible (and easy-ish) to add the check?

@spaze
Copy link
Owner

spaze commented Oct 26, 2021

FYI, I have moved the new FQCN-related issue to #89

ruudk added a commit to ruudk/phpstan-disallowed-calls that referenced this pull request Oct 27, 2021
This reverts commit 9ae657d, reversing
changes made to 14138dc.
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.

Disallow classes to be used more than N times
2 participants