[CI] Introduce our own PHPStan ErrorFormatter to avoid multiple PHPStan executions

Tweaking the title slightly

Nit about whitespace.

Also, maybe there should be a separate MR/build with a PHPStan error to show that works as expected as well.

lgtm

This is great! Really happy to see it.

However, I don't think this is ready. Haven't debugged to figure out why, or closely reviewed the changes, but something isn't quite right here.

The failed PHPStan job itself has a job log showing only 2 failures (the same violation from 2 different contexts):

https://git.drupalcode.org/issue/drupal-3568641/-/jobs/8176394

Code quality tab on that MR is showing 3 new problems:

https://git.drupalcode.org/issue/drupal-3568641/-/pipelines/721677/codeq...

So the undefined $a is making it to the codequality_report, but not the actual phpstan job output.

Haven't looked at the generated updated baseline artifact, yet.

Okay, fair enough. Too bad it's not consistent, but I'll grant that failures in Traits is a more weird case.

Tried again with what should be a "simple" failure. Let's see what the pipeline says.

Hrm, more consistent, but still not there, yet. I'm nervous that some errors are reported in some of the places, but not all.

Now we're seeing the warning about $mondrake on both code quality and the failed phpstan job log, but we only see the warning about $dww in the quality report and not in the actual failed job (nor test failure summary):

https://git.drupalcode.org/issue/drupal-3568641/-/jobs/8178245
vs.
https://git.drupalcode.org/project/drupal/-/merge_requests/14460

Sorry, I'm an idiot. The $dww failure isn't a phpstan failure at all, but a phpcs report. No wonder!

Looked at the artifacts for the 1-failure case. All seems reasonable.

Pushed another commit with a 2nd failure, to ensure everything works well with multiple problems.

It might make sense to push another MR that's just HEAD plus the same PHPStan failure and compare the test results/output.

Okay, not good. With multiple failures, the fail MR isn't generating any phpstan output files. No time to actually debug it.

https://git.drupalcode.org/issue/drupal-3568641/-/jobs/8178913

Opened 14461 that just has the same multiple failures as 14460, but otherwise is the same as main. The test results and output between 14460 and 14461 look the same to me.

Yeah, code quality reports on both MRs look to be (truncated) the same.

I downloaded the raw logs from both failed PHPStan jobs, and they look effectively the same.

I am +1 for RTBC, but let's have @dww confirm if he can.

Thanks for the 3rd MR. I didn't do a 100% exhaustive review of every job / pipeline, but from what I checked, everything seems to be working great.

Totally unscientific, but it looks like the new plumbing isn't actually speeding up the phpstan jobs in any noticeable way. Seems the time cost of having to re-run composer for each job (why did we move to that?) and the variation in times that entails hides whatever performance improvements this change is making. But on the face of it, running phpstan only once and then a script to parse a single file into 3 files has got to be less work overall than running phpstan 3 times.

Made the MR we care about a little more self-documenting. Updated the summary a bit. The other 2 MRs are draft, so that'll be obvious.

Don't see any other improvements to make.

Thanks again!
-Derek

1st pass at saving credit. Sorry @mstrelan. 😂 But thanks for the title tweak. Maybe the committer will feel differently. 😉

Went a step further and made the formatter totally configurable. Perhaps this is worth uploading to packagist as a standalone PHPStan extension package...

I swapped the keys and values by injecting the container and retrieving the services instead of injecting them directly.

Re env vars it's possible to configure like this:

      outputs:
        gitlab: %env._ARTIFACTS_DIR%/phpstan-quality-report.json
        junit: %env._ARTIFACTS_DIR%/phpstan-junit.xml
        table: php://stdout

but I can't find a way of setting a fallback, so if the var is not set (e.g. when you are running locally) you get Missing parameter 'env._ARTIFACTS_DIR' - this happens even if you aren't using the multiplex formatter.

Figured out a way of optionally injecting env vars.

I don't think I have time to do this now but as a followup maybe we could make the metrics generator also be a custom formatter, and then multiplex that as well (if the formatter has access to the baseline, I guess)

I'm now wondering whether the file handling should be done with SplFileObject instead of a resource, but I don't think it's worth blocking on. LGTM.

Have to admit I didn't really understand the remaining nitpick on the MR - @mondrake would you be up for opening a follow-up for that?

This is a very small change that will knock 10-30s wall time off each MR run for core, so going ahead here.

Committed/pushed to 11.x and main, thanks!

Sorry, push didn't make it, but done again now.