🇬🇧United Kingdom @jonathan1055

I've just read through the MR. It looks interesting, and good to see a simple worked example.

Adding a notes that that this slack thread has some useful info which can be documented in this MR

Just checked specifying the full word --verbose and this also removed as intended

Here's an example of the cspell job passing green when it has failed to run anything due to supplying an invalid option. Locally I have tried to detect this error but not managed to. the $? is only set if there are spelling errors. I've yet to detect when the command fails to run at all, but there should be a simple way. We don't really want to go down the route of pipeling the output and using grep to detect "error:" in the output. But if there's a way to immediately detect when the command has not run we could use that.

I wanted to check if we needed to cater for case-insentivity in the regex matching. We don't ... because the options have to be in lower case. But in checking this I found that the CSpell job ends green when invalid options are specified, see this example. This gives a false sense that the job worked, when it did nothing, and it would be nice to trap this in the EXIT_CODE. But that is currently remaining blank in this scenario.

I know there is one school of thought about "not chasing core" as these things would not cause actual failures until Drupal 12. That point of view is fine for real modules, but I think we have to at least try to keep these downstream branches running green for 'next minor', as this gives us instant clarity from upstream that all is well. If we accepted amber results, then it always requires double-checking that we've not caused an extra problem that is hidden.

Removing -v and --verbose before the second run has the desired effect. There is no verbose output in the artifact file.

Hello,
Woud you be able to give me a credit on the contribution record please?

When the downstream job is forced to fail, using BEFORE_SCRIPT_ACTIONS: 'cspell-use-madeupword, cspell-use-flagged-word' we see the problem - the log says there are 62 misspelled words but this includes the verbose output line, as written to this artifact file

Updated issue summary with the fixes to be worked on.

Updated the issue summary.

I decided that it would be good to make the change suggested in #42 as this allows recipe projects to continue using symlinks. Yesterday I completed 📌 Ignore deprecations in search_api Active and 📌 Fix FunctionalJavascript test classes must specify the #[RunTestsInSeparateProcesses] attribute Active so that the downstream testing is clean & green and easier to see that everything is working.

Downstream d11-recipe composer creates symlinks
Downsream d11-recipe phpunit passes because the symlinks in /tests/ are replaced with real copies of the files.

The non-recipe branches all run as expected. I have adjusted the help pages accordingly.

There are a few debug lines to remove, but this is ready, please review, so we can get this merged and start supporting Contrib recipes.

Fixed.

After rebasing following 📌 Fix FunctionalJavascript test classes must specify the #[RunTestsInSeparateProcesses] attribute Active we only get deprecations in gtd1. This is because this test has protected static $modules = ['gitlab_templates_downstream'] to actually load this module (and hence its dependencies), where as the other two test groups do not install the module.

With two messages ignored we only see two remaining, so this is the right approach.

Merged for d10-theme and marking it fixed.

If this needs to be done in any other branches, say d11-recipe, then this issue can be re-opened. We could use a slightly different solution, by having a baseline file, not a phpstan.neon config file, thus giving test coverage for the inclusion of the file.

gtd2 and gtd3 end green. gtd1 now has four deprecations not five, and these will be fixed in 📌 Ignore deprecations in search_api Active

For the PHPStan problem, we could use:

phpstan (previous major):
  variables:
    # Drupal 10 does not have the phpunit attribute classes.
    _PHPSTAN_ALLOW_FAILURE: 1

This works as required, and lets just the Previous Major version fail amber, whilst keeping strict on the other variants. But the aim of the downstream pipelines is to always end green, so that we have full confidence at a glance that all is passing. Therefore I add a phpstan.neon config file to the project. This not only solves the problem, but also gives test coverage for when the project has its own file, as this has not been the case until now.

Thanks @ciprian.stavovei for the test. It looks good at first glance and I will do a detailed check too, locally. I have triggered the test-only changes job but it is not a good example, as it fails with unknown schema, because the updated .schema.yml is reverted. Could you add _TEST_ONLY_FILE_PATTERN: '*.yml' to the variables section so that the schema and settings are preserved, then the test should run and fail properly as required. See the doc page for Test-only changes for more info.

I have also left some comments in the MR.

Regarding (1) adding #[Group('gtd3')] to the javascript test definition allowed it to be discoverable again. So it seems like adding one attribute turns on the switch that forces the Group atttribute to be required.

With (2) I suspect it is just a poorly worded deprecation message.
Functional/FunctionalJavascript test classes must specify the #[Run ....
actually is referring to the test being run, which is a Functional test. The fact that it also includes /FunctionalJavascript makes it read like it's referring to the Javascript test. I can check this with the next commit.

