Create a 'syntax linting' build step plugin

This first patch is an unsophisticated first iteration. As per the issue description, it implements a basic build step which runs php lint on all the files in the drupal root (not vendor) directory and only does so if the DCI_SyntaxCheck variable is included in the job definition. The remainder of the tasks described in this issue will require further iterations or a more complete patch.

In the case of the production DCI test runs, the syntax linting is actually unnecessary for our spot instance runs. The additional overhead of spinning up a spot instance and running a lint adds an extra step which increases the total time it takes to test a patch.

This feature should be something the user can toggle for local DCI runs to save time on local tests, but adds additional overhead in our production pricing model with spot requests.

The syntax linting is currently a key performance regression relative to the PIFT testbots ... it is required for any patch tests.

I was under the impression that syntax linting was on the pift/pifr bots to keep from running tests on invalid code, and therefore was a preventative step to conserve testbot resources. Since we'd be spinning up a bot regardless to run the tests, It's unclear what benefit this test would add, or why it is required for patch tests.

One imaginable scenario Is to have a module with a lot of files, and some of those files don't actually get loaded or tested as there is not test coverage for whatever functionality exists in that file. In that case, we should probably push for that test to be added to core, as seen by the effort here: #1191682: Add a test for E_PARSE compliance (PHP lint)

Currently, the only thing standing in the way of innovation is a very large sum of money being expended every month paying for the pift/pifr bots. The purpose of these child issues is not to scuttle any ideas or valuable features, its merely to assess whether or not we are truly blocked by a feature being an absolute must have feature, or whether a feature can be delayed while we work on higher priority issues.

If I understand correctly, the value of a syntax linting build step is to catch those easy to make mistakes in code for which there is no coverage, everything from the tests fail to load some code, to there simply are no tests. This is helpful as it helps explain some other anomalies we've noticed in the current testing infrastructure - namely that there are a *lot* of contrib projects that do not have any tests, yet have enabled "automated testing" for their project. So if I understand this correctly, under that scenario somebody could submit a patch to a contrib project and the current testbots would 1. see if the patch applies and 2. Lint afterwards, and fail that patch if need be. Additionally if a user commits broken code to a branch, when the branch tests run they would *also* get a notification that their branch test failed due to lint failure.

Given that defined use case, I wholeheartedly agree that this feature provides value, but still I think we should evaluate whether or not this is a feature that contrib developers are relying on, or if its something we can add back in later.

An additional consideration is that from drupal.org's perspective, we currenly only offer the ability of users to "enable simpletest jobs" for their projects - i.e there isnt an interface yet to allow maintainers to configure their projects for "'one-commit, multiple checks' model", so it would begin as "one-commit, all checks" instead. I dont see that particularly being a problem with syntax linting, but in the eventual implementation of codesniffers, linting, performance tests, visual regression tests, and any other jobtypes we might add, it will become something that needs architecting.

I just want to call attention to @webchick's comment in #2534132-47: Disable Legacy Testbots and use drupalCI as our testing infrastructure -

webchick: I think [this issue] probably belongs in the D8 blocker list. There's no reason we should ever be committing PHP syntax errors to D8.

Yep. I had a half typed comment. This is a regression for d6/7 contrib and maybe d8 contrib. core is so well tested that the likelyhood of ever getting a patch in that somehow passed tests yet had a syntax error is negligible. We need this before we shut off d6/7 testing, but can live without it for the month that we do not have d8 bots.

git diff HEAD --name-only|xargs -n 1 php -l will check only the files changed in the patch. As non-PHP files pass, this will do.

Adding the updated patch from the branch that is being merged to the dev branch. I have a few suggestions which I will put in follow up comments.

+++ b/src/DrupalCI/Plugin/Preprocess/definition/SyntaxCheck.php
@@ -0,0 +1,30 @@
+    $definition['setup']['syntaxcheck'] = $value ? "TRUE" : "FALSE";

This may not work with quotes, since these appear to be TRUE/FALSE unquoted in SimpletestJob.php and drupalci.yml, so I'm putting this back on needs review.

This has been merged into dev, and production. And is live.

so what were the changes here?
is php syntax linting only done on commit?

I have a feeling the d.o integration would be tricky, but there'd be some value to running this (and phpunit tests, but excluding simpletests) on all patches for all PHP versions on patches, then only doing the full simpletest run on one PHP version + nightly/on-commit.