Fun Phan Fixes Part 3 #3544
Fun Phan Fixes Part 3 #3544
Conversation
ZainVirani
dismissed
their stale review
realized travis failed because there were still mismatched signatures
Fix another class of errors from Phan.
|
@ZainVirani Fixed them (hopefully).. |
| @@ -38,6 +38,12 @@ class DirectDataEntryMainPage | |||
| var $key; | |||
|
|
|||
| var $TestName; | |||
|
|
|||
There was a problem hiding this comment.
why is this the only varialbe that got a cooment ?
There was a problem hiding this comment.
It was the only one generating an error.
Without it phan was inferring that it's a string, and generating the error:
htdocs/survey.php:112 PhanTypeComparisonToArray string to array comparison
| @@ -147,7 +147,7 @@ class Mri_Protocol_Check_Violations extends \NDB_Menu_Filter | |||
| * | |||
| * @param integer $count The offset that this page is starting at | |||
| * | |||
| * @return none but as a side-effect populates $this->tpl_data['items'] | |||
| * @return void but as a side-effect populates $this->tpl_data['items'] | |||
There was a problem hiding this comment.
sure, I'll add them for consistency..
| * @access public | ||
| * @deprecated Use pselect wrappers instead | ||
| */ | ||
| function selectRow($query, &$result) | ||
| { | ||
| $this->select($query, $result); | ||
| $result = $this->pselect($query, array()); |
There was a problem hiding this comment.
shouldn't this function be deprecated ? (replaced by pselectRow)
is this changes related to phan fixes ?
There was a problem hiding this comment.
yeah, it should be.. but do you think 20.0 is the release to do it? It's been deprecated for a while now, so maybe it's finally time, but this change was just to fix an error from phan (I can't remember which)
There was a problem hiding this comment.
I think it should either be completely removed or ignored by phan but I dont think we should update these functions to avoid people using them in the future...
There was a problem hiding this comment.
Oh, now I remember: it was to fix the error:
"Call to deprecated function \Database::select() defined at php/libraries/Database.class.inc:568"
so that PhanDeprecatedFunction could be removed from the ignore list.
There was a problem hiding this comment.
then I guess the solution would be to remove those
There was a problem hiding this comment.
I'll send a PR after this to remove, but I don't think it should be done as part of this PR, and I'm not convinced that we're ready with all the legacy instruments that might still be calling it. It's going to be really painful for projects to upgrade, but at least with this change we can be sure we're not calling anything that's been deprecated in the main codebase
There was a problem hiding this comment.
Arguably with this change they have no more reason to stop calling these functions. I think it's been over 3 years these functions are deprecated.
We can do a very quick survey see the effect on projects. but yeah, it should not be part of this PR I agree
php/libraries/LorisForm.class.inc
Outdated
| @@ -2060,7 +2060,7 @@ class LorisForm | |||
| * @param array $arr A reference to an array to store the | |||
| * extracted values into. | |||
| * | |||
| * @return none, but will modify &$arr parameter passed as a side-effect | |||
| * @return void, but will modify &$arr parameter passed as a side-effect | |||
| @@ -603,7 +603,7 @@ class NDB_BVL_Feedback | |||
| * | |||
| * @return array | |||
| */ | |||
| function getThreadEntries($feedbackID) | |||
| static function getThreadEntries($feedbackID) | |||
There was a problem hiding this comment.
this change seems out of context
There was a problem hiding this comment.
It's to be able to remove "PhanStaticCallToNonStatic" from the ignored list. All the calls to it are static, so the method should be static too.
There was a problem hiding this comment.
Just confirmed it. Dismissed
|
@driusan seems to be having a phpcs issue but otherwise looks good. The only important comment is the "static" change, can you explain it ? |
This builds on #3535 to also fix the class of errors "PhanTypeMissingReturn"
This primarily involves changing the return comment from "none" (or similar) to "void", or fixing it to "void" on functions that don't return something when they claim to.
(The changes for this PR are entirely contained in 3a92d25, everything else in the diff is inherited from #3535 )