For (3) we have the option of not running PHPStan Previous Major, or allowing it to fail.

Adding the attribute initially just the to javascript test produced an unexpected result - the javascript test gtd3 is no longer discoverable but the other two groups (which do not have the new #[RunTestsInSeparateProcesses] attribute are still discoverable. Maybe adding one type of attribute requires them all to be added? I will add #[Group('gtd3')] to test that hypothesis.

Secondly, the gtd1 and gtd2 tests still complain that the javascript test does not contain the attribute, when it does now have it. That sounds like a bug.

Thirdly, we get problems at PHPStan Prevoius Major because the attribute classes have not been backported to Drupal 10.

There are also some other new deprecations clouding the logs, and I have opened 📌 Ignore deprecations in search_api Active to address them.

jonathan1055 → created an issue.

I've rebased, and now that we have SCRIPT_VERBOSE from ✨ Add --verbose parameter to symlink_project.php, and a variable turn on verbose Active I decided to remove the --debug option and put all output into --verbose instead, as this can be controlled by the environment variable.

Ignoring the new options, and all the verbose otutput, the actual functional change in this MR is to remove the two extra specific deep_merge calls, and instead soecifiy the projects own array twice in the single call to deep_merge.

Before

// Merge the default and the project composer.json.
$json_rich = merge_deep($json_default, $json_project);

// The order of the 'repositories' entry values is important, so prioritize the
// module's values first, if defined.
$json_rich['repositories'] = merge_deep($json_project['repositories'] ?? [], $json_default['repositories'] ?? []);

// Likewise for 'installer-paths' prioritize the project's values first.
$json_rich['extra']['installer-paths'] = merge_deep($json_project['extra']['installer-paths'] ?? [], $json_default['extra']['installer-paths']);
...
function merge_deep(): array {
  return merge_deep_array(func_get_args());
}

After

// Merge the default and the project composer.json arrays. At the deepest level,
// values from the later array(s) will overwrite the same keyed value in the
// earlier array(s), so the project array needs to be last in the sequence.
// However, when new keys are found they are pushed onto the end of the result,
// so we also repeat the project array at the start, to ensure that in the
// output file the projects own values remain in the same order as the input and
// and are always placed ahead of the additional defaults. This does not affect
// the result, but makes comparison between input and output easier.
$json_rich = merge_deep($json_project, $json_default, $json_project);
...
function merge_deep(): array {
  // Use preserve_integer_keys = TRUE because the input list of arrays contains
  // the project's array twice.
  return merge_deep_array(func_get_args(), TRUE);
}

This is ready for review. The downstream pipelines run fine, and we can see the verbose output array. However, it would also be good add a customised installer-path to one of the downstream projects, to prove that this really works.

This is all test classes, not just FunctionJavascript

Not urgent, but its good to have the downstream branches all geen, so we can easily see when a problem occurs.

📌 End compatibility with Drupal 8 Active merged, so can now work on this.

Before - @group scheduler test log length 17,116 lines
21 x 21 deprecations
1 x 23 deprecations
1 x 24 deprecations
https://git.drupalcode.org/project/scheduler/-/jobs/6953047/viewer

Now log reduced by 2,600 lines to 14,468
21 x 17 deprecations
1 x 18 deprecations
1 x 19 deprecations
https://git.drupalcode.org/project/scheduler/-/jobs/6963786/viewer

There were 210 lines of code removed in this change.
Doing this unblocks 📌 Fix @Constraint annotation for plugin Active

The new release Scheduler 2.2.2 → was created on 19 October 2025. We can now drop Drupal 8 compatibility.

The next release will have to jump from 2.2.x to 2.3.0

jonathan1055 → created an issue.

I have released Scheduler 2.2.2 →

Thanks again Berdir for providing this significant performance improvment. If you have any notes or ideas about the tidy-up please add them to the follow-up issue 📌 Remove redundant code after caching improvements Active

jonathan1055 → created an issue.

Fixed

This is fixed by adding reportUnmatched: false to override the default just for this specific message.
https://git.drupalcode.org/project/scheduler/-/merge_requests/259/diffs?...

Adding the message to phpstan-baseline.neon fixes PHPStan next minor, but now at 'current' (which is running 11.2) we get:

------ ----------------------------------------------------------------------- 
  Line   scheduler.module                                                       
 ------ ----------------------------------------------------------------------- 
         Ignored error pattern #^Call to method getSystemData\(\) of deprecated class                                                       
         Drupal\\migrate_drupal\\Plugin\\migrate\\source\\DrupalSqlBase#        
         (method.deprecatedClass) in path                                       
         /builds/project/scheduler/web/modules/custom/scheduler/scheduler.module
         was not matched in reported errors.                                 
 ------ ----------------------------------------------------------------------- 
 [ERROR] Found 1 error    

because the deprecation is only from 11.3+

Expanded the scrope to cover 11.3
Added 📌 Fix Call to method getSystemData() of deprecated class Drupal\migrate_drupal Active

jonathan1055 → created an issue.

I have removed the change to src/Theme/SchedulerThemeNegotiator.php until we have test coverage for that. There are still the six occurences listed in #4 above but I wanted to get this large MR merged. Part 2 can follow later, to finish the job.

jonathan1055 → changed the visibility of the branch 3356800-test-only-changes to hidden.

The change looks reasonable. Is there any way to add test coverage for this?

The MR was very out of date, at 37 commits behind 2.x-dev but it is all up to date now.

It would be good to have some test coverage for this functionality. Maybe in Functional/SchedulerFieldsDisplayTest.php as that seems the most appropriate place?

Hi antonio nunez,
I am interested in helping you, and getting this MR committed. Do you have any info relating to my questoin two months ago about how to replicate the problem?

This is postponed until we drop Drupal 8 support.
📌 End compatibility with Drupal 8 Active

Thank you @berdir for this excellent enhancement. I will make a new release soon.

I have tested this with SCMI on the 3502119-pipeline-test-3.x branch. The first commit applied the patch from this MR but the phpunit job failed becuase the patch does not apply on the latest Scheduler release. So the next commit used Scheduler 2.x-dev and that worked. The phpunit jobs ran OK
https://git.drupalcode.org/issue/scheduler_content_moderation_integratio...

So I think this is ready to commit, I can't think of any further testing to do.

Just for interest I tried out idea 3 in our downstream recipe branch and it worked. The code I actually used was:

variables:
  _COMPOSER_SYMLINK: 1

phpunit:
  before_script:
    - rm -Rvf $DRUPAL_PROJECT_FOLDER/tests
    - cp -Rvp $CI_PROJECT_DIR/tests $DRUPAL_PROJECT_FOLDER

I added a couple of ls to show that just the test files were copied and the others remained as symlinks. Here's the full pipeline
https://git.drupalcode.org/issue/gitlab_templates_downstream-3503337/-/p...

I think we shoud consider doing this within Gitlab Templates, as this fixes it for all cases. Thoughts?

Thanks mxr576.

... symlinking mainly impacts DDEV Contrib currently, and only in context of running tests, the proposed fix here could be merged here

Yes I agree with you. We need to be able to support recipe testing now. It took longer, because the changes I made spawned more work which became necessary to separate out into their own issues ✨ Add --verbose parameter to symlink_project.php, and a variable turn on verbose Active and #3425971: Allow copying or symlinking when building the project → . Now that both of these are merged, I have rebased and removed that work from here, and now MR408 is ready for review.

There are some options available now for devs and maintainers, here are some things I have thought of:

1. Keep using symlinks

   If the recipe does not have any phpunit tests and you and you want to stick with using symlinks for ease of use, eg when running the linting jobs locally, set the top-level variable:

   variables:
     _COMPOSER_SYMLINK: 1
   

2. Alter the setting per variant

   The setting can also be changed per variant, so it may be that symlinks are used for 'current' and the phpunit job is skipped, so that the beneift of symlinks remains for the linting jobs. A separate variant can have the copies and run phpunit tests

   composer:
     variables:
       _COMPOSER_SYMLINK: 1
     SKIP_PHPUNIT: 1
   composer (previous minor):
     variables:
       _COMPOSER_SYMLINK: 0
   

3. Custom copying of the test files

   As it is only the files in the /test/ folder that need to be 'real' and not symlinked, it is possible to continue with symlinks for all jobs and then do a custom copy of the required files in the phpunit job

   composer:
     variables:
       _COMPOSER_SYMLINK: 0
   phpunit:
     before_script:
       cp -v $CI_PROJECT_DIR/tests/ DRUPAL_PROJECT_FOLDER/tests
   

I'm not saying that any of these are the best solution, nor that they will be suitable for all recipe projects, just putting the ideas down for reference. A proper solution from ✨ Add getRecipePath() helper method to RecipeTestTrait Active would be the best. But at least for now we have a way forward, with plenty of choices.

This is good, thanks for opening the issue.

Just a note: the function getRecipePath() is already defined in Recipe/GenericRecipeTestBase.php. This means that if the Contrib recipe test just extends GenericRecipeTestBase - like we do in the Gitlab Templates Downstream test module - recipe branch then there is no customization required. The path resolves correctly (providing the file is real not symlinked, of course)

I had to add

phpunit:
  # Temporarily need to allow the tests to fail, otherwise cannot merge the MR via UI.
  allow_failure: true

to be able to merge the MR via the UI. When ✨ Add recipes path handling Active is completed I will remove these lines.

Hey! I learned that typing https://www.drupal.org/comment/15470452 → does work, and it resolves to the issue and comment ->
https://www.drupal.org/project/gitlab_templates/issues/3397699#comment-1... ✨ Preserve original composer.json in the modules directory Fixed so I have fixed that in the summary.

Excellent. I'll work on ✨ Add recipes path handling Active now, which is where this change was originally developed.

Just wondering, as you have set this to fixed, the issue will become closed. But the MR here only addressed one small part of the problem, and gave an alternative approach, but it's not a full solution. There is lots of good discussion and ideas in the summary and comments 1-3. I guess we can still read these even when the issue is closed, as it is referenced from several other places.

In the issue summary the number 15470452 in [#15470452-13] is a comment ref number, so this is a typo, and I can't find the issue that it is refering to.

Ah, I read it as "class statement" but the warning actually says "class comment" so technically it is kind-of correct. But the mis-understanding was because it says "one newline" and there is one new line - at the end of the class comment. Maybe a better message would be to say "no blank lines after the class comment"

jonathan1055 → created an issue.

The printed output from gtd1 is

Unmodified getRecipePath() gives /builds/project/gitlab_templates_downstream/recipes/gitlab_templates_downstream

and gtd2 shows

_DIR__ = /builds/project/gitlab_templates_downstream/recipes/gitlab_templates_downstream/tests/src/Functional
realpath(dirname(__DIR__, 3))=/builds/project/gitlab_templates_downstream/recipes/gitlab_templates_downstream
calling this->applyRecipe(/builds/project/gitlab_templates_downstream/recipes/gitlab_templates_downstream)

I'm going to remove the echos and revert the custom phpunit.xml..

I changed ls -l to ls -la so that we see the hidden files. Otherwise the list is shorter than in the section above it (which is sort of redundant in the pipeline, but should be left in for when running as a standalone script)
Here's an example showing the .hidden files.

Doc page suggestion applied and thread closed. Ready for review.

Several users are wanting to opt-in to multiple projects at once. Maybe one request per maintainer would be easier to manage in the first instance? Also folks are not following the required request text (see issue summary). Maybe they could edit their request comments above.

jonathan1055 → created an issue.

jonathan1055 → changed the visibility of the branch 3503337-mr-testing-d7-composer to hidden.

jonathan1055 → changed the visibility of the branch 3503337-mr-testing-d10-plus to hidden.

I'd rather keep it variable driven unless we really need to change it.

Oh yes, I wasn't thinking about removing the variable, nor of doing any work in this issue, just noting the idea for future reference. We do have the possibility of running _COMPOSER_SYMLINK:1 for one variant and _COMPOSER_SYMLINK:0 for another, which gives great flexibility, say for a job that only works with copying. I think I will add that variation in the GTD branches somewhere.

I have removed the temporary code, and removed the conditional check before displaying the ls output.
Final test in d10-theme
Final test in d7-composer

I have also written a new paragraph in the Composer doc page. I don't think we have documented the symlink process anywhere else, so I did not know how much to write. Can you check this is OK and please make suggested changes if you want to.

Thanks for the feedback, addressed.

• d10-theme which has no setting for _COMPOSER_SYMLINK shows the new message
• d11-recipe also has no setting, and shows the required message

I did not add the message to main-d7 as the default will always be '1' given there are no recipes in D7. But I could add that confirmation if you think its necessary.

I did, however, add ls $DRUPAL_PROJECT_FOLDER to confirm if the files are copied or symlinked:

• d10-theme showing the project folder contains symlinks
• d7-basic likewise showing contents are symlinked
• d11-recipe shows that the files are copied not symlinked

This output is currently only shown when SCRIPT_VERBOSE is on, but as this is inside the collapsed section I think it might be helpful to show it always. There will likely be times when we want to see the files on other projects, when there is a question, and they won't have had verbose set, so will require a change and re-run.

On my question at the end of #13 about writing the variables to build.env, I will leave it as-is for now. An idea that was brewing is about whether we symlink always, but re-do a copy of the files only in the specific jobs that need them (eg. only in phpunit job, when the project is recipe). This maintains the dynamic sourcecode updating ability useful when using DDEV, but makes sure the pipeline jobs run as required. I think this idea was mentioned before, somewhere else.

There are some debug lines to remove, and the setting of variables in the downstream pipelines to remove, but this is ready for review and feedback.

I realised that if the default for _COMPOSER_SYMLINK is set to '1' then it would be impossible to detect if that had been set as an override in the projects .gitlab-ci.yml (eg to allow recipes to force symlink) or whether it was simply the default. Therefor the default is now blank, and we derive the required value 1 or 0 only if there is no value already in the variable.

d10-theme has no setting, so defaults to doing symlinks

d10-profile has custom _COMPOSER_SYMLINK: 1 and therefore also creates symlinks

d9-basic has custom _COMPOSER_SYMLINK: 0 and makes copies as required

d11-recipe has no custom _COMPOSER_SYMLINK but has _PROJECT_TYPE: 'recipe' and also makes copies as required

Regarding the deriving of the two variables _COMPOSER_SYMLINK and COPY_OPTION, would it be good to do this in the environment-variables section, and write them to build.env, so that they can be used (or at least examined) in any other job? This could be useful for future expansion.

Thanks, I have addressed the feedback in the PR. Also added a new test file good/good.yml and tested without the change (to prove coverage) then with the change.
https://github.com/pfrenssen/coder/pull/268/commits

[ignore the change to extentions at the top of the ruleset.yml. I later realised that the recommended way to run as per the README is to specify these in an extentions= parameter. If you don't do this, it defaults to just .php and .inc]

Ready for review.

@mxr576 Yes I agree. This section will be changed in #3548754-4: Check that the core constraints make sense if there are submodules → and I commented there that we should detect the recipe.yml file first. I was going to leave it to be done in that issue, as it involved a change to this entire process. But maybe it should be done better here anyway. I'll do that.

I also made the same change to main-d7 and the checked the D7 downstream branches - both run ok with copy instead of symlink
https://git.drupalcode.org/issue/gitlab_templates-3425971/-/pipelines/62...

Thanks for merging the verbose work in MR413. I've now rebased, and got the --copy work from the recipes issue into MR414 here and set the variable to test some downstream branches.

d9-basic has _COMPOSER_SYMLINK: 0 just to see how a 'module' project fares with copy instead of symlink. All seems to work fine. It might be worth running with the various "force validation and linting to fail" options, so we can check urls and links in the generated artifact files.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

d10-profile has _COMPOSER_SYMLINK: 1 to check it does the same as the default. No change, works as expected.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

d11-recipe has _COMPOSER_SYMLINK: 0 which is where we need it. Ultimately this variable will be set automatically when the project is a recipe, but for testing here we can set it directly. All the jobs pass, in particular the phpunit jobs.
https://git.drupalcode.org/issue/gitlab_templates_downstream-3549283/-/p...

So this is ready for review and feedback. The setting of the variable in the downstream pipelines will be removed from here, but we could set it in the .gitlab-ci.yml for some variants in a few non-recipe downstream projects, to increase test coverage.

Just to explain, for future reference, the 'current' variant runs the phpunit tests unmodified. That means that it depends on a 3rd-party conrtib recipe. Initially the tests were failing, because the path to 'recipes/' was incorrect. To prove that this was the only cause of the problem I added a conditional piece of processing in before_script that deletes the 3rd-party recipe from the recipes.yml definition when running the 'Max PHP' variant. Those tests then passed, even when the 'current' tests failed.

The recipe path problem has been resolved, but we may as will leave in the conditional modificaton to the recipes.yml file, so that we maintain broader test coverage for when the recipe under test both does and does not depend on another Contrib recipe.

Thanks, yes this is ready, essentially.

I added a temporary phpunit.xml so that I could set these five options to false during development. But when everything is done in ✨ Add --verbose parameter to symlink_project.php, and a variable turn on verbose Active and MR414 on #3425971: Explore the posibility of a different setup for the project, composer expansion and symlinking process → I will re-test this MR and remove the echo/print and remove the phpunit.xml file before merging. Just adding this here, so we remember. 🐛 beStrictAboutOutputDuringTests=true or failOnRisky=true causing test failures due to PHP8.4 Implicit Nullable deprecation Active would help in this situation, and would avoid having to add the temporary file.

Thanks for the feedback. Yes I have removed the .gitlab-ci.yml change.
I have also removed the temporary setting of this for the downstream pipelines. It can be set on when making a change to a script.
Ready for review.

Thanks @ciprian.stavovei I am pleased to hear it works well for you.

Tagging with "Needs Tests" because we need to have test coverage for this new feature.

Following the ideas by @dpi in #2 and subsequent suggestions, I have this working in MR408 ✨ Add recipes path handling Active . But it should really be a separate MR here, with it's own changelog entry etc.

It would be good to get ✨ Add --verbose parameter to symlink_project.php, and a variable turn on verbose Active done first, as I think it may be better to implement a --copy option instead of getting the environment variable directly. We will still have an environment variable, though.

jonathan1055 → made their first commit to this issue’s fork.

The two phpunit tests are running and passing, now that we have a --copy option for symlink_project.php.

I have pushed the change here to add the --copy option to symlink_project.php, and tested it downstream.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
For details of the tests see 📌 Add a Recipe branch Active and MR37

MR413 on ✨ Add --verbose parameter to symlink_project.php, and a variable turn on verbose Active should be merged first. Then I can re-do the work here.

Thanks for linking that issue. Good to get some more background on this. Yes you are right in your understanding that all the changes in MR408 are necessary for proper recipe support. At least, that is the best of my knowledge.

But I've also made a new discovery - in #32 above I said "It would be nice if the core getRecipePath() actually did the correct thing, but it doesn't" but I think now that this is not a fault of the core function but instead it is a problem in how we create the CI environment - specifically it is because the project files are symlinked into modules/custom/mymodule or in this case recipes/myrecipe. The getRecipePath function has

    // Assume this test in located in RECIPE_DIR/tests/src/Functional.
    return dirname((new \ReflectionObject($this))->getFileName(), 4);

which should do the correct thing for Contrib recipes, now that we are locating them in the proper folder. But if the file is a symlink then ->getFileName() returns the target file, not the test file being used. This is why we get /builds/whatever/mymodule when it should be /builds/whatever/mymodule/recipes/mymodule. I have tested this locally, and when the project files are copied the recipe tests work perfectly, without any cutsomisation, when they also depend on another 3rd-party contrib recipe.

There was an idea on #3425971-2: Explore the posibility of a different setup for the project, composer expansion and symlinking process → which has a simple change to make symlink_project.php optionally copy the files instead of symlink them. So I'm going to make that change here to demonstrate the result. I would like to do it on that issue, ultimately, but we need the recipe changes from here also. If this works, I can separate out the copy option changes and make a MR on that issue.

All downstream pipelines pass. This is ready for review. I have opened one thread in the MR, to ask a question.

jonathan1055 → created an issue.

It will be useful to see those environmwnt variable values anyway, but I think the reason the second pipeline ran was because there is still a value for $CI_MERGE_REQUEST_IID in the merge_train pipeline. So the rule clause immediately above the one you changed would still cause the pipeline to be run. It will be easy to adjust, once we decide what combination of values should make a pipeline run.

I do not think this is a valid solution because it couples the test to the execution environment.

Yes, OK fair enough. Each project can solve the problem however they wish, as that is within their control. For our downstream testing, it's a very specific module with custom branches, with the sole purpose of providing test coverage for changes in the Gitlab Templates. So we may decide that using the environment variable is acceptable. It really was the only way that I could develop these tests locally and have them running in the pipeline too.

Thanks for giving those other reference links. Both the _DIR_ and the \ReflectionObject($this))->getFileName() techniques work fine when the recipe does not depend on another 3rd-party contrib recipe. It would be nice if the core getRecipePath() actually did the correct thing, but it doesn't. I think the reason is that storing contrib recipes in /recipes has only evolved to be become a common thing, it is not a prescribed site standard as far as I have been able to find out.

Hi @mxr576
Thanks for coming here to test, yes I found your llm_support recipe was almost the only other one so far that was attempting phpunit tests, and it was a good source of help to me during my investigations written up in #17 above ✨ Add recipes path handling Active

Sorry you had a tortuous trip getting to test this MR. Here is the doc page but not everyone finds that straight away.

Regarding your test failure in #29 - I know exactly what is wrong, and also exactly how to solve it :-) Just this morning I have just been finalising our downstream branch for testing recipes. When a recipe does not depend on any others, or only depends on core recipes, the tests work as-is. But if the recipe depends on another contrib recipe, the value returned by getRecipePath() has to be the path the recipe being tested within the general /recipes top-level directory, so that the other 3rd-party recipes can be found.

In your test file you already know that you have to use a different $directory value but __DIR__ = /builds/project/{your_project}/tests/src/Functional, so realpath(dirname(__DIR__, 3)) will return /builds/project/{your_project} when it needs to be /builds/project/{your_project}/recipes/{your_project}. I found that rather than try to construct this full path from _DIR_ and other components (which can vary depending on whether the pipeline is run in an issue fork MR or the canonical project) I used the existing environment variable DRUPAL_PROJECT_FOLDER which is now set precisely to the required value for this. See this change to the d11-recipe branch test that is exactly what you could do.

[As a side note, I also found this was the best way to allow me to test locally, where my working repo for the recipe project is actually symlinked from elsewhere and my local sites have diferent named directories. I can pass the environment variable in when calling run-tests.sh or phpunit locally]

MR408 is now ready for review, given that the downstream d11-recipe branch in MR37 is now passing at "current" (when a 3rd-party recipe is a dependency) and "max PHP" (the simpler version when only core recipes are required)

The phpcs job now passes green again.
One file could be fixed easily by getting the empty lines correct.
The two tests that had an early 'return' because they fail - this is better done by setting them to skipped. We also now see this info in the phpunit log, making it clear.

Ready for review.

First commit to remove the three phpcs:ignorefile to demonstrate the warnings
https://git.drupalcode.org/issue/webform-3550939/-/jobs/6810972

jonathan1055 → created an issue.

Now that this is merged it has unblocked 📌 Fix PHPUnit tests that fail in Core 11.3-dev Active

Now that MR742 on 📌 LogicException: The hook update_dependencies does not support attributes Active has been merged, this work can be unpostponed. We now see the details of the three failing tests:

There were 3 failures:
1) Drupal\Tests\webform_scheduled_email\Functional\WebformScheduledEmailTest::testWebformScheduledEmail
Behat\Mink\Exception\ExpectationException: The string "Webform submission from: Test: Handler: Test scheduled email</em> sent to <em class="placeholder">simpletest@example.com</em> from <em class="placeholder">Drupal</em> [<em class="placeholder">simpletest@example.com</em>" was not found anywhere in the HTML response of the current page.

2) Drupal\Tests\webform\Functional\Element\WebformElementDateTimeTest::testDateTime
Behat\Mink\Exception\ExpectationException: The string "datetime_default: '2009-08-18T16:00:00+1000'" was not found anywhere in the HTML response of the current page.

