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

PHPStan configuration and basic fixes #1896

Merged
merged 12 commits into from
Apr 6, 2021

Conversation

xmorave2
Copy link
Contributor

@xmorave2 xmorave2 commented Mar 23, 2021

This PR contains phing and travis configuration for running PHPStan checks on codebase and also fixes for level 0 of PHPStan analysis

TODO

  • Integrate into Jenkins when merging

@demiankatz
Copy link
Member

Thanks for this, @xmorave2! I've added a TODO to myself to hook this up with Jenkins when we merge it. I plan to merge many of your fixes to dev in advance of this full PR, which I think will make it easier to review what remains here. We may also want to discuss whether this complements or supersedes the work with Psalm in #1764. I'm not opposed to using multiple tools to maximize quality, but if one or the other is clearly better, it might make sense to choose. I'll add a more detailed review here once I've whittled things down by merging fixes.

@xmorave2
Copy link
Contributor Author

@demiankatz From my point of view we could take this as complement to Psalm to maximize code quality. I would not say the one tool is better then the other one, both have their pluses and both are actively developed. Both are extensible, but only phpstan has plugin specific for laminas framework (but I don't try it yet, so maybe, we don't need it)

For me the PHPStan has more clear issues description. If we adopt the PHPStan I am ready to continue on fixing code for more strict rules.

@demiankatz
Copy link
Member

Great, thank you again, @xmorave2, and I agree that the two tools will likely be complementary. I'll work with you on getting this finished up first since I think it is going to be less work than Psalm, and once we merge the fixes from this work into the Psalm branch, we will see what remaining issues are reported there.

@xmorave2
Copy link
Contributor Author

Great, thanks @demiankatz

@demiankatz demiankatz force-pushed the phpstan branch 10 times, most recently from ba94c31 to b7bd8d1 Compare March 23, 2021 21:09
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks again for all of this work, @xmorave2 -- it's been tremendously helpful. I committed most of the fixes from here to the dev and release-7.1 branches, reducing this just to the actual PHPStan setup, and a few outstanding changes that I had questions about. In a few cases, I solved problems differently, but the vast majority of your work was carried through as-is. Hopefully it won't take too much more work to figure out the issues highlighted below, and then perhaps we can merge this!

composer.json Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Search/UrlQueryHelper.php Outdated Show resolved Hide resolved
tests/phpstan.neon Outdated Show resolved Hide resolved
@demiankatz
Copy link
Member

Thanks again, @xmorave2, for all the work on this! I've incorporated the remaining fixes into the dev branch (some with minor adjustments, like refactoring of the UserCreationTrait to simplify matters), and I think this is now just about ready to merge. I'm going to be on vacation until April 6, so I am not going to merge it until after I get back -- but I hope to do it shortly thereafter, along with the necessary adjustments to use this in our Jenkins instance.

@demiankatz
Copy link
Member

I've gotten clearance to remove the LBS4 driver, so I've gone ahead and done that to further simplify this PR. I'll look at Jenkins integration next; once that is figured out, we're ready to merge. Thanks again for your help!

@demiankatz
Copy link
Member

I think I've taken care of the Jenkins integration; merging now to see how it goes! Now the question is if/when to start working on higher PHPStan levels. :-)

@demiankatz demiankatz merged commit bef3819 into vufind-org:dev Apr 6, 2021
@xmorave2 xmorave2 deleted the phpstan branch April 7, 2021 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants