Skip to content
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

[Test] Add phpstan tool to test suite and fix detected errors. #4928

Merged
merged 18 commits into from
Feb 3, 2020
Merged

[Test] Add phpstan tool to test suite and fix detected errors. #4928

merged 18 commits into from
Feb 3, 2020

Conversation

johnsaigle
Copy link
Contributor

@johnsaigle johnsaigle commented Jul 4, 2019

Brief summary of changes

I found a cool static analysis tool called PHP Stan which is like phan but much faster and flags additional issues.

This PR cleans up some of the errors it flags.

I'm going to add another commit that adds phpstan as a dependency but it requires PHPUnit >= 7 to work properly. This should be solved by #4433 so I'm marking this PR as blocked for now.

@johnsaigle johnsaigle added Category: Feature PR or issue that aims to introduce a new feature Cleanup PR or issue introducing/requiring at least one clean-up operation Testing PR contains test plan or automated test code (or config files for Travis) Meta PR does something that organizes, upgrades, or manages the functionality of the codebase State: Needs work PR awaiting additional work by the author to proceed labels Jul 4, 2019
@johnsaigle
Copy link
Contributor Author

Forgot to do make checkstatic. There are some phan errors I'll fix here.

@xlecours
Copy link
Contributor

xlecours commented Jul 9, 2019

Nice catches!
I see that you choose var instead of a visibility keyword. Is it recommended?

@johnsaigle
Copy link
Contributor Author

@xlecours No I don't think the tool cares. As far as I know var is the same as public in PHP. I can change it if public is preferred.

@xlecours
Copy link
Contributor

I usually prefer explicit statements. But in this case, the visibility would need to be set to public, because there might be calls like $that->_variablename, and ideally they should be set to protected or private (in a utopian futur).

[TL;DR] leave them var

@driusan
Copy link
Collaborator

driusan commented Jul 10, 2019

"public" is better than "var" because it's explicit. The "var" keyword shouldn't be used outside of legacy code.

@johnsaigle
Copy link
Contributor Author

There are some phan issues that exist on the 21.0-release branch.. not sure how they got there.

Many of these are/will be addressed by #4914 so I'm going to mark this PR as Blocked until that one is merged.

@johnsaigle johnsaigle added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Jul 10, 2019
@johnsaigle johnsaigle changed the base branch from 21.0-release to major July 17, 2019 15:44
@johnsaigle johnsaigle removed the State: Needs work PR awaiting additional work by the author to proceed label Sep 23, 2019
@johnsaigle johnsaigle added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Oct 17, 2019
@johnsaigle johnsaigle removed State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Oct 29, 2019
@johnsaigle johnsaigle changed the title Fix/clean problems in codebase detected by phpstan [Test] Add phpstan tool to test suite and fix detected errors. Oct 29, 2019
@johnsaigle
Copy link
Contributor Author

This PR now includes phpstan in the test suite. I was able to run it at max level and fix all detected errors! Pretty sweet.

@johnsaigle johnsaigle requested a review from driusan October 29, 2019 17:00
@johnsaigle
Copy link
Contributor Author

Travis is failing because PHP 7.2 is too old to run the dependencies for this version.

Since this is unlikely to go into LORIS 22 anyway, I think I'll punt this until 7.3 is required

@johnsaigle johnsaigle added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Oct 29, 2019
@johnsaigle
Copy link
Contributor Author

To be rebased onto the "Dev" branch after 22 is released.

@johnsaigle johnsaigle added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Nov 12, 2019
@johnsaigle johnsaigle removed the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Dec 2, 2019
@johnsaigle johnsaigle changed the base branch from major to master December 3, 2019 19:24
@johnsaigle johnsaigle removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Dec 3, 2019
@johnsaigle johnsaigle removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jan 20, 2020
@johnsaigle johnsaigle dismissed ridz1208’s stale review January 27, 2020 16:04

Suggestions incorporated

