-
Notifications
You must be signed in to change notification settings - Fork 442
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
Show custom tip for unfound Drupal symbols #2603
Conversation
You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x. |
f951720
to
29b7f09
Compare
- add optional $className argument to discoveringSymbolsTip() method - if that name starts with 'Drupal/' show link to phpstan-drupal project - pass the symbol name to all 22 calls to discoveringSymbolsTip()
29b7f09
to
d0eaa36
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And please rebase on top of 1.10.x, as the bot says.
@@ -41,7 +41,7 @@ public function processNode(Node $node, Scope $scope): array | |||
} | |||
|
|||
return [ | |||
RuleErrorBuilder::message(sprintf('Function %s not found.', (string) $node->name))->identifier('function.notFound')->discoveringSymbolsTip()->build(), | |||
RuleErrorBuilder::message(sprintf('Function %s not found.', (string) $node->name))->identifier('function.notFound')->discoveringSymbolsTip((string) $node->name)->build(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to do (string) $node->name)
only once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand your comment below, you actually don't want the argument at all in these cases, since here the symbols are for functions, not classes. Right?
@@ -73,7 +73,7 @@ private function checkConstant(Node\Name $name): ?IdentifierRuleError | |||
{ | |||
if (!$this->reflectionProvider->hasConstant($name, null)) { | |||
return RuleErrorBuilder::message(sprintf('Used constant %s not found.', (string) $name)) | |||
->discoveringSymbolsTip() | |||
->discoveringSymbolsTip((string) $name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the passed argument in cases where it's not a class name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So don't do what you asked me to do in the issue?
Yes, all 22 calls should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I did ask about non-classes in a comment on the issue.)
One more clarification. I assume that although we're calling the new argument $className, what we want to test is the name of the thing which was not found, even if that "thing" isn't a class (for example, an interface, contstant, trait, etc.). Right?
{ | ||
if (isset($className) && substr($className, 0, 7) === 'Drupal/') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A test would be nice, I'm pretty sure this code is wrong (It's Drupal\
, not Drupal/
) :) And please use str_starts_with
.
The test should be written for one of the impacted rules, it doesn't matter which one. An error tip is tested with a third array item (https://phpstan.org/developing-extensions/rules#testing-the-rule) - besides the message and the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right about the backslash. My mistake.
And please use
str_starts_with
.
Hmm. I was under the impression that PHPStan supports PHP >= 7.2. The str_starts_with()
function was only introduced in PHP 8.
I'll see what I can come up with for a test.
I did see what the bot said.
I decided that as long as it kicked in at some time in the future, it didn't really make any significant difference in the long run when that was. But I'll do the rebase if that's the only way the PR will be accepted. 😎 |
Sorry, I'm going to drop this PR. I tried the rebase, but it failed, with a merge conflict. I could probably figure that out, but I also looked into what would be involved in creating a test, and I realized I had no idea where to start. I've created plenty of tests for other environments, but I don't know enough about this particular environment's expectations. I was willing to invest some effort into saving other developers from the difficulties I ran into trying to figure out why the tool wasn't working as advertised, but I've come to the realization that I'm probably not in the best position to accomplish that goal. |
Closes #9837