Add Psalm to pipeline

[bdsl]

Q	A
Bug fix?	no
New feature?	no
Doc updated	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT

As discussed at #1091 (comment) , this PR adds psalm to the travis config, and will cause a build failure if psalm detects a new issue, for example an InvalidDocblock in SerializerInterface , where I added psalm-specific annotations yesterday.

It's normal for any static analyser to find many issues on first run, and since it would be too much work to fix these all at once, the existing issues are listed in .psalm/baseline.xml and will not fail the build. However they do give a good idea of what sort of issues psalm will complain about in future, so I would suggest reviewing this (or the output at https://travis-ci.org/bdsl/serializer/jobs/541397116 ) before merging this PR.

Since this PR is all about the build pipeline, I suggest reviewing the build history at https://travis-ci.org/bdsl/serializer/builds

[bdsl]

It's possible to cut the baseline file down from 468 lines to 256 by running:

vendor/bin/psalm.phar --alter --issues=all
vendor/bin/psalm.phar --update-baseline

If you do that psalm will make automated changes to about 50 files. I wouldn't want to commit the changes without knowing the codebase better and reviewing them carefully, and I certainly wouldn't commit all the changes psalm makes without further editing.

[goetas]

i think .psalm/baseline.xml should be in the list of ignored files... isn't?

[bdsl]

@goetas No, that needs to be committed. The updated docs in c21af97 might help.

[bdsl]

When I first ran psalm it found 217 errors. Without the baseline file every build would fail until all those errors were fixed, or annotated with @psalm-suppress. Since we have the baseline file builds will fail only if new errors appear.

[bdsl]

@goetas Did you get a chance to look at this? Any thoughts?

[goetas]

I was really hoping to have it enabled somehow without the "baseline" file... that essentially condones the existing bad code...

[bdsl]

It acknowledges that it exists, I wouldn't say it condones it. And it won't all be bad - some may just be things that psalm doesn't understand correctly, and some I think is just docblocks that need updating or deleting.

You could try to fix or suppress all the issues listed in the baseline file first, and then add Psalm to the pipeline, but if there's active development on psalm people might add code that creates new issues in the meantime - that's what the baselining will stop.

Or you could add an issue or several to the issue tracker to resolve the things in the baseline file and them merge this as is.

I do think that the longer an issue has exited in the codebase the less likely it probably is to be a big problem, since it's had more time to be found, complained about and fixed, so baselining makes sense to me.

Decision for the maintainers about how to deal with this. Feel free to close the PR if this isn't your favoured approach.

[bdsl]

Another option is to delete the baseline and change vendor/bin/psalm.phar --show-info=false in the test script to vendor/bin/psalm.phar --show-info=false || true . Then psalm will run and display the list of issues in every build, but never cause a build failure until you delete the || true. But I'd worry that people won't look at the psalm output.

[goetas]

By running psalm with your current configs, it shows ~300 errors, adding few dockblocks and tweaks in the psalm i took down those errors to ~120. The remaining error-reports looks legit and should/could be fixed.

Thus I'm not interested in adding psalm just for the sake of having it, prefer to have it enabled and analyze/fix properly the whole src directory.

Adding psalm and disable use the baseline file feels as tricking the system (same for the |true option)

[bdsl]

Ok. I will close this PR then, but of course if you want to fix the whole src repository you're welcome to merge it as part of that.