🇬🇧United Kingdom @jonathan1055

The above order is what we have mostly been following, but I thought it worth taking a look to see what other developers do, and/or if there is consensus

Tim Plavec on Medium suggests an order that is fairly close to what we are already are doing:
https://tinplavec.medium.com/gitlab-ci-cd-best-practices-i-recommend-aft...

extends
stage
needs
rules
tags    << we do not use tags 
image
environment << we do not use 
variables
script
allow_failure
artifacts
cache  << we do not use 

He does not have 'services' in his list. I have not found any other recommended orderings.

His idea of having artifacts at the end sounds quite good. They are logically produced by the script, at the end of the job.

It would also be quite easy to write a simple script to verify the order in our two files, but Gitlab Templates own .gitllab-ci.yml, and run this as part of our code checks. We won't be adding too many new items very often, but the script would help us to get the order right, and keep it so.

jonathan1055 → created an issue.

jonathan1055 → created an issue.

Wow, that was one of the quickest MR commits. I was about to upload this screenshot to demonstrate what I meant about the 'find all'. So here it is, just for the record. Thanks for the merge.

jonathan1055 → created an issue.

The D7 downstream pipeline yesterday failed for phpcs and phpunit, but the problem seems to be in the Composer jobs (but not anything changed in this MR). We get an odd error when trying the symlink

$ php symlink_project.php "$CI_PROJECT_NAME" "$DRUPAL_PROJECTS_PATH"
Retry later

See https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/49...
I note that we do not have a $COMPOSER_END_CODE check in D7 like we do for D10, so I think this needs to be added. It would highlight this error in the Composer job, and not give misleading info about phpcs or phpunit failures. But that is for another issue, as it will need separate testing.

Thanks. I have pushed changes for items 4 and 5 and this is ready for review and feedback.

Just to note (I'm sure you knew this already) that the syntax needs: [composer (next minor)] if perfectly valid without the quotes even when there are multiple words. So I did not add quotes, just to keep it cleaner, but if you don't like the look of it I can add them. ESlint/Prettier demands no spaces between [ and the first character, and if using quotes they have to be singles not doubles.

First three changes done.

The first one was actually already done (I had not updated my notes). So there are only five proposed changes.

These changes have the potential to conflict with existing MRs but apart from the documentation MR there are currently only 3 other open and active MRs (where active = worked on in the last month) so now is a good time to do this. Those other earlier MRs will already be out of date and probably have conflicts.

I will make the changes in sequence, and keep them in separate commits for easier reviewing.

jonathan1055 → created an issue.

Thanks. It's been a long time coming, both this issue and it's sibling 📌 Update to new driver when D11 "current" uses Nightwatch 3 Postponed
I'm pleased we found a good solution for the new INSTALLED_DRUPAL_VERSION variable.

I've removed the test values for the downstream jobs. No more tests, so this is still RTBC if you are ok with the Pages doc changes.

Thanks for running KeyCDN, the --verbose works as intended.

I've just updated the Pages doc page, probably after you marked this RTBC. Here's the updated one. I know that the changes to the documentation are more than just adding this extra variable. I have been making notes on each documentation page, and when a change is needed I've been bringing them into alignment with a standard style. I will put more details in the documentation issue summary.

I am guessing that we do not want to have these three _EXTRA variables set permanently for the downstream pipeline definitions, so I will remove them.

For reference, in addition to the Experience Builder project mentioned above, Project Browser has also implemented this rather nicely. Although it looks like the code has also been shared between them already. We could borrow/learn from both.

PHPStan --debug works
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/49...

Nightwatch --verbose works
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/49...

Could you trigger the KeyCDN pipeline please?

I tried the folllowing, to add the EXTRA variable just for one variant

'→ GTD D10 Plus':
  extends: .downstream-base
  trigger:
    strategy: depend
    project: project/gitlab_templates_downstream
    branch: d10-plus
  'phpstan (max PHP version)':
    variables:
      _PHPSTAN_EXTRA: --debug
  'nightwatch (max PHP version)':
    variables:
      _NIGHTWATCH_EXTRA: --verbose

But the pipeline produced the following error

jobs:→ gtd d10 plus config contains unknown keys: phpstan (max php version), nightwatch (max php version)

Oh well, it was a guess but I had a hunch it would not work. So it means setting the variable for all variants (unless you know of a better syntax?)

Remember that we can pass variables to the downstream pipelines

Yes absoluetly, I'd not forgotten that. But I had forgotten that KeyDCN has a pages job, so thank's for the tip.

Regarding the backticks, I was thinking we would just delete them if not serving any rendering process. But if you still want quotes of some sort around those words then let's leave them as backticks, as they are clean an unobtrusive, and they do imply a 'code' word even if not rendered. Double-quotes would have a different kind of meaning.

First commit pushed. We could use the downstream pipelines to test PHPStan and Nightwatch, but we do not have a Pages job yet - will be done in 📌 Add documentation .md files to test the pages job Active

I did notice that the variables.yml file has lots of words in the descriptions in backticks implying "code" formatting. These do not get rendered as anything special in the Run Pipeline UI form, see attached. Is there anywhere else that this file is displayed/rendered where those backticks are actually useful? Or maybe they signify something even without any different rendering.

All temporary code removed. This is ready now.

Good that you like the last changes, I was very happy when I discovered how to make it both better, clearer, and more efficient all at the same time :-)

Here's a Scheduler test overriding the 'current' job with the legacy values
'show environment variables' has all 3 variables as expected, and the old driver values are used.

I'll remove all the temporary test code now.

I have made a futher change, that is both more efficient and also gets us better information. I noticed that .show-context was getting the real \Drupal::VERSION via vendor/autoload.php and this snippet was being called from several jobs. So instead, I have replaced the composer show with that php -r autoload.php and store the variable that way. Then when *show-context is called we simply write the value out, instead or running the php command. The value of DRUPAL_CORE is only displayed when it differs from the actual installed version. We can see the benefit of this in this next minor job where we do now get 11.2 showing.

The swap was made when you were first adding the quotes around each character, and I did not notice that when I made the first suggestion to revert. Some editing programs make auto-corrections when typing one quote it inserts two, so it can be easy to make these types of unintended change :-)

