Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix issue with modified backtrace #250
Conversation
php allows the backtrace arguments to be modified via the backtrace. By using array_map we make sure that we do not overwrite references.
| @@ -15,4 +15,8 @@ | |||
| <rule ref="SlevomatCodingStandard.Classes.SuperfluousAbstractClassNaming.SuperfluousPrefix"> | |||
| <exclude-pattern>*/src/*/Abstract*.php</exclude-pattern> | |||
| </rule> | |||
|
|
|||
| <rule ref="SlevomatCodingStandard.Classes.UnusedPrivateElements.UnusedMethod"> | |||
williamdes
Aug 12, 2020
•
this could be moved to the source file, but it depends on your code policy.
this could be moved to the source file, but it depends on your code policy.
williamdes
Aug 12, 2020
My proposed change would avoid this error
My proposed change would avoid this error
| static function (array $call) use ($flatten) : array { | ||
| array_walk_recursive($call['args'], $flatten); | ||
| function (array $call) : array { | ||
| $call['args'] = array_map([$this, 'flattenArguments'], $call['args']); |
williamdes
Aug 12, 2020
Give the php version requirements I would recommend (not tested):
https://github.com/phpDocumentor/ReflectionDocBlock/blob/master/composer.json#L17
Suggested change
$call['args'] = array_map([$this, 'flattenArguments'], $call['args']);
$call['args'] = array_map(function($value) { return $this->flattenArguments($value); }, $call['args']);
Give the php version requirements I would recommend (not tested):
https://github.com/phpDocumentor/ReflectionDocBlock/blob/master/composer.json#L17
| $call['args'] = array_map([$this, 'flattenArguments'], $call['args']); | |
| $call['args'] = array_map(function($value) { return $this->flattenArguments($value); }, $call['args']); |
GrahamCampbell
Aug 12, 2020
Contributor
👎 for changing this. The array-callable syntax is much shorter and clearer?
williamdes
Aug 12, 2020
Nope, for me this syntax is "compilation" compatible and explicit for any code searcher or analyser :)
Don't you agree?
Nope, for me this syntax is "compilation" compatible and explicit for any code searcher or analyser :)
Don't you agree?
williamdes
Aug 12, 2020
And that would remove the need of ref="SlevomatCodingStandard.Classes.UnusedPrivateElements.UnusedMethod">
And that would remove the need of ref="SlevomatCodingStandard.Classes.UnusedPrivateElements.UnusedMethod">
GrahamCampbell
Aug 12, 2020
Contributor
Both can be formally detected. I guess it just hasn't been implemented by that code style package. psalm itself certainly recognises uses like that, in it's core stuff.
Both can be formally detected. I guess it just hasn't been implemented by that code style package. psalm itself certainly recognises uses like that, in it's core stuff.
|
@williamdes thanks for the review, are you able to test this with doctum? |
Not yet, it will take me some time to test it because I can not reproduce it locally.. |
|
If you can provide a phar I can reproduce the issue in docker |
Well if you succeed that would help me because I could not replicate this on my workstation or docker instances Follow nikic/PHP-Parser#687 (comment) to replicate the issue, @nikic seems to have reproduced it |
|
That's what I already did, but can you provide me a phar with the fix included? |
Sure, here it is: https://doctum.long-term.support/releases/5.0.2-dev/doctum.phar You can see on my manifest diff that I applied your patch code-lts/doctum@d2695c0#diff-d313403d9a8ddaf7d325ee4001bd3fb6 |
|
The patch works fine on TravisCI ! |
d870572
into
master
php allows the backtrace arguments to be modified via the
backtrace. By using array_map we make sure that we do not overwrite
references.
fixes #249