PhabricatorInlineCommentController calls $this->canEditInlineComment($viewer, $inline) which is undefined in this very class.
The two classes with actual implementations of that method (DiffusionInlineCommentController, DifferentialInlineCommentEditController) extend PhabricatorInlineCommentController.
Thus define canEditInlineComment() as an abstract method in PhabricatorInlineCommentController and use assert() calls in both implementation classes to avoid that the $inline parameter types are not contravariant.
Details
Details
- Reviewers
valerio.bozzolan - Group Reviewers
O1: Blessed Committers
Run static code analysis; read/grep the code.
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Branch
- canEditInlineComment
- Lint
Lint Passed Severity Location Code Message Advice src/applications/differential/controller/DifferentialInlineCommentEditController.php:125 XHP41 Unnecessary Double Quotes Advice src/applications/diffusion/controller/DiffusionInlineCommentController.php:92 XHP41 Unnecessary Double Quotes - Unit
Tests Passed - Build Status
Buildable 1470 Build 1470: arc lint + arc unit
Event Timeline
Comment Actions
I'm personally OK with this, premising that Phorge never used assert(), as far as I can see.
Maybe it's because assert() raises problems only if you have its related flag activated (https://www.php.net/manual/en/ini.core.php#ini.zend.assertions) and otherwise you have just a skipped check. I understand that this is probably enough to avoid this approach since the performance benefits probably are not greater than downsides.
What else? Maybe we can just explicitly cast. Like:
$line = (PhabricatorAuditInlineComment) $line;
This will be a don't-care for base cases but it should crash with a reasonable readable exception if this can't be done. Plus, it makes static analyzers (even more?) happy.
Plus, we don't have to fix that nonsense linter message lol
Comment Actions
...but don't follow my tip, since the above thing forces to a specific class and it's different than the old and already-proposed behavior
Comment Actions
Indeed, we seem to prefer throwing exceptions after if (!($foo instanceof PhutilSortVector)), or use assert_instances_of() on an array, using gettype() and get_class().
Comment Actions
Going to abandon for now as I'm not convinced either that this is something worth to change only to make PHPStan happier.
Note that https://we.phorge.it/book/libphutil/function/assert_instances_of/ might be an option when/if I want to look into this again at some future point.