Now the MR affects 319 files not 320 which confirms the revert. Still RTBC.

jonathan1055 → created an issue.

I don't know if this helps, but attached is a comparison of the logs of the different failure on re-running. This is one particular test, and the first run shows "element not found" but the second says "stale element reference".

Yes, this is still RTBC but the revert was not quite right, the quotes were swapped round, which could cause other unnecessary conflicts. I have made the suggestion in the MR to fix this.

I have made the change to create the new variable MINK_DRIVER_ARGS_WEBDRIVER_DEFAULT and I think this does simplify the readability and understandability of the change.

Tested in the downstream pipelines and all OK. Testing with more complex tests (eg Scheduler) is still producing variable and unpredictable results. I ran a job and it failed, then I re-ran it in the same pipeline with no other changes and the failures were different. These tests have passed with the new webdriver before, and I expect they will again. Could you trigger the other downstream pipelines please? I will also runa test with a customized override to make sure that is still catered for.

jonathan1055 → changed the visibility of the branch mr257-3475581-use-w3c-driver-in-current to hidden.

Regarding Drupal 7 I have answered my own question, and we do not need to do it. There are only two variants, both using the same D7 core version. Technically it would be possible to add other variants to test earlier D7 releases but that is pointless. So there is no need for the new variable.

This is ready for review. Could you trigger the other downstream pipelines? I've made reminders in the MR to remove the extra test lines.

Reading the comments in #13 and #18-23 it seems like this issue is not going to be progressed? if so, can we close it?

Thank you @berdir

I rebased, and triggered all of our new GTD downstream pipelines. All passed except "PHPUnit (max php version)" in d9-basic and d10-plus

This was "closed (duplicate)" in November 2023, so MR35 can be closed too. Thanks.

This is "closed (works as designed)" so MR48 can be closed.

This issue has been "closed (works as designed)" so @berdir would you be able to close MR54 that you opened, please? I'm tidying up the Merge Request queue and trying to close the ones that are done, so we can more easily see the active issues.

As this is "won't fix" could you close MR52 please?

Would you like me to get this mergeable again, and so we can continue work on it?

@taran2l Did you have any more to add regarding #20 and the not having access to the logs when CI_DEBUG_SERVICES is on? Is it only maintainers of the project that can read the log? We could add a line about this in our documentation page about this.

Also MR107 can be closed.

I'm trying to tidy up the MR queue so we don't waste time rebasing things that have been discarded.