3) Drupal\Tests\webform\Functional\WebformEntityTranslationTest::testSettingsTranslate
Behat\Mink\Exception\ExpectationException: The string "<span lang="en"><p>Submitted on [webform_submission:created]</p>
<p>Submitted by: [webform_submission:user]</p>
<p>Submitted values are:</p>
[webform_submission:values]
</span>" was not found anywhere in the HTML response of the current page.

Note that I have no knowledge of how webform works, and I am not actively fixing these tests, just preparing this issue so that they can be worked on by someone else.

Just making a note here that the processing and logic for reading the *.info.yml files and for setting PROJECT_NAME and PROJECT_TYPE can be re-factored here, following the changes made on ✨ Add recipes path handling Active . The sequence could be adjusted to look for a recipe.yml file first, and if found then there will not be any *.info.yml so that processing can be skipped. Currently the *.info.yml files are searched for first.

Yes that's a good idea. I am fairly confident that the changes here to cater for recipes are the correct approach, and @jcandan agrees (see #13). So I will tidy up this MR and get it ready for merging. Given that each downstream pipeline is manual there is no harm in having the phpunit jobs fail at the moment. I have done some testing locally and I know what the problems are, which will be worked on in 📌 Add a Recipe branch Active

There is just a question about using $DRUPAL_PROJECT_FOLDER as the input to symlink_project.php, and I have opened a couple of threads on the MR to discuss it.

