Tweak PHPStan config so paths are always correct and baseline is more usable

Regarding #3395419-40: Allow contrib modules to run PHPStan via GitLab CI → :
Because of how the baseline will have a different path to the file, (web/modules/custom/tfa/tfa.module vs the baseline only has tfa.module) we based on my testing already get a baseline that includes ALL entries not just new.

Autoload usage is apparently deprecated, https://phpstan.org/user-guide/autoloading, even though we are already using it implicitly we might want to start looking at other options rather than doubling down with an explicit usage.

The intend of the previous issue was not to replace the baseline, but rather provide an updated baseline in the artifacts. This way maintainers can choose to replace the baseline if needed. I am not a fan of replacing the baseline automatically, but maybe other people like this workflow better.

@simonbaese we're not replacing the baseline automatically - we're offering you a new baseline that you can use if you so choose - nothing will happen unless you choose to download and commit the new baseline to your project.

@cmlara good point about existing baselines - I;ve fixed that.

Also I've realised that we need the paths to be correct in the code quality report otherwise the links your code quality report will not work so this issue is fixing a major bug in the PHPStan integration as broken links are very frustrating.

Also re #4 setting a custom composer autoloader on the command line is not deprecated - see https://phpstan.org/user-guide/discovering-symbols#custom-autoloader - PHPStan needs to support this because you can change where composer puts your autoload files.

Sounds good.

Just tested this in contrib and there seems to be a problem with the autoload directive. See this job for example: https://git.drupalcode.org/project/language_country_negotiation/-/jobs/2...

@simonbaese oh good point about sed - I thought we were only doing the baseline but I see we're doing the code quality report too.

Thanks for testing - it seems the issue is the DrupalAutoloader class... I tested passing in an autoloader locally and it worked great, but obviously something is amiss.

So this is happening because we are mucking with the project's composer.json file (in expand_composer_json.php) and the symlinking the code to a modules/custom directory (in symlink_project.php). These actions prevent DrupalFinder from working as expected.

This is actually problematic because we have plans to use a module's composer.json so we no longer have duplicate information in the .info.yml and composer.json files - can't find a link to the issue but will try to provide later.

Is there any reason for this to stay at Major priority? At worst I see that maybe the JUnit stage is for some reason not re-writing every entry (is sed only matching once per line? Seems thats the case for my projects too I just never noticed) however honestly that is a flaw that exists extensively in the contrib template already, definitely worth fixing, however considering its broken in the rest of the template already I'm not sure this is major. The baseline and code quality report are correctly formatted.

setting a custom composer autoloader on the command line is not deprecated

Ah, I interpreted it to mean that using scan directorates and files was the preferred method now, however I can see how they might have meant automatically using vendor/autoload.php is deprecated with preference to specifying the auto-loader.

and the symlinking the code to a modules/custom

This isn't directly related, however I'll note that our symlinking causes other issues as well. I ended up having to make a duplicate check-out of the code in the module directory without any symlinks because it appeared necessary for PCOV code coverage to work correctly (might be a PCOV bug, but wasn't something I followed up on).

Not sure that would make Drupal-finder happy, however if it does, perhaps that is something we should consider globally. Bonus points, I realized recently we could also skip checking out the repo on each stage since the repo is already in the archive, that could take a little load off the GitLab instance (may not be a concern now that they have migrated fully to AWS.)

This isn't directly related, however I'll note that our symlinking causes other issues as well. I ended up having to make a duplicate check-out of the code in the module directory without any symlinks because it appeared necessary for PCOV code coverage to work correctly (might be a PCOV bug, but wasn't something I followed up on).

Not sure that would make Drupal-finder happy, however if it does, perhaps that is something we should consider globally. Bonus points, I realized recently we could also skip checking out the repo on each stage since the repo is already in the archive, that could take a little load off the GitLab instance (may not be a concern now that they have migrated fully to AWS.)

Yes checking out the code in the module directory would make DrupalFinder happy. I think we should leave this at major was I think that changing the code under test as we do here with composer.json is not a great practice and something that's going to lead to issues.

Correction to my comment in #4:
I had said the baseline includes all entries not just now.

As part of updating my usage of the templates I removed my custom phpstan stage and discovered (at least in my setup) that the baseline only includes new entries.

Not sure how my previous testing showed different.

It really wouldn't hurt us to zero out the baseline to make it guaranteed to be all entries.

#13 we do zero the baseline. My guess is that you're custom phpstan set-up is using a different baseline name. This expects the baseline to be called phpstan-baseline.neon

we do zero the baseline. My guess is that you're custom phpstan set-up is using a different baseline name

We would with this patch. Currently in mainline we do not.

My comment should be taken more to mean that we should keep the zeroing out that is present in this patch, even if someone comes along later with a test that implies its not needed (similar to my #4 comment).

I presume this change is still needed? So could @alexpott who created the MR edit it to change the target branch from '1.0.x' to 'main'. It needs a big rebase, which failed in the UI so may need local conflict resolution too.

jonathan1055 → changed the visibility of the branch 1.0.x to hidden.

jonathan1055 → changed the visibility of the branch 1.0.x to hidden.

Actually, this issue fork was created before the 'main' branch existed, so I am adding that now.

jonathan1055 → changed the visibility of the branch main to hidden.

Totally untested yet, but all changes taken from MR69 manually, into new MR189 with target 'main'. Summary of changes:

1. Run in project directory, $_WEB_ROOT/modules/custom/$CI_PROJECT_NAME
2. If the project has their own phpstan.neon we use it, otherwise get our default from /assets/
3. Add absolute $CI_PROJECT_DIR/ to get to the vendor path
4. Change path to run to .
5. Remove path to phpstan.neon config file
6. Add absolute $CI_PROJECT_DIR/ for path when writing out artifacts
7. Erase and set to empty the baseline phpstan-baseline.neon file
8. Remove the sed as this is no longer needed

Thanks for the changes so far. I triggered the pipelines: https://git.drupalcode.org/issue/gitlab_templates-3397162/-/pipelines/14...
I made a small comment in the MR, but happy to hold off until more testing/tweaks are done.

The keycdn phpstan job gives the error

PHP Warning:  include(/builds/project/keycdn/web/modules/custom/keycdn/vendor/autoload.php):
Failed to open stream: No such file or directory
in /builds/project/keycdn/vendor/mglaman/phpstan-drupal/src/Drupal/DrupalAutoloader.php on line 100

I was not involved in the previous MR so I don't know if this was the same then. But it looks like the path /builds/project/keycdn/web/modules/custom/keycdn/vendor/autoload.php needs to be changed.

It's weird to have the issue in keycdn but not in decoupled_pages. I made a comment in the MR that might help.

For info, the error does not happen on Scheduler either
https://git.drupalcode.org/project/scheduler/-/pipelines/149149

I will make your suggestion to wrap the file name in quotes.

That path in the error for keycdn does not look right. We don't normally have /vendor within builds/project/{project}/web/modules/custom/{module} so maybe keycdn is different? I have added the quotes and also some debug, if you'd like to trigger the downstream pipelines.

Yeah, I looked at keycdn and there is nothing weird. It has no configuration variables nor configuration file for phpstan.
Pipeline running here: https://git.drupalcode.org/project/keycdn/-/pipelines/149235

Linking issue to do a deeper review on the reports generated to make sure that the paths are correct. The solution here might help as reference to the other jobs.

fjgarlin → closed merge request !69

I have merged 'main' into this MR, it was very out of date. I did my best with the conflicts in the script of .phpstan-base. It was the addition of --level=$_PHPSTAN_LEVEL which caused the conflict but I think I've got it sorted. Now need to start testing again.

Tested again https://git.drupalcode.org/project/scheduler/-/jobs/3027245
The generated phpstan-baseline.neon file does still have a long path

message: "#^\\\\Drupal calls should be avoided in classes, use dependency injection instead$#"
count: 3
path: web/modules/custom/scheduler/src/Theme/SchedulerThemeNegotiator.php

It is relative to $CI_PROJECT_DIR which is the default directory for jobs to run in, even though we have changed directory cd $_WEB_ROOT/modules/custom/$CI_PROJECT_NAME at the start of the job. Was this working before? The existing MR deleted the sed line, which I thought was the whole purpose of this issue.

I want to get back to this. I am guessing we will work on each job separately, as in MR189 here for PHPStan, even though we have 🐛 Make all jobs report artifacts use source relative paths. Active

I guess whatever is done in one issue can help the other, unless each tool requires a different approach. Maybe you can work on this first, and then, once done, go to the other one, which is more generic, and see what's needed and if anything can be taken from here.

I have done a lot more investigation into this, as clearly something was going wrong that we do not understand, given that the generated baseline is worse now when it was correct. I also needed to work out precisely the effect of the sed command, so I also ran tests based on the current job in the repo but the sed command. There are three outputs to the PHPStan job:

1. Junit report

Original but without sed command
View junit report in UI from 'tests' tab, click 'phpstan'
https://git.drupalcode.org/project/scheduler/-/pipelines/349148/test_rep...
All three names have the incorrect long path

Existing with sed
https://git.drupalcode.org/project/scheduler/-/pipelines/348373/test_rep...
The first error has the correct short path, but the'view details' shows full path e.g. /builds/project/scheduler/web/modules/custom/scheduler/src/Theme/SchedulerThemeNegotiator.php (this is when count does not match the expected number)
The two actual errors have incorrect long path. None of the links are active.
The downloaded junit.xml - has two incorrect 'path' values and the wrong path in message when count does not match expected. This matches the UI, so is this the exact same file?

MR189
https://git.drupalcode.org/project/scheduler/-/pipelines/348381/test_rep...
All three names are correct, which is an improvement. The message in the first 'details' still has the same long path. This always seems to give the full absolute path (tested locally). Is there anything we can do about that? I can't see any options in PHPStan. Maybe we can use sed to fix this?
The downloaded junit.xml has same improvement as the UI version.

2. Code Quality report

Original without sed
https://git.drupalcode.org/project/scheduler/-/pipelines/349148/codequal...
All three paths are wrong

Existing with sed
https://git.drupalcode.org/project/scheduler/-/pipelines/348373/codequal...
The active links to repo are correct (thus are being fixed with the sed command). One text has long path when count does not match expected, which is the same behavior as the junit file.
The downloaded quality report matches the UI

MR189
https://git.drupalcode.org/project/scheduler/-/pipelines/348381/codequal...
The report is not better. In fact it now has the even longer incorrect path in the first message.
But the three active links are still correct, thus showing that we have removed the need for the sed command to fix these.

3. Baseline.neon file

Original with no sed
The downloaded file contained paths starting web/modules/custom/{module}

Existing with sed
The baseline.neon is correct. The paths are relative to the project root, so this was fixed using sed.

MR189
The baseline.neon is wrong again, it has the full long path.

Testing locally I discovered that the paths being written to the baseline.neon change depending on where the file is being created. This was a surprise. The path is the relative path from the baseline.neon location to the files being analyzed. Running the command in the projects folder at $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/$CI_PROJECT_NAME does not fix it when the file is being written to $CI_PROJECT_DIR. So I'm going to try creating the baseline file in same directory as the files being analyzed (which should produce the correct short path) then move the file back up to the top-level $CI_PROJECT_DIR where it needs to be along with the other artifacts. rmaybe the artifact path could be changed for that file. I will test that afterwards.

Attached is an example of the discovery in point 3 above, where the location of the baseline file affects the contents.

I have pushed the change to write the baseline file to the current location then move it back to top level, and this solves the problem. The paths are now correct relative to the project root, see attached download.
https://git.drupalcode.org/project/scheduler/-/jobs/3486507

To answer my question in #35 "Or maybe the artifact path could be changed for that file. I will test that afterwards." Doing this produces a nested path in the downloaded zip file, so it is better to keep all artifacts in the top-level $CI_PROJECT_DIR

The final corrections, to the message text in the junit and code-quality reports can be fixed using sed. The existing command did not have the /g modifier, so it was only fixing the first ocurrence on each line (the junit file is effectively all one line, hence only the first path was corrected).

I've pushed a change to fix the full absolute paths in all three files. In the tests I've done this was only needed to fix the message details where an error in the existing baseline is found but the count does not match the expected number. All paths to actual files are already correct now that we run the commands in the deeper project directory. I have included the baseline file in the sed in case there are other messages written to it which contain the full absolute path (it does no harm)

I am not sure yet whether the sed should use $CI_PROJECT_NAME or $MODULE_NAME as they are both equal to the module name in my tests.

Tested here - all OK
https://git.drupalcode.org/project/scheduler/-/pipelines/350503

Attached comparisons showing previous path problems fixed.

There is some debug in the output, but this is ready for review and feedback. Would be useful to run keycdn, to see if the problem from #25 is still there.

Thanks so much for all the detailed answers, tests and screenshots. I wanted to find a solid gap to go through it.

The code looks great and so do the tests you ran! Keycdn and decoupled_pages keep on complaining about the baseline file not existing: https://git.drupalcode.org/project/keycdn/-/jobs/3506158

We should create it early if it doesn't exist, as it is referenced by our phpstan config file.
Then, we should continue emptying it right before generating a new one.

Other than that and the debug lines, it is looking almost ready. Setting to NW based on the above. Great work!

Thanks so much for all the detailed answers, tests and screenshots. I wanted to find a solid gap to go through it.

Your welcome! Yes, it was a bit of a long one to write. I needed to get it really clear in my head what was going wrong, before I could attempt to fix it and then explain it. Anotated screen shots helped me too.

Thanks for the downstream tests. Yes the baseline.neon file used to be created empty at the start if it did not exist. That still needs to be done, but I guess it got dropped somewhere in the changes between MR69 and MR189 in #22

I have pushed a change, and tested with a generic MR on KeyCDN, and it passes OK.
https://git.drupalcode.org/project/keycdn/-/merge_requests/28/pipelines
There are no PHPStan warnings, so I may introduce some so we can check the reports and baseline.neon work

I've marked the debug with a MR comment, but will leave that in until the last minute, when all testing is done.

The code looks great. Downstream pipelines are happy now and the additional tests show the last remaining issue is now fixed.

Marking RTBC. I have not removed the debug lines yet, you can do it or I can do it as well if you are happy with me merging the MR.

I want to test again with KeyCDN using a baseline with a message that does not match. This will check that the sed works properly when running in a issue fork with name such as CI_PROJECT_NAME=keycdn-12345, as opposed to when I run in Scheduler and it is just CI_PROJECT_NAME=scheduler.

Finally got the incorrect count warnings to show. The paths are not replaced due to the mismatch of the directory path containing the fork name "keycdn-12345". This can be fixed, I expect, hence back to MW

See the 'tests' and 'code quality' tabs on
https://git.drupalcode.org/issue/keycdn-3437565/-/pipelines/354621/

I have downloaded the incorrect artifacts from keycdn, and tested the commands locally, and it all works correctly

CI_PROJECT_DIR=/builds/issue/keycdn-3437565
_WEB_ROOT=web
CI_PROJECT_NAME=keycdn-3437565
sed -i "s#$CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/$CI_PROJECT_NAME/##g" junit.xml  phpstan-quality-report.json

So I do not know why this fails to edit the files when run in the pipeline job. I will add some debug and run it again.

I have found the problem and fixed it. The phpstan-baseline.neon file in web/modules/custom/keycdn-3437565 is initially a symlink back to /builds/issue/keycdn-3437565 as shown in the log:

cd $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/$CI_PROJECT_NAME && pwd
/builds/issue/keycdn-3437565/web/modules/custom/keycdn-3437565
lrwxrwxrwx 1 root root   33 Dec  6 21:24 phpstan-baseline.neon -> ../../../../phpstan-baseline.neon

The mv command fails, and produces

$ mv -v phpstan-baseline.neon $CI_PROJECT_DIR
mv: 'phpstan-baseline.neon' and '/builds/issue/keycdn-3437565/phpstan-baseline.neon' are the same file

so the script stops there and did not even attempt the sed

I added some extra debug (see https://git.drupalcode.org/issue/keycdn-3437565/-/jobs/3621509) and this shows us exactly what is happening. It turns out that the symlink is not replaced by the newly written real phpstan-baseline.neon file. Instead, writing out to the symlinked file changes the contents of the original file. This was probably known and obvious to many, but I did not realise this.

The fix is simple. Instead of erasing the contents with echo '' > phpstan-baseline.neon we actually need to delete the file (i.e the symlink).

Tested here and it worked as expected. The junit.xml pop-up messages no longer have the full path, and the quality report also correctly shows the shortened path
https://git.drupalcode.org/issue/keycdn-3437565/-/pipelines/361943

I have attached two screenshots comparing before and after. This is ready for review, and I won't remove the extra debug in case any more testing is required.

Good discovery!! I don't think it was obvious to anyone!

I think that the MR and the tests look good. There is a suggestion to clean up debug code (and there are more debug code I think), so let's clean this up before merging. I'd set it to RTBC but setting to NW for the clean-up.

I have removed the debug and re-run the same test here, all OK.
https://git.drupalcode.org/project/scheduler/-/pipelines/364319

I just noticed that documentation page needed updating to show that the artifact now contains all warnings, so I made that change. There was no previous mention of the default file phpstan-baseline.neon so I added that too.

Ready for review.

The documentation update does not give the full picture. We only get a complete set of warnings in the artifact file when the single default phpstan-baseline.neon was being used. If the project has multiple baseline files, it is only the one called phpstan-baseline.neon which gets zero'd out. So in those scenarios it is not the case that the artifact contains all warnings.

If this issue (according to the summary) is all about getting a full set of warnings in an artifact, regardless of which baseline(s) are in the custom config, then we need to do more work.

Yeah. maybe we just need to tweak the language slightly about which ones get updated. It might not be a full set of warnings for some, but it will be for most using the common configuration.

To make this more useful (and also easier to work with and explain and document) I created a new variable _PHPSTAN_BASELINE_FILENAME which defaults to the current hard-coded value 'phpstan-baseline.neon'. I used a placeholder text in the default phpstan.neon config file, so that a project does not need to have their own config if all they want is a custom name. All other processing has not changed from #47

This baseline file (whether the default name or a custom name) is erased before doing the final run to generate the new baseline file. This means that the artifact will have all the warnings we want for the replacement of that file. If the project has two or more baseline files, those others will not be erased beforehand, so the ignored warnings in them will be adhered to, and won't be repeated in the new baseline (which is exactly what we want).

Tested with the default unchanged phpstan-baseline.neon name
https://git.drupalcode.org/project/scheduler/-/jobs/3714310

and tested using variable _PHPSTAN_BASELINE_FILENAME: 'phpstan-baseline-to-fix.neon'
https://git.drupalcode.org/project/scheduler/-/jobs/3714999
The download artifact is correctly named as required, and I've attached a comparison of the original baseline and artifact - identical except for the message count (that is the only thing changed in this test MR)

I've updated the documentation page too, so this is ready for review.

I've reviewed the commits after #47 and they make total sense. Having the variable would be useful for the cases where names will differ. Thanks for the additional testing. I'm setting this RTBC.

Thanks. I made a small chnage to the variable description, which overlapped with your post above. It was just to remove the bit about "you will need a custom phpstan.neon config" as that is no longer the case.
https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/189...

Unrelated pipeline failure due to 11.1.0 being released. When you have time, could you trigger the downstream pipelines please. This is quite a major change, and I would like to see more tests that just in my one module. @alexpott, @simonbase, @cmlara - do you want to do any testing of this MR?

I thought I triggered the downstream pipelines, but clearly didn't (I did it in some other issues), so here they are: https://git.drupalcode.org/issue/gitlab_templates-3397162/-/pipelines/37...

I'm happy to wait for other people to review here too.

As this change is just isolated to phpstan and also only affects the artifacts generated, I think it's safe to merge.
We can get wider testing if it's in contrib and we can always revert if needed, but I think the change is pretty solid (and so was the testing).

We can reopen if needed (and revert if needed) or create follow-ups if new things come up.

Yes that's a good decision. It's great to have this long-standing issue, opened 14 months ago, done and committed.

Thank you @jonathan1055 for all the work. I especially like the changes mentioned in #50.

@simonbaese you're welcome, it was a good improvement to work on.

I did notice that the downloaded artifact baseline has tabs instead of spaces. The Drupal standard would require each tab to be replaced by two spaces. Do we need to say anything about this in the help page? or is that just part of general knowledge that a maintainer should be aware of and know how to fix?