There are two MRs here that were merged (259 and 293) and two that are still open but very out-of-date (258 and 261).

This issue was fixed on 25 November, so I presume there is nothing in those MRs that we still need? So can the non-merged ones be closed?

jonathan1055 → changed the visibility of the branch 3475974-1 to active.

jonathan1055 → changed the visibility of the branch 3475974-2 to active.

jonathan1055 → changed the visibility of the branch 3475974-pass-variables to active.

The duplicate MR238 can be closed now.

Setting to NR as this change could be committed, and just to let you know, I'm not planning on working on the actual phpstan fixes. They could be done here or in a follow-up issue.

Migmag MR13 now correctly runs PHPStan jobs.

There is still some investigation required as to why your original phpstan-baseline.neon caused the failure.

That's good. The phpstan jobs now run as expected and report actual warnings.

I have replicated your error by using your exact phpstan-baseline.neon file in our Gitlab Templates Downstream testing project. Here is the failing pipeline

My understanding is that the baseline file usually has just errors to ignore, not other config parameters. These other config parameters usually go in the project's phpstan.neon config file. This may only be a convention of practice, not a rule, but I tried changing the phpstan-baseline.neon file so that it did not contain the excludePaths and the job then passed perfectly! Here is the passing pipeline

I have opened a new MR on 📌 Fix testing on most recent Drupal core versions Active to test this in your project.

I'd like to open a new MR here to test (and potentially fix) the phpstan problem.

That third call to phpstan uses

--allow-empty-baseline

which should allow the baseline file to not exist. At least it did when we tested those changes back then.

In your committed 1.8.x repo you just have the default phpstan-baseline.neon file, so I was wondering where the custom-phpstan-baseline.neon name came from? Did you set this via pipeline UI variable?

Looking back through your pipeline history, PHPstan passed '3 months ago' here
https://git.drupalcode.org/project/migmag/-/jobs/3697585

Just trying to understand the background of the problem.

Hi huzooka,
Thanks for reporting this. You say since 21 December, so it is likely that 📌 Tweak PHPStan config so paths are always correct and baseline is more usable Active has something to do with this, as those changes where made default in version 1.6.11 released on 18 December. Just for verification, would you be able to run a test pipeline using version 1.6.10? You can do this via the pipeline UI form. You need to enter the value for _CURL_TEMPLATES_REF as 1.6.10. This variable is right at the end of the form.

Can you give a link to this pipeline, then I can check how the processing differs from the later run. Thanks.

Good, let's keep it all here. I have renamed the variable to INSTALLED_DRUPAL_VERSION as suggested in the MR.

I note that there are some temporary changes to undo in the downstream jobs definition.

Would it be a good idea to also create this new variable in the D7 pipeline? It won't be quite the same becauase Drupal is not installed until the phpunit jobs. There is no specific need for it yet in D7, this issue has no other changes for D7, so maybe not worth the effort.

Was it this commit?
https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/341...
This is only changing the echo of the statement, as it was missing the --xml argument that was already in the executed command.

we are also changing the path of the generated files in here, not just creating empty ones.

That was not the intention, and I may be wrong but don't think we are changing the paths. What specific bit make you think this?

As this change now creates a new environment variable DRUPAL_CORE_VERSION which is generic, is created in the composer job and can be used anywhere, do you think that this work should really be done in a separate issue, with a separate entry in the changelog.md? It would be done first, then return here when merged, just for the phpunit driver changes. I'm OK to leave as-is and do all in the same MR because it is not a very complicated change, (and it also reduces work for both of us) but just thought I would check with you.

Applied you suggestion and responded on the MR.

Downstream d10 current with concurrent=0. The log is clean with no warnings and we see that phpunit-ignore.xml is created

Downstream d10 max PHP with concurrent=0. The log is clean with no warnings and we see that junit.xml is created

This is ready for review.

Good work. Just for the record, I can see you have released both 1.8.1 and 1.9.0 today, with 1.9.0 as new default. Was this in case there are problems with the new images then people can revert to 1.8.1 but still get those earlier improvements?

Thank you. That's a great find :-)

@murz I am interested to know how you discovered the environment variable FORCE_COLOR=1

Thanks to all here and on Slack for working to solve this.

I have also tested this MR on Scheduler here, and everything works - including loading PHP7.4 for Drupal 8.