Thanks. I have modified the documentation page with changes to the Project Name and Project Type paragraphs. Also added a SCRIPT_VERBOSE hidden variable as per your suggestion.

I have included the new d11-recipe downstream branch, and that flushed out a problem in eslint where we were using $_WEB in a path. I have now fixed that, and also removed the other usages of the combined string $CI_PROJECT_DIR/$_WEB_ROOT/$DRUPAL_PROJECTS_PATH and replaced with $DRUPAL_PROJECT_FOLDER/.. which is consistent for all projects.

The downstream Recipes phpunit tests do not pass, but that is expected, as they are not ready yet. I think we need to decide on 📌 Add a Recipe branch Active what flavours of phputnit testing we want to cover, then merge that MR, before completing this one.

96 commits behind, tried to rebase, but needed a merge. It was all clean, no conflicts.

Updated IS with notes on the related issues.

jonathan1055 → made their first commit to this issue’s fork.

I've not done a full review, but the first thing that struck me were the many changes from $args('the-string') to $input->getOption('the-string'). In fact there are 73 of these, and I suggest it would be more readable if the $args array was retained, and simply built by the necessary calls to $input->getOption() but doing each only once. This would also reduce the compexity of the MR, and make it possible to print the entire $args array when doing debug. But having looked now at more of the changes, maybe my idea is not so important and stick with what you have done. But the diff is massive and needs a lot of checking. I can try to use this in a Contrib pipeline to test, but I expect the Core tests are doing enough to verify it already.

