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

QueryPlanAnalyzerRule inconsistent with documentation #510

Open
hemberger opened this issue Feb 16, 2023 · 2 comments
Open

QueryPlanAnalyzerRule inconsistent with documentation #510

hemberger opened this issue Feb 16, 2023 · 2 comments

Comments

@hemberger
Copy link
Contributor

The QueryPlanAnalyzerRule documentation says:

the callable format is class::method#parameterIndex, while the parameter-index defines the position of the query-string argument.

This is consistent with the parsing in QueryPlanAnalyzerRule::processNode:

sscanf($classMethod, '%[^::]::%[^#]#%i', $className, $methodName, $queryArgPosition);

However, in QueryPlanAnalyzerRule::analyze, the $queryArgPosition is not used, and it assumes that the query string is the first argument, and the parameters are (optionally) the second argument.

Personally, I don't have a need to specify the position of the query string argument, but maybe both could be supported, if desirable? Something like:

the callable format is class::method#parameterIndex, while the parameter-index defines the position of the query-string argument. If the parameter-index is omitted, then phpstan-dba assumes the method takes a query-string as a 1st and the parameter-values as a 2nd argument.

If that makes sense and would be helpful, I'd be happy to submit a PR. Thanks!

@staabm
Copy link
Owner

staabm commented Feb 16, 2023

However, in QueryPlanAnalyzerRule::analyze, the $queryArgPosition is not used, and it assumes that the query string is the first argument, and the parameters are (optionally) the second argument.

you are right. the $queryArgPosition is not really used. I think we should simplify the config format (while beeing compatible with the old format for BC reasons) and adjust the docs.

Personally, I don't have a need to specify the position of the query string argument, but maybe both could be supported, if desirable? Something like:

I don't want to implement it right now, if noone needs it yet. we can discuss again when someone comes up with a valid use case for it

@hemberger
Copy link
Contributor Author

Okay, so if I'm understanding you correctly, I won't make a PR at this time. Please feel free to assign this issue to me in the future, if you'd like!

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

No branches or pull requests

2 participants