RTBC+1 and when merged hopefully we can make a new release soon.

As we have the last follow-up issue I think we can set this issue to fixed.

We can contiue to use MR8 as a general testing merge request, to try out things or use a specific Gitlab Templates MR. It does not matter that this issue will eventually become closed.

Thanks. I have triggered our downstream testing projects, and the Nightwatch current and next minor (running Nightwatch 3.9) are now in color.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
It also does not affect the older versions. Does this need to be added to any documentation page and/or echo the variable to the log? Not sure it is necessary but just asking.

Thanks, merged.

I see now it is being discussed in this slack thread about docker.

Given than our Drupal 7 template only uses run-tests.sh there is nothing to port to main-d7.yml. And the d10-profile and d10-theme branches do not run any phpunit tests. So this is all done now.

MR16 is ready for review
https://git.drupalcode.org/project/gitlab_templates_downstream/-/merge_r...

To test the change I triggered a pipeline from UI with incorrect mink webdriver values, causing the javascript tests to be skipped. The jobs fail. Note - Max PHP ends with a warning, because the tests are skipped. It is running concurrent=0 for every variant because the UI form variable takes precedence over the job-level variables
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515487/-/p...

Another from UI, this time with concurrent=1 and also bad mink driver args - all jobs end green because run-tests.sh does not cater for the 'fail-on-skipped' concept
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515487/-/p...

Maybe if run-tests.sh has a long future, which I presume it does, we could ask for that enhancement? It should be quite easy, just pass the argument through to phpunit. Or is there a way we can do that from this end? It's a feature which is quite important for control of pipeline success/failure.

There seems to be a lot of unpredictability in pipelines at the moment. Several composer jobs fail and are autmatically re-run and pass. But some never do.

Fetch step
camelcase@npm:6.3.0: The remote server failed to provide the requested resource
Response Code: 403 (Forbidden)
Request Method: GET
Request URL: https://registry.yarnpkg.com/camelcase/-/camelcase-6.3.0.tgz
section_end:1743524709:fetch_step
camelcase@npm:6.3.0: The remote server failed to provide the requested resource
touch $_WEB_ROOT/core/.env
touch: cannot touch 'web/core/.env': No such file or directory

Look at the number that failed here on the branch after I merged MR15
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/48...
But I do think it could have been caused by the commit, so I will continue, and assume all will be OK later.

When cherry-picking to d9-basic, do we need to have a variant running _PHPUNIT_CONCURRENT=1? That branch only runs two variants - "current" which is Drupal 10, and "previous major" which is 9.5. I think both of those should remain with _PHPUNIT_CONCURRENT=0 so that the MINK_DRIVER_ARGS_WEBDRIVER variable is tested in both of the phpunit jobs. We could opt in to 'max PHP' which would be defined to test Drupal 10 at the maximum php which I think is CORE_PREVIOUS_PHP_MAX: '8.4'. This could be a useful extra variant in the d9 branch. What do you think?

The intermittent failures could be caused by the latest Chrome release
https://github.com/teamcapybara/capybara/issues/2800

This is now ready for review. The downstream d10-plus pipeline uses a GTD branch which has _PHPUNIT_CONCURRENT: 1 for Max PHP, all the other variants have the default _PHPUNIT_CONCURRENT: 0 so both cases are covered. We get no warnings in the log, and browsing to the empty files didplays it properly. If the file was completely empty we would get a render error as in this earlier job

This all works as expected, but I was wondering if there is scope to simplify the directory structure, because files are being written to both
web/sites/default/files/simpletest and web/sites/simpletest/browser_output. It makes browsing the files more tedious, either online or after downloading, having two output folders. Maybe that is for another issue though? This one solves the problem of removing the warnings in the log.

If any of the new people recently commenting and working on this issue would like to contribute here, then issue summary needs to have the "Benefts" section filled in. Currently it only has the original text from in the template:

If we adopted this change, the Drupal Project would benefit by ...

Actually, we can set _PHPUNIT_CONCURRENT=1 for the d10-plus, just for the "max PHP version" variant. That runs the same Core version as "current" which gives us test coverage for MINK_DRIVER_ARGS_WEBDRIVER over there. It means we cover both methods of running phpunit tests within d10-plus which is highly desirable. I've pushed that change and here is the job running concurrent=1

When running with _PHPUNIT_CONCURRENT: 0 we do get the junit.xml file but no simpletest/phpunit-*.xml files -

junit.xml: found 1 matching artifact files and directories 
WARNING: /builds/project/gitlab_templates_downstream/web/sites/default/files/simpletest/phpunit-*.xml: no matching files. Ensure that the artifact path is relative to the working directory (/builds/project/gitlab_templates_downstream) 

as shown in this log

I think (but I could be wrong) that the definition of the artifacts is fixed and cannot be conditional on an environment variable. Therefore we need to create an empty junit.xml file to avoid the error when using run-tests.sh (concurrent=1) and an empty phpunit-*.xml to avoid the error when running phpunit binary (concurrent=0)

Fix typo in title

I've opened ✨ Nightwatch tests have lost their colors Active

jonathan1055 → created an issue.

I've removed the temporary bits. Here is the regular pipeline on the MR
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

Here is the pipeline from UI, where I set the two variables to cause javasecript tests to be skipped
In the log show-environment-variables we see

MINK_DRIVER_ARGS_WEBDRIVER=-added-in-UI-
MINK_DRIVER_ARGS_WEBDRIVER_LEGACY=-legecy-added-in-UI-

https://git.drupalcode.org/issue/gitlab_templates_downstream-3515487/-/p...
The jobs failed as required.

This is ready for review.

I've just realised that MINK_DRIVER_ARGS_WEBDRIVER has no effect when running concurrent with run-tests.sh. The second pipeline in #6 passed all tests, inlcuding the javascript ones. So we should definitely leave _PHPUNIT_CONCURRENT=0 for the d10-plus branch, otherwise we'd have no coverage for the w3c driver issue 📌 Update PHPUnit jobs to new driver when D11 "current" is W3C-compliant Needs work

I have added a paragraph abut skipped tests in the doc issue
https://git.drupalcode.org/issue/gitlab_templates-3473000/-/blob/3473000...

The change to only add the option when _PHPUNIT_CONCURRENT=0 is tested here and the skipped tests fail as required -
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

And when running with _PHPUNIT_CONCURRENT=1 the option is not added, and the skipped tests have no effect (as per the existing behavior)
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515487/-/p...

Do you have a preference for which out of d9-basic and d10-plus gets changed to use _PHPUNIT_CONCURRENT=1? There are more variants being tested in the d10 branch, and it includes Drupal 11, but concurrent=0 is the default setting. I'm not sure it makes a big difference which we pick?

Do you want an issue opened for the color support? I am not sure how to progress it, and I've got enough other open issues on the go at the moment. But if someone else, who knows more about Nightwatch than I do, would like to pick it up, that's fine with me.

Adding --fail-on-skipped into $_PHPUNIT_EXTRA causes all the jobs to fail as required.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
The above is for _PHPUNIT_CONCURRENT=0 which runs the native phpunit binary

When testing with _PHPUNIT_CONCURRENT=1 using run-tests.sh we get ERROR: Unknown argument '--fail-on-skipped'
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515487/-/j...
So that needs to be a condition.

Both d9-basic and d10-plus use the default concurrent=0, so I think it would be better test coverage if one of them used run-tests.sh

Here is the initial pipeline. I faked an error in the job by setting MINK_DRIVER_ARGS_WEBDRIVER: '--'
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

'next minor' at core 11.2 fails with Driver info: driver.version: unknown. All the other jobs just skip the javascriopt test and end green.

jonathan1055 → created an issue.

Ready for review. It will be interesting to see if the unreliability is observed in other projects.

The error with 'next minor' was really simple - I forgot to add export when setting the variable, so even though it looked fine in the script, and in the after_script, it was blank in the scope of phpunit execution. The other jobs just skipped the javascript test because the webdriver was not running, but in Drupal 11.2 it caused a proper failure Driver info: driver.version: unknown which is much better. I had not noticed that the tests were skipped in the other jobs, I just saw green and assumed ok. Maybe in our downstream jobs we can add the phpunit option to fail on skipped tests _PHPUNIT_EXTRA: --fail-on-skipped

This is ready for review, however I have tested this in Scheduler and there seems to be a bit of unreliability with the new driver. The 'current' job sometimes fails, then I re-run and it passes. Here are some examples
Pass with new values scheduler-3445052/-/jobs/4746560
Failed scheduler-3445052/-/jobs/4743730
Re-run same job, get different failures scheduler-3445052/-/jobs/4746954
Re-run again and passed scheduler-3445052/-/jobs/4747136

