PHPCS error in contributed module caused by core recipe.README.txt

The logic to symlink is here https://git.drupalcode.org/project/gitlab_templates/-/blob/main/scripts/... and we are skipping a few things, so at the very least we can add those three files into that array.

I've pushed the changes and created MR315.

Test to show existing fault. Initially there was no failures, so I added _PHPCS_EXTRA: '-v' to show which files are being checked, and the recipe file was not in the list. I had to add txt to the projects phpcs.xml extensions list, as these are not checked by default. Then we get the failure
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/388748

Now with the MR315 - debug added to showing the number of files and folders at top level, to prove there are no extras linked except build.env
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/388812

This is ready for review and feedback. But I think we could make symlink_project.php a bit clevere, as we will be caught out by this again and again, no doubt. The debug output in list 1 shows precisely only the projects files, before any installations or extras. We could capture this list and save it at the start of the job, then pass it into the script and use it as a positive filter for the symlink, instead of trying to list everything that should be excluded.

I like that approach. Get a "snapshot" of files/folders before and then copy those over. We need to make sure that hidden files are included in the list too.

I have pushed further changes for the more future-proof idea in #5 and #6. Thanks for the hint about hidden files and folders.

Three questions

1. The .git folder was excluded in the original symlink loop, and that is obviously there right at the start, so I excluded it in this process. Is that the correct thing to do?
2. The other folder that was previously excluded and I don't know its background is .idea , so I have excluded this too. Is it something that could be in the projects own repo? or might it get added during the installation? If that is no longer relevent I can remove it from the additional exclusion list.
3. The build.env file does not exist right at the start, but does exist at the time of symlinking, so I added this manually into the list, but thinkig about this again this file only needs to exist in the top level and I don't think we need a symlink for it. So I may remove it - is that right?

1. Yes
2. .idea is the folder that PHPStorm editor creates. We can leave it or remove it, no big deal. Happy to keep excluding it.
3. Correct, no symlink needed for build.env

Feedback in the MR for 3.

The tests on #5 were made before the last round of changes, so we need to check again before and after, but things look good.

Downstream pipelines with max php running: https://git.drupalcode.org/issue/gitlab_templates-3497525/-/pipelines/38...

Yes, thanks for the hint about adding OPT_IN_TEST_MAX_PHP into the .downstream-base. That is probably a good permanent addition.

Spelling fails in https://git.drupalcode.org/project/decoupled_pages/-/jobs/3946152 due to the MR branch name in build.env being checked. That file should not be tested now that there is no symlink to it, so that needs to be explained.

Test in Scheduler, showing directory and files.
https://git.drupalcode.org/project/scheduler/-/jobs/3947040

I forgot that cspell is being run in the top-level /builds/project/{project}. That explains the failure, and maybe for another issue we can look at running it in $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/$CI_PROJECT_NAME

Debug removed. Ready for final review.

We should provide a fallback where, if the variable is not defined, then it should contain exactly the same list as before (scandir minus a few files/folders), as otherwise it can break integrations: https://git.drupalcode.org/search?group_id=2&repository_ref=main&scope=b...

We can create a follow-up in the "ddev" package once we merge this with the changes so the original file list variable is created before running the script.

Well spotted, I was unaware of that. Yes, will do.

I have made the change, and left debug in. Tested locally and works as required, but is there a way to test this properly?

Tested locally = tested locally with the ddev plugin? If that's the case, then great (tho we will still need to create the issue upstream so the templates and the package behave in the same way, at least to make them aware of that situation).

Just to be extra cautious, let's create an MR in any of these projects (reduced list): https://git.drupalcode.org/search?group_id=2&repository_ref=main&scope=b... and then test.
These are directly downloading an using the PHP script, and obviously not creating the variable beforehand.

No, sorry, I should have been clearer - 'tested locally' = just mocking up the environment, eg export PROJECT_FILES= then running php symlink_project.php and checking the resulting symlinks.

Thanks for the list. Do you have a preference for which project we test with? I have not worked on any these. Are some maintaiers more amenable for this type of testing? Only the first three on your reduced list actually execute the script, the other hits were in comments

• https://www.drupal.org/project/openid_connect_windows_aad →
• https://www.drupal.org/project/commerce_pos →
• https://www.drupal.org/project/commerce_cart_api →

I'd say https://www.drupal.org/project/openid_connect_windows_aad → seems to currently have a green pipeline and +5K installs. I'd just create an issue in their queue and prepend [ignore] in the issue title. Then link this one in the issue description to explain that it's nothing on their end but that we need it to ensure that we keep their integration running as expected and that should do it.

I'm happy to jump in that related issue if needed, but I'm sure it'll be ok.

I have opened 📌 [ignore] Testing gitlab_templates merge requests Active and am testing on a branch and running the pipeline manually (no need for an MR yet). All runs fine. It is only the composer variant which has those customisations, their other core variants run plain defaults and those are OK too.

Their composer script is a very cut-down version, so was missing calculate-gitlab-ref and echo "COMPOSER_END_CODE=0" >> build.env so I added them. I think I will offer to help on a few issues there, as the customisation is only for composer, but has caused their cspell to fail (becase they do not have the composer.json backup/restore that has been added)

I left a small suggestion in the MR.

I'm also thinking that we will need a "Change record" here, not for the normal integrations, but for those mentioned in #15, so they can know what to do to use the improved version (aka define the env variable before calling the script).

I have added output following your comment in the MR. Re-tested here openid_connect_windows_aad-3499440/-/pipelines/397847
Composer log shows this full info:

WARNING: The list of project files and folders has been derived here, because the environment variable PROJECT_FILES is empty.
Array
(
    [2] => .editorconfig
    [4] => .gitattributes
    [5] => .gitignore
    [6] => .gitlab-ci.yml
    [7] => README.md
    [9] => composer.json
    [10] => composer.lock
    [11] => config
    [13] => openid_connect_windows_aad.info.yml
    [14] => openid_connect_windows_aad.install
    [15] => openid_connect_windows_aad.module
    [16] => openid_connect_windows_aad.routing.yml
    [17] => recipes
    [18] => src
)
This may not be accurate and to avoid this warning, at the start of your customized composer script execute the following command:
export PROJECT_FILES=$(ls -A)
See https://www.drupal.org/project/gitlab_templates/issues/3497525 for more details

The other variants (which are not customized) show

debug: projectFiles=Array
(
    [1] => .gitignore
    [2] => .gitlab-ci.yml
    [3] => README.md
    [5] => composer.json
    [6] => config
    [7] => openid_connect_windows_aad.info.yml
    [8] => openid_connect_windows_aad.install
    [9] => openid_connect_windows_aad.module
    [10] => openid_connect_windows_aad.routing.yml
    [11] => src
)

from a debug line I have left in. Maybe it would be worth writing that list always (not just debug) in a collapsible section, as it can be helpful to see what was there.

I can draft a chnage record too.

Yes, I see the value on having that list collapse. Let's make that change too.

The warning is perfect, tho I'd say that once you create the change record, the link should point there instead of this issue.

Draft CR is ready for review and I've linked to that url in the warning.

For the collapsed section, the exitsing ones have this in the script:

- echo -e "\e[0Ksection_start:`date +%s`:generate_stylelint_artifact[collapsed=true]\r\e[0KGenerating stylelint artifact"

I am trying to write it from the php script, so first tried a plain print then exec('echo -e "...") and neither of them have produced the collapsible section. There should be a way to do it, I will research more.

Collapsed section is working now. I included the curl statement inside it too
https://git.drupalcode.org/project/scheduler/-/jobs/4044022#L1019

I also need to test this on Drupal 7

The code looks good to me.

Downstream pipelines: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/398524

This is a great improvement, thanks!! Publishing the change record.

I particularly like the collapsed section, it makes the log more readable and I'm going to open an issue to do more of those sections.

Just looked at the changelog and for the link to the CR, the url is copied from the previous, but not updated.

I've just fixed the link to the change record. Well spotted!

Automatically closed - issue fixed for 2 weeks with no activity.