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

Install phpstan and fix some errors #62

Merged
merged 14 commits into from
Feb 15, 2023
Merged

Install phpstan and fix some errors #62

merged 14 commits into from
Feb 15, 2023

Conversation

mpesari
Copy link
Contributor

@mpesari mpesari commented Jan 25, 2023

Description

Run PHPStan (static analyser) in workflow and fix some errors found by it.

You can run it manually with: vendor/bin/phpstan

Needs rebase after #59 is merged.

@@ -28,6 +28,14 @@ jobs:
- name: Run linter
run: composer lint

- name: Run phpstan
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to have phpstan in composer scripts as well. Could be as part of composer lint -command, since it mostly is for code quality. But since it has own existing action in github, maybe own command would be better in pipeline sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's important to run static analysis with different PHP versions to ensure interoperability with promised legacy versions (ie not use too new language features). Not sure if this can be done without using the separate action? I'm not very familiar with GitHub Actions.

Maybe add composer analyse? This way it would still be separate in the pipeline. In our project we also use Psalm which could be added to the same composer script.

Copy link
Contributor

Choose a reason for hiding this comment

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

Analyze sounds good command.
Actions can have only one command if I recall correctly. But we can always add command like composer check or something that runs composer analyze and composer lint as pre commit hook, when adding those later in line.
But that is more sens to set when code quality is in level that it should be in order to ensuring maintaining it that way. This repo is really old, from Checkout era from different version control, so it still has probably quite few out dated conventions and such.

You can also add psalm in this PR as well, I don't think it breaks too much, and would anyway makes sense to have, since there's already plan to add it anyway when have time to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like some of the dependencies pulled in by psalm are not compatible with PHP 7.3 :)

I made another branch psalm based on this one, but it can't be merged right now.

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 added the command composer analyze which now just runs phpstan.

Let me know when #59 is merged - I'll rebase afterwards

@loueranta-paytrail loueranta-paytrail merged commit 9430c8e into paytrail:master Feb 15, 2023
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