This happens with _PHPUNIT_CONCURRENT: 1 which runs run-tests.sh
Here is an example

Need to check if there is different behavior with _PHPUNIT_CONCURRENT: 0 which runs phpunit binary.

jonathan1055 → created an issue.

Thank you. Both are done.

Did you notice that the older versions of Nightwatch (2.4 and 1.7) have output in colour, but version 3.9 is plain white. I don't know if there was a default of color 'on' which is now 'off'. I have just checked nightwatch --help and there does not appear to be any option to control this. So I guess we just go with it.

jonathan1055 → changed the visibility of the branch mr325-3380694-run-phpcs-in-project-folder to hidden.

Done. I left in the echos but changed to say "Using default ..." and therefore the absence of these indicates that the values were preset in a custom job. d9-basic and d10-plus all pass. Here's an example

Here is a test with override values

Ready for review. Could you trigger the Decoupled Pages pipeline please?

That info is actually always shown later in the Nightwatch job - see https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/47... But I like your concise output at the top so I will change that.

Thanks. I have created 📌 Add documentation .md files to test the pages job Active and responded in MR14

jonathan1055 → created an issue.

I have made one suggestion in the MR to improve an ignore line, but still leaving this RTBC. Hopefully this can be merged soon, even though the sniff is not enabled in this MR it will be great to get the bulk of the warnings fixed. Then we can more easily see the difficult cases. Let's try to avoid too many more rebases as this touches 230 files :-)

The work previously done in MR257 removed the legacy override in the 'current' phpunit job, but adds it back only for 'previous major'. Which means that 'previous minor' 11.0 would use the new values. This is wrong, just like it was wrong for Nightwatch. The downstream failures show this - https://git.drupalcode.org/issue/gitlab_templates-3514999/-/pipelines/45...

I've pushed the changes to show the new approach, by testing the drupal core version. We have to get the actual version used, not just rely on the value of DRUPAL_CORE as this could contain composer-like constraints such as ^11.

This is working ok apart from 'next minor' 11.2, which needs some investigation.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/47...

Pushed a change so the message is given only when the value was blank and is being set. In the normal job we get

Nightwatch version is 3.9.0
DRUPAL_TEST_WEBDRIVER_HOSTNAME set to 'selenium'
DRUPAL_TEST_WEBDRIVER_PORT set to '4444'
DRUPAL_TEST_WEBDRIVER_W3C set to 'true'

But when overrides are used we get get the version line but not anything else - shown in this job

Here is the updated documentation page

I have removed the temporary code which skips the other downstream jobs, and this is ready for review.

I have also triggered the downstream pipelines for d10-plus and d9-basic, both passed and the log shows what we expect. In particular, in d9-basic, the "current" variant automatically uses the legacy values, because it is running Drupal 10 not 11. The "previous major" is running Drupal 9.5 and an even earlier Nightwatch version 1.7, and it uses the same legacy values.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

Here is a test of overriding the variables in Max PHP and here is the job showing that it worked. The legacy values were used, but the log message is confusing, so I'm going to improve that. The message is shown, then we do the checks for if the variable is empty, and may not make any changes. This needs to be better.

jonathan1055 → created an issue.

I have made the changes to implement option 3. Keeping BC by defaulting the actual variables to blank, and only setting if non-blank. The legacy variables remain, as they may be referred to directly in customisations.

The PHPUnit changes are better done in a separate issue, so I have removed them here. The work is similar, but testing separately is easier.

To make progress here, it needs to be in a Merge Request so that pipeline testing is run.

I've pushed some changes, the detection of the Nightwatch version is working, but now I see why the variables are empty in build.env - thi si s because they are only defined in .nightwatch-base so are empty at the time the composer job is running. We have several ways forward -

1. move the definition of the variables from .nightwatch-base into .composer-base, or
2. move the definition of the variables from .nightwatch-base into hidden-variables.yml
3. do the derivation of the variables in .nightwatch-base no .composer-base

I think option 3 is better, as then the work is only done if the Nightwatch job is actually being run. It will sometimes be wasted effort if doing it always in Composer job.

jonathan1055 → created an issue.