Closes #7123: Rules about get_subscribed_events #7124
Conversation
Miraeld
added
the
type: enhancement
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more Footnotes
|
|
|
||
| // Get the function/method name from the scope | ||
| $functionName = $scope->getFunctionName(); | ||
| if ('get_subscribed_events' === $functionName) { |
There was a problem hiding this comment.
@Miraeld two things:
- It would be better to bail out here to keep the logic simple.
- We also need to check that we extend from the interface
Subscriber_Interfaceto prevent side effects
| $functionName = $scope->getFunctionName(); | ||
| if ('get_subscribed_events' === $functionName) { | ||
| // Check if the return expression is an array | ||
| if ($node->expr instanceof Node\Expr\Array_) { |
There was a problem hiding this comment.
We need to bail out here too
| // Check if the return expression is an array | ||
| if ($node->expr instanceof Node\Expr\Array_) { | ||
| foreach ($node->expr->items as $item) { | ||
| if ($item instanceof Node\Expr\ArrayItem) { |
There was a problem hiding this comment.
Same here with a continue
| if ($methodValue instanceof Node\Scalar\String_) { | ||
| // Simple structure: array('hook_name' => 'method_name') | ||
| $errors = $this->checkIfMethodExistsInClass($methodValue->value, $scope, $methodValue); |
There was a problem hiding this comment.
We can bail out here
| } elseif ($methodValue instanceof Node\Expr\Array_) { | ||
| // More complex structures: array or nested array | ||
| foreach ($methodValue->items as $subItem) { | ||
| if ($subItem instanceof Node\Expr\ArrayItem) { |
There was a problem hiding this comment.
Same here, we can bail out
| // More complex structures: array or nested array | ||
| foreach ($methodValue->items as $subItem) { | ||
| if ($subItem instanceof Node\Expr\ArrayItem) { | ||
| if ($subItem->value instanceof Node\Scalar\String_) { |
There was a problem hiding this comment.
and same here with continue
Description
Fixes #7123
Explain how this code impacts users.
Type of change
Detailed scenario
Explain how to reproduce the issue (for bugs) or how to trigger the new code. You can refer to the GitHub issue or grooming if it is still applicable.
Technical description
Documentation
Explain how this code works. Diagrams & drawings are welcome.
New dependencies
List any new dependencies that are required for this change.
Risks
List possible performance & security issues or risks, and explain how they have been mitigated.
Mandatory Checklist
Code validation
Code style
Additional Checks