Thank you. That's a better way to do it. Much cleaner to read. I will test in a real contrib pipeline, but as you said before this could also have a build test. I don't know where the tests for run-tests.sh are, but if you point me there I could look at expanding to add coverage for this. Or if you can do it easily and quickly then carry on and do so.

The 'phpunit next minor' correctly tries to run just the three tests, but we get no details due to the pre-existing problem
https://git.drupalcode.org/issue/webform-3550117/-/jobs/6764595

Therefore postponing this, as it requires 📌 LogicException: The hook update_dependencies does not support attributes Active

OK I'll open a new MR so I do not pollute your work, then

I have a static patch of the work done so far, so I can use that in other jobs, and for reference, so I'm fine with you using this MR, then we can see the further changes you do from this point. Core has enough redundant MRs already :-) But if you want to start again that's OK too.

Thanks for clarifying.

I've opened 📌 Fix PHPUnit tests that fail in Core 11.3-dev Active and removed those changes from here.

jonathan1055 → created an issue.

this is definitely wrong and essentially just a revert to how it was

Just checking, when you say "this is wrong" do you mean the current commited code is wrong? or that the fix suggested here is wrong? I think you mean the former, but just want to make sure.

Yes I could move the Gitlab CI changes to a separate MR in this issue or into separate issue. But the error in webform is only evident when testing against 11.3-dev, (aka 11.x or "next minor"). So the gitlab CI change to opt in to 'next minor' is needed to demonstrate the fix. The other CI change (moving _LENIENT_ALLOW_LIST to the top-level global variable) is not just convenient for this change, and is needed for the job to run at 11.x, but it also allows easier running of a pipeline via the UI. If you try a UI pipeline and do not specify _LENIENT_ALLOW_LIST = select2, styleguide then all the composer jobs fail, because the blank value from the form takes precdence over the job-level coded value in the .gitlab-ci.yml. I could make a change here to repeat the job-level chnage for 'next minor' but that is still not the best way, which is to set the value at the global level.

