-
-
Notifications
You must be signed in to change notification settings - Fork 221
Fix PhpStan suggestions #378
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
base: master
Are you sure you want to change the base?
Conversation
…open-to-window links
|
|
||
| if ( | ||
| $this->email | ||
| && $this->mailer |
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.
this is wrong fix, instead the type of the $mailer property should be changed to callable|null
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.
Ok, now I understand. In original implementation property $mailer was setted by __construct in all cases. But property is public and can be changed.
I reverted condition here and change property definition:
/** @var callable|null handler for sending emails */
public $mailer;
src/Bridges/Nette/TracyExtension.php
Outdated
| $item = new Nette\DI\Statement(['@' . $builder::THIS_CONTAINER, 'getService'], [substr($item, 1)]); | ||
| } elseif (is_string($item)) { | ||
| $item = new Nette\DI\Statement($item); | ||
| if (is_string($item)) { |
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.
why?
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.
Because in original implementation when you verify variable $item is string, second elseif condition does not make sense.
src/Tracy/Dumper/Dumper.php
Outdated
| $maxI = $maxLength * 4; // max UTF-8 length | ||
| do { | ||
| if (($s[$i] < "\x80" || $s[$i] >= "\xC0") && (++$len > $maxLength) || $i >= $maxI) { | ||
| if (($s[$i] < "\x80" || $s[$i] >= "\xC0") && ((++$len > $maxLength) || $i >= $maxI)) { |
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.
why?
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.
Operations priority might differ from what programmer expect. Brackets are safe.
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 it's not related the phpstan error it should not be part of this PR
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 should not mix code changes related to phpstan errors with code style changes in one PR
dbabb37 to
eb2e772
Compare
de3ad52 to
191c0d2
Compare
2b958bb to
f36b649
Compare
68b0ec8 to
344c772
Compare
c3c8bf0 to
6f7e1c3
Compare
84dee11 to
bd73e33
Compare
7799297 to
a386d38
Compare
f3f04e0 to
0a90e93
Compare
5ee33d7 to
8e708de
Compare
Fixed PhpStan suggestions.
New errors:
Old errors: