WIP: Introduce CS Fixer

[grummbeer]

This PR is a continuation of the work started by @dregad to add a CS fixer. This is the first step with adding the "easy coding standard" to the CI. No fixers are added yet, these will be added in small steps to keep the changes small and reviewable.

Issue #34400

First attempt: master...dregad:mantisbt:codesniffer

Some sniffs are outdated, e.g. "Generic.Arrays.DisallowShortArraySyntax", others may no longer be relevant in other ways. But there is a minimum of basic fixer that can help a lot. Better having just a few basic fixers that work than nothing at all. For example spaces_inside_parentheses > func( $arg, $arg2 ) to reduce situations like this.

Todo:

• CI. Adding "allow_failure" to the "cs" job. It should only be used for information and not cause the entire pipline to fail.
• check if or which files can be deleted e.g. "build/check_formatting.php"

[grummbeer]

Added some basics to see the results of ecs check

Last run: https://app.travis-ci.com/github/mantisbt/mantisbt/jobs/620277330

• OK EncodingFixer
• OK LineEndingFixer
• NoTrailingWhitespaceFixer
• SingleBlankLineAtEofFixer
• NoWhitespaceInBlankLineFixer
• SingleQuoteFixer
• OK FullOpeningTagFixer
• OK EchoTagSyntaxFixer
• OK NoClosingTagFixer
• 1x ConstantCaseFixer
• 2x LowercaseKeywordsFixer
• SpacesInsideParenthesesFixer::class, ('space' => 'single')

SpacesInsideParenthesesFixer looks not good for multiline

@@ -29,7 +29,7 @@
 /**
  * @var \Slim\App $g_app
  */
-$g_app->group('/issues', function() use ( $g_app ) {
+$g_app->group( '/issues', function() use ( $g_app ) {
 	$g_app->get( '', 'rest_issue_get' );
 	$g_app->get( '/', 'rest_issue_get' );
 	$g_app->get( '/{id}', 'rest_issue_get' );
@@ -74,7 +74,7 @@
 	$g_app->get( '/{id}/files', 'rest_issue_files_get' );
 	$g_app->get( '/{id}/files/{file_id}/', 'rest_issue_files_get' );
 	$g_app->get( '/{id}/files/{file_id}', 'rest_issue_files_get' );
-});
+} );

^ }) looks better … but I think it's more important that they all look the same, rather than how exactly they look.

• Add some basic fixer (e.g. those that don't complain) with this PR?
• Add one rule at a time, check results, then do ecs --fix for a couple of files, repeat this, until the added fixer pass?
• Add more fixers and select the ones with the fewest hits and fix them first?

[grummbeer]

Dear maintainers, please leave some hints, suggestions or advice.

Having limits on time for reviews in mind, what do you think is a viable approach?

[dregad]

Thanks a lot @grummbeer for picking up and improving on this half-baked attempt I started with PHP_CodeSniffer so many years ago.

I started reading #34400 but it's a long post and I don't have enough time at the moment to go in details. Will give feedback later.

-});
+} );

^ }) looks better … but I think it's more important that they all look the same, rather than how exactly they look.

} ); is actually correct according to the guidelines, IMO. And indeed it's better to be consistent.

For as long as you're developing and testing this, please disable all unnecessary build jobs by commenting them out in .travis.yml, so it does not unnecessarily consume build credits - as a nice side effect, it will make the CI execution faster for you, too. Thanks for your understanding.

As for moving forward, this kind of changes will likely introduce a lot of churn, that will cause merge conflicts with all other unmerged PRs and local development. Of course those should be easily fixable by running the fixer in the feature branch, but to reduce the impact I would rather have an incremental approach within a single PR that can be merged in one go, rather than a series of individual PRs.

What I mean by this:

• iterative approach (i.e add one or more rules, check, adjust rules if needed, apply fix and commit updated source code)
• number of rules to add per iteration does not matter, but I would suggest keeping change sets small to facilitate checks and review
• order in which rules are added I also leave up to you, whatever makes the job easier
• No force-rebase / squash commits ! like you've done in your recent contributions. I don't like it so much in general because it rewrites history and causes loss of traceability when multiple reviews are needed; it is much better to add incremental fix commits, and rebase/force-push just before merge.
• Here I would expect a one or more initial commits with the necessary setup (composer, ecs config), followed by N commits with code style fixes, i.e. one per (batch of) added rule(s).
• When ready to merge, the feature branch can be rearranged as and if needed

[dregad]

Just an idea for optimization, not sure if it's worth the effort or even feasible, but maybe the CI job could run only against the files modified in the submitted PR, instead of the whole code base ?

[grummbeer]

I started reading #34400 but it's a long post and I don't have enough time.

I know the "time situation", the issue is really verbose, too much. Maybe you do not need checking all rules. As we go in iterations, only the one from the current iteration is important. I try to do my best keeping the work for you as less as possible.

Here I would expect a one or more initial commits with the necessary setup (composer, ecs config), followed by N commits with code style fixes, i.e. one per (batch of) added rule(s).

To get it right. You mean this PR?

• commit: Add cs fixer (composer, travis, ecs.php)
• commit: Add SingleQuoteFixer > 80 affected files
• commit: Add LowercaseKeywordsFixer > 2 affected files
• …

[grummbeer]

To get it right. You mean this PR?

Sorry … reading helps, will do it this way.