The three tests that I have marked with @group @group webform_fail_at_11_3 I will move to a separate issue, as that is unrelated.

Thanks. I've replied in the threads.

Here is a test on the Webform module, submitted via UI just trying to run with _PHPUNIT_TESTGROUPS = webform_access
https://git.drupalcode.org/issue/webform-3547627/-/pipelines/616970 - it fails as expected becuase of errors in the Feed module test files

Now the same UI pipeline, but using Gitlab Tempates MR410, which applies the patches from here and add --directory in the command
https://git.drupalcode.org/issue/webform-3547627/-/pipelines/616992 - phpunit runs just the small subset of tests and passes.

I have pushed a one-line fix for the empty CI_PARALLEL_NODE_INDEX and that has fixed all of the tests :-)

This is ready for review and feedback. I have also used this patch in contrib jobs that were failing due to 3rd-party Test Discovery errors, and now they pass. I'll add links to them here.

Thanks @mondrake for the link. I've followed that issue.

To allow testing in Gitlab pipelines here is a static patch file for core 11.2 as there have been some changes in 11.x (11.3) which clash when trying to apply using https://git.drupalcode.org/project/drupal/-/merge_requests/13363.diff

I have opened ✨ Allow --directory and @group to work together in run-tests.sh Active and will now use the .diff url from that MR to apply a patch here, and show that the fix does what we need.

