Expose phpcs patches to comment stream

Ideally proposed patches should be auto-fixed. If a patch is posted by a user with coding standard problems then the testbot should post a new patch with a comment "There, fixed some coding standard bullshit for you".

That would of course trigger another testrun - not sure how problematic it is that the testbot can trigger itself. It should probably only autofix patches that have not been posted by itself.

should probably not be autotested, but should still have ad hoc testing buttons available

Split that part off into #2852640: Allow not-previously-tested files to be tested.

#2854700: DrupalCI: Display or link to checkstyle results on Drupal.org also got split off from this issue.

The patches are ready for the taking now.

For branch tests, we should grab a copy of the file locally, and display a simple link to it. We don’t want to increase our reliance on Jenkins as a front-end.

For issue tests, we should either post a new comment as system message with the patch, or edit it into the existing comment next to the tested patch. I’m leaning toward existing comment, so the files don’t get too free-range.

We should post the files from each tool (phpcs, eslint, csslint) separately.

We're working on fixing this during Acquia Build Week's hackathon :) Sister issue for DrupalCI: #2881456: Phpcs: automatically generate patch to fix coding standards violations.

This is what I have so far. Feedback is welcome.

+++ b/includes/pift_ci_job.inc
@@ -202,10 +202,20 @@ class PiftCiJob extends Entity {
+            $cs_patch_url = $this->ci_url . '/artifact/jenkins-drupal_patches-' . $this->job_id . '/artifacts/phpcs/codesniffer_fixes.patch';
+            $cs_interdiff = $this->ci_url . '/artifact/jenkins-drupal_patches-' . $this->job_id . '/artifacts/phpcs/codesniffer_autofix.patch';

These should be the other way around. "autofix" is the actual patch, "fixes" is the interdiff.

Also, they're both URLs, so let's make them both have a _url suffix.

Fixed. Thanks for the review!

Do we want these patches to be testable by DrupalCI? From comment #3:

should probably not be autotested, but should still have ad hoc testing buttons available.

This would be done by putting the files in field_issue_files along with user-submitted patches. #2852640: Allow not-previously-tested files to be tested is the issue to make those and other untested patches testable.

@drumm, for the MVP, we wanted the artifacts to just be linked and the issue marked NW with the early fail, so that contributors could find them on issues as soon as possible, since that will have immediate positive impact. Then we envisioned maybe posting and testing them as described a next step, since that has some more detailed requirements for repeated test runs, user expectation/design, etc. Do you think that would be doable? The sooner we help contributors find the artifacts directly on issue, the better, and then we can iterate based on actual contributor experiences. :)

For #12.

Just to reiterate, it'd be really valuable to have just the NW comment linking the artifacts at first, so we can start changing our process for core contribution to match that. (Trivial issue credits are causing a lot of angst in the community right now and that angst in turn sucks up committers' time.)

I think once we adapt community process around that MVP, then we can work on and deploy making the reroll a testable attachment, with more scenarios and more complete design, to further reduce the manual steps needed.

Oops, yes, I forgot to mark #12 NR again. I have no more comments on this patch.

The text was proposed by @xjm, implemented by @csg.

Pointing in the direction of #2881456: Phpcs: automatically generate patch to fix coding standards violations

Committed & deployed with a couple changes:

• Check if each file is present individually. Currently DrupalCI isn’t outputting both files in production. (That’s happening soon it is in dev.) This is implemented as a list of interesting filenames and descriptions, so it is relatively straightforward to change those to keep up with DrupalCI changes.
• Also check if the patch isn’t empty. Linking to empty files would be frustrating.
• Since this is a more-generic list of files, update the comment rendering to match.

Looks good to me; thanks @drumm! Now to finish things from the CI side...

This is great, thanks everyone. It worked nicely on a core coding standards error in #2068063-267: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button where a previous passing patch (which was about to be committed) had one cs fault.

Now I am looking forward to seeing this on contrib patches - currently we have no cs checking - #2870645: PHPCS is failing on contrib modules

Yay it works!

@jonathan1055, the marking issues NW solely for coding standards is not happening yet; that will be a part of deploying #2881456: Phpcs: automatically generate patch to fix coding standards violations once it is ready.

@drumm, one thing we noticed in testing, that could also have been part of the confusion above, is that the System Message comment does not link the CI result for the test fail, only a link to the comment which is disorienting and can be further confusing when there are multiple patches attached to a comment. Can we use this same mini-list to add a first bullet linking the actual test result? Posted #2883814: Link failing test result in System Message comment for failed patches for this.

the marking issues NW solely for coding standards is not happening yet

OK, but here is the comment, with link to the newly generated patch.

Maybe the NW was generated by one of the random test fails we have been getting, but we certainly did get the codesniffer patch, with a single fix for coding standards, and when this was included in the patch it passed again - so that part is working nicely.

@jonathan1055, yep, it was a random failure that caused it to be mark NW, and that happy coincidence surfaced the coding standards interdiff that's already available with the formatting that's just been deployed. :) That's why I proposed #2883814: Link failing test result in System Message comment for failed patches, so it would point people to the actual fails in the same comment as the coding standards info.

It's great to have part of it working already!

YAY! This is a small step, yet a big milestone :)