Starting with CI

[MatthewVita]

@bradymiller,

For issue #489

This is a rather exciting piece of the Modernization Project. Although it's just doing linting at the moment, I'm sure you can see the more advanced usages of CI as the codebase becomes more modern (unit tests, style cop checks, automated tests, etc).

Here's the build on my fork (runs on all PHP versions we support):

When this is brought into the main codebase, stats will appear on each pull requests in real time such as:

Here's the build badge on my fork (with some README cleanups):

All that needs to be done to merge this is sign up for https://travis-ci.org, click on OpenEMR, open up the badge config modal:

(Obviously replace with OpenEMR master and master branch)

...and update the badge URL on the README.

Thanks,
Matthew

[MatthewVita]

Note that there's some TODOs with this because I excluded the following dirs from the lint check:

• interface/reports (this one is mess... lots of broken code)
• contrib/util
• library/edihistory

With that said, I think it's more important to get this checked in and deal with cleaning these up later. Perhaps some of the interface/reports and contrib/util code can be removed because it's a) broken in some places and b) probably not used (I realize that contrib/util has a lot of code that needs to stay, but there are some really old perl scripts in there)... would probably want to follow up with the original authors if they are active on OpenEMR

[bradymiller]

@MatthewVita ,
This is awesome!

It is set up and appears to be working well. Just brought in the README badge fix and it did the build testing etc.

We should document this function (and future plans) on the "Continuous Integration" wiki page I placed here:
http://www.open-emr.org/wiki/index.php/OpenEMR_Wiki_Home_Page#Testing

-brady

[bobinson]

Good stuff. May be we need to update the wiki here : http://www.open-emr.org/wiki/index.php/Continuous_Integration

Further, apart from linting, are we running PHPCodesniffer etc ?

[MatthewVita]

Hi @bobinson,

I just updated the wiki page with useful information.

As far as PHPCodesniffer, we are not running it. I'm not sure what the next step is with our CI. Right now, CI just runs linting from PHP v5.4 up to PHP v7.1. I would like to run checks with our https://github.com/openemr/openemr/blob/master/.php_cs code style, but we'd have to update OpenEMR to use the same code style everywhere. This is a lot of friction and all forks/branches will have to catch up.

What do you suggest?

[robertdown]

We aren't running sniffing in CI but I'm beginning to get very picky about incoming code not following our standards. The only way to standardize code is to fix it as it comes in via PRs (First I suggest to the author to fix it, but will do it myself if it is simple enough).

@bradymiller has pointed out maybe not doing this in a PR where we are making code changes, and I agree that a PR with code changes should not intertwine code standards improvements UNLESS it's new code. This happens a lot with my UI updating; but there is no way around it. I won't write new code that follows the file's broken style (Nor do I suggest anybody else do it either).

Trying to find a solution where we can run code sniffing on just the code that has been changed in a PR

[bradymiller]

@MatthewVita ,
Would there be a way to start small. For example, for now to not allow php short tags since those actually produce bugs and always seem to slip in occasionally?

@robertdown ,
How tough would it be to go through openemr file by file and fix the coding styles (sounds like phpstorm could do this and sublime appears to have a plugin to help do this also)?
-brady

[MatthewVita]

@bradymiller Looks like PHPCheckStyle handles this: https://github.com/PHPCheckstyle/phpcheckstyle/blob/ba40d01e7230b13f01823d6e8dbd3bb32790695c/test/sample/bad_php_tags_end_not_needed.php#L1

@robertdown do you think it make sense to run PHPCheckStyle on the existing code base, fix everything, and then add PHPCheckStyle to our CI?

[robertdown]

@MatthewVita We already have phpcodesniffer incorporated into the project. We even have a ruleset to help with the transition (Basically PSR2 with no namespace enforcement)

@bradymiller phpcodesniffer has a way to automatically fix stuff. We could run it and see what happens. Trying to manually fix everything would take months

[bobinson]

@MatthewVita @bradymiller @robertdown ,

May be for the incoming code via Pull requests, we can think of running automatic fixes with

for example,

phpcs admin.php
this gives an option like following:

----------------------------------------------------------------------
PHPCBF CAN FIX THE 67 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

output of " phpcbf admin.php"

Processing admin.php [PHP => 923 tokens in 148 lines]... DONE in 18ms (67 fixable violations)
        => Fixing file: 0/67 violations remaining [made 3 passes]... DONE in 41ms

It fixed high level stuff like:

 139 | ERROR | [x] TRUE, FALSE and NULL must be lowercase; expected
     |       |     "false" but found "FALSE"

IMHO this can give us a good starting point and later we can incorporate stricter static code analysis with tools like codeclimate.

[robertdown]

So later I'll run phpcs on the whole project (excluding third party stuff). I'll see what it says can be fixed and run the auto fixer and post the changes in a PR that I think the three of us need to all glance at. Not that I don't trust the auto fixer, it's just a matter of shear number of files changed. Also, I'm gonna write up a section on the dev docs on how various IDE's can show you errors. Phpstorm handles this very well, as I'm sure many of the big editors do

[bradymiller]

How about we just do a file or 2 for now so we can see how it looks at a low level? Then can do larger chunks (and in the the end the entire codebase when comfortable).

[MatthewVita]

Just saw @robertdown's output. It's pretty daunting. However, I'm with @bradymiller on changing a few files at a time to get an idea of what this will look like in the long run.

[robertdown]

I didn't see Brady's comment until after running the auto fixer..... will post the PR. We can always just pick a few files at first

[bradymiller]

Sounds good. Looking forward to seeing this PR, which will break many records (most files touched, most lines modified, ... :) ). Then can go from there. -brady

[MatthewVita]

Part of me says we should just check in the PR......... we have to assume correctness in the automated tool though.

[robertdown]

See #657