Interesting with the PHPUnit Build failed job, the command is
sudo -u www-data -E -H php ./core/scripts/run-tests.sh --phpunit-configuration $PHPUNIT_CONFIGURATION_FILE_PATH --debug-discovery --color --keep-results --types "$TESTSUITE" --concurrency "$CONCURRENCY" --repeat "1" --sqlite "./sites/default/files/tests.sqlite" --dburl $SIMPLETEST_DB --url $SIMPLETEST_BASE_URL --verbose --non-html --all --ci-parallel-node-index $CI_PARALLEL_NODE_INDEX --ci-parallel-node-total $CI_PARALLEL_NODE_TOTAL

but the test failed due to an unexpected value for test_names

    [test_names] => Array
        (
            [0] => 1
        )

This would normally be either empty, or a test group or a class or module, but not "1". Then we also see

[ci-parallel-node-index] => --ci-parallel-node-total
[ci-parallel-node-total] => 1

so this implies to me that $CI_PARALLEL_NODE_INDEX is unintentionally empty, thus shuffling the value along so it reads the string "--ci-parallel-node-total" as the value. This means we get left with a "1" which is taken to be a test group (positional parameter) and that causes the $group array to be filters for the test "1", so we end up with no tests found.

I have pushed an initial commit to the MR. I'm utilising the existing --debug-discovery argument to give further debug output to show what it happening. Some of this debug could stay, other bits will be deleted.

I will use the diff url from this MR to download and apply the patch, to deminstrate it in contrib testing.