@johnsaigle johnsaigle requested a review from ridz1208 January 27, 2020 16:04
Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good cleanup. Now we need to fix all those public variables. At least we now have a better idea of how many there are...

Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly looks good except for some minor things

php/libraries/BVL_Feedback_Panel.class.inc Outdated Show resolved Hide resolved
php/libraries/BVL_Feedback_Panel.class.inc Outdated Show resolved Hide resolved
php/libraries/Database.class.inc Outdated Show resolved Hide resolved
php/libraries/LorisForm.class.inc Show resolved Hide resolved

$formArray = $renderer->toArray();
}
$formArray = $this->form->toElementArray();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the other part of the else statement? What is this trying to solve? I think this change of behaviour is likely to break things with different types of instruments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there was an issue with HTML_QuickForm_Renderer_Array because it doesn't exist in the code.

I made this PR 6 months ago so I'm not sure about the details.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did a little investigation of the impact of losing this else case and I think it means the API would be broken for the endpoint which gets the instrument in a JSON format for projects that had old instruments that couldn't be updated to LorisForm and were still using QuickForm.

There was only 1 such instrument that I could find when I searched the IBIS repo, and I'm not aware of anyone/thing depending on that endpoint of the API, so I guess it should be fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xlecours or @ridz1208 do you want to chime in or know of anything using the /projects/$proj/instruments/$instrumentname endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add a note in the CHANGELOG about this if it ends up being a breaking change.

@johnsaigle johnsaigle added State: Needs work PR awaiting additional work by the author to proceed Needs formatting PR requires formatting fixes to meet our coding standards (PHPCS, phan, etc.) and removed State: Needs work PR awaiting additional work by the author to proceed labels Jan 28, 2020
@johnsaigle johnsaigle added Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects and removed Needs formatting PR requires formatting fixes to meet our coding standards (PHPCS, phan, etc.) labels Jan 29, 2020
@johnsaigle
Copy link
Contributor Author

@driusan I've incorporated your suggestions. I added a note in the CHANGELOG about QuickForm complications and added the Caveat... label.

Could you re-review?

@johnsaigle johnsaigle requested a review from driusan January 29, 2020 21:05
*/
var $_feedbackThread;
protected $feedbackThread;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc (@private) and the code (protected) here still don't seem to agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, just fixed it above

php/libraries/Module.class.inc Outdated Show resolved Hide resolved
&& (is_integer($value));
return intval($value) > 0
&& strlen($value) <= self::SESSIONID_MAX_LENGTH
&& ctype_digit($value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was wrong with is_integer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_int() returns true if the argument is an integer type, ctype_digit() takes a string argument and returns true if all the characters in the string are digits.

The value is passed as a string so is_integer() always returns false

https://stackoverflow.com/questions/236406/is-there-a-difference-between-is-int-and-ctype-digit

test/phpstan-loris.neon Show resolved Hide resolved
@driusan driusan merged commit ab78eee into aces:master Feb 3, 2020
@ridz1208 ridz1208 added this to the 23.0.0 milestone Feb 25, 2020
driusan pushed a commit that referenced this pull request Feb 27, 2020
Reverses changes from #4928 that renames $ValidityEnabled to $validityEnabled and $ValidityRequired to $validityRequired. The variabes are UpperCamerCase elsewhere on Loris. This removes the following error that does not allow the Validity flags to render in instruments' control panels

PHP Notice:  Undefined property: NDB_BVL_Instrument_aosi::$validityEnabled in /var/www/loris/php/libraries/NDB_BVL_InstrumentStatus_ControlPanel.class.inc on line 101
PHP Notice:  Undefined property: NDB_BVL_Instrument_aosi::$validityRequired in /var/www/loris/php/libraries/NDB_BVL_InstrumentStatus_ControlPanel.class.inc on line 102
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Feature PR or issue that aims to introduce a new feature Cleanup PR or issue introducing/requiring at least one clean-up operation Meta PR does something that organizes, upgrades, or manages the functionality of the codebase Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects Testing PR contains test plan or automated test code (or config files for Travis)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants