🇪🇸Spain @fjgarlin

The solution looks good.

I think it's ready for final clean up and RTBC. NW for now until the clean up happens.

Merged. Thanks!

I have done the edits suggested in the issue description.
For point 4, I changed from "Mollom" to "Salesforce".

Please review to make sure I got all the changes right.

And yeah, according to https://docs.gitlab.com/ci/variables/#cicd-variable-precedence, pipelines variables (3) have precedence over "dotenv" reports (7).

Well, for problem 1, I'd say that we change the logic from

# REPO and REF need be non-blank to ensure that the curl will give an easily detectable return code.
_CURL_TEMPLATES_REPO=${_CURL_TEMPLATES_REPO:-repo-is-blank}
_CURL_TEMPLATES_REF=${_CURL_TEMPLATES_REF:-ref-is-blank}

to

if [[ $_CURL_TEMPLATES_REPO == "" || $_CURL_TEMPLATES_REF == "" ]]; then
   echo "Error message goes here..."
   exit 1;
fi

That's great, I think the output now is much clearer and we can spot the error easier than before.

RTBC but we need to revert the change in ".gitlab-ci.yml". I'll set it as NW but once that's done, it can go directly to RTBC.

Thanks for confirming.

1.9.6 tag pushed and made the default-ref.

Good call. Re opened.

Core reverted the changes, so we should get tests running again as before: https://www.drupal.org/project/drupal/issues/3497431#comment-16127407 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active

We need to re-evaluate if this is now fixed (for now). No code required, just checking that pipelines running 11.2.x+ work again as before.

I commented on the core issue to see if there is the possibility of reverting, at least to a point where the script works with the documented usage.

Note that this has broken testing for 11.2.x+ pipelines for all contrib.

Related issue in "gitlab_templates": 📌 MissingGroupException: Missing group metadata with run-tests.sh in 11.2.0-dev Active

Modules are either getting MissingGroupException or ERROR: No valid tests were specified.

Key projects like Project Browser are affected 📌 Update for changes to System requirements Active , but again, this is all contrib testing NEXT_MINOR via run-tests.sh

SA nodes can now be sent to the new site to create the corresponding Contribution Credit.

MR: https://git.drupalcode.org/project/drupalorg/-/merge_requests/350/diffs
Once deployed:
- drush drupalorg-contribution-records-sync --node-type=sa --raw-import=2
- drush queue-run drupalorg_issue_events (This might not be needed as it is the same queue as with the previous step)

Merged.

fjgarlin → created an issue.

Merged.

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

It looks good to me. Good additions. RTBC.

Yeah, I would have thought that drupal/core-dev would have forced the right version of phpunit/phpunit based on the core version.

---

Extract from a Project Browser thread talking about this: https://drupal.slack.com/archives/C01UHB4QG12/p1748443110991539

@mondrake
so given the cmdline dump above, I think the problem is that you are not passing a directory of tests but a combination types+module

in this case PHPUnit will look at the directories to scan in the corresponding testsuite in phpunit.xml, and in PHPUnit 10 they must be explicitly defined for the module directory

two options: 
1) bump PHPUnit to 11: that will allow using the globstar patterns to find test files also in all the modules/ subdirectories, and files should be found without further ado
2) keep PHPUnit 10 but add a module phpunit.xml or edit the existing one adding explicitly the module directories to scan for tests

From that:

adding explicitly the module directories to scan for tests

Maybe we could use prepare-phpunit-xml.php to do something.

We also have expand_composer_json.php where we could add some logic to force a version of PHPUnit.

---

We will continue investigating here, but yeah, I would agree that the real issue seems to be in drupal/core-dev and not here since we are calling the script with the documented options.

No need to spread everywhere then. I think the improvements we've made / are making will be good enough, and at the very least, clearer than before.

[edit] I didn't initially think of the "needs" and "dependencies" involved.

We will try to look for a service that can capture queue requests, whether on rabbitmq or the drupal database, and make sure that we don't lose them if the servers are unreachable.

Leaving this MR (as I had written most of the code) for posterity.

fjgarlin → created an issue.

Cool. Using the alpine image is a good choice here.

Maybe we can avoid having these duplicate .pre jobs, as in this case, if "Pipeline set-up failed", we know that any other subsequent job is going to fail.

I think we can add to the other .pre jobs (in D7 and D10+) the condition: - if: $_CONFIG_DOCKERHUB_ROOT != null, so they only run if the variables are set up. We could even make that rule reusable and add it to the .composer-base job as well.

Do we want to copy the whole job to D7? Happy to do it here or as a follow-up.

I think it looks much cleaner. Since this is a (mostly) internal job, it's all good to go.
RTBC.

Thanks!

Agree on ⚠️.
I gave a few suggestions for the name in the MR but happy to go with any.

It might be a PHPUnit 10 vs 11+ thing.

I see this commit has a lot of logic for it https://git.drupalcode.org/project/drupal/-/commit/64212ca3018ce85b76e54... and that the error is coming from the file that was created in that commit core/lib/Drupal/Core/Test/PhpUnitTestDiscovery.php

I found this #2443839: Drupal does not install when auto-creating the MySQL database with special characters → . I'm not sure if that underscore in the name could be causing issues. It should not, but who knows.

I suggest that we try to connect to mysql (directly or via drush sql:cli) and try to run the show grants
and the show databases commands, to see if we can find more information about these errors.

New MR to use: https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/362...

Oh, that's the same issue we had in #14/#15.

Let me revert the commit and we can continue the investigation then until we are 100% sure of it.

I keep thinking that it might be related to how the image is created, but I'm not sure of this (mostly where to look).

It's an easy change so we can recreate the MR as/when needed.

Thanks for reporting this @xmacinfo!

I've removed the link to the search form and fixed the reference to the existing ID of the containing div.
This is now merged to the Bluecheese 2.x branch and it will go to new.drupal.org in the next deployment that we do.

Merged.

Merged. Thanks!

Are you sure that's the case?

We are defining the needs key for the validate jobs, as seen here for example: https://git.drupalcode.org/project/gitlab_templates/-/blob/main/includes...

Could you provide a pipeline that shows the issue? I'll mark the issue as closed as the code seems to be ok.

It gives me confidence knowing that the GTD projects reported the failure and they no longer do it (it was such a great idea to have GTD!!), so yeah, I'm feeling confident too.

I think this is totally internal, and if somebody has overridden this value, that's still ok because that's actually the change that we did in here. Nobody should be using the "mysql" database for a Drupal installation, so we are actually fixing a (long standing) bug in my opinion.

Yup, makes sense to have MR17 as wildcard for these things.

RTBC.

That's great! Marking RTBC.

The changes here look good. NW just to revert the GTD project/branch to use, otherwise it'd be RTBC.

Running the rest of the pipelines here: https://git.drupalcode.org/issue/gitlab_templates-3463044/-/pipelines/50...

smustgrave → credited fjgarlin → .

I can still see and reproduce the issue following the steps mentioned in the issue description. The only difference is that instead of blue, I get all white.

Note that this block is going to dissapear completely with the move to GitLab issues. It will be just a simple link to the issue queue.

If we don't worry about the PHPunit required in the Max PHP variant, this is RTBC.
If we want to fix that, then this is NW.

Setting to NW, but if we don't care about that, it can be changed to RTBC.

Merged. Thanks!

I think this is good to go. The descriptions look good and all the comments in the script too.
RTBC.

Merged! I will deploy the changes to api.drupal.org this week.

Thanks!

There is still some clean up for the now-unused beta versions logic. I made suggestions in the MR.

I added a test that proves that all three options work.

We'll need to see how this looks in bluecheese, but I think it's good to go. RTBC.

I'm happy for CORE_NEXT_MINOR to be 11.2.x-dev.

The "beta" comment was made long ago, probably before we had variants, so we don't need to wait for the beta phase to start testing it in CI. As part of this change, I'd also change the description of the variable, which probably brought all the confusion.

I left feedback on both MRs, mostly about the GitLab CI configuration.

I reviewed the logic on our end and it really seems ok, so I am going to close this issue. If it were to happen again, I'll implement a workaround that is a one-line change, but only if it's needed.

Merged and change record published.

Great. I think this ready.

I suggest that the last comment in the MR is made into a follow-up issue.
Once I merge this, I will also publish the change record.

Thanks for all the extra comments, the options for verbosity and the additional changes. I left a really small suggestion in the MR.

I didn't understand the commands to simulate, but the example above is perfect to understand it. That's a great way to test for non-existing tags yet.

Needs work based on that really small suggestion (purely cosmetic), but mostly ready in my opinion.

Thanks for the additions. I left some small feedback in the MR to cater for different syntax of the flagWords option.

Things are looking really good. I think all we need now is a quick test.

Some handy test helpers can be useful for this case (see usage in other tests):

// Inside the setUp method
$this->branchInfo = $this->setUpBranchUi(NULL, TRUE, ['supported' => FALSE]);

You might want to add the default value in "setUpBranchUi" for "supported" as well, and just override when desired.
It's probably a good idea to set up three branches, 1 supported and preferred, 1 supported and 1 not-supported.

Then, inside the test, it might be as easy as:
1. Visiting the page of the non-supported branch and checking for the error message.
2. Visiting the page of the supported but non-preferred branch and checking for the warning message.
3. Visiting the page of the supported and preferred branch and checking that there is no error message.

The feedback in the MR was addressed. Tests are happy (HEAD fails in the same ones, so no regressions).

I left some dev notes in the MR to explain the reasoning behind some code.

We are just normalizing the keys when we store something in the keyValue store and when we retrieve it we apply the normalization to the given ID. We don't need to normalize keys anywhere else.

This is ready for review.

Not sure we need to do that far. Everyone with the defaults will get it.

Someone with a cspell configuration file might actually want full control of the job and might not want modifications on top of it.

Something we could do is to detect this situation and if the flagwords that we recommend are not present then show a big warning message in the log. They might not see it at first but when they get a cspell error in the future they will see it.

If you are happy with the changes made to the CR, then we can merge this (probably tomorrow).

Thanks for drafting it. I added some more text to indicate workarounds or actions to do if people want to fix the possible issues. I also mentioned that the overall status of the pipeline should remain unchanged.

This 📌 Add 'supported' flag to branches to use to direct users and bots Active should go first.

> Should we have a reassuring message when reading current doc? That may be useful, maybe superfluous?
Happy either way. For simplicity, I would add it, but I'm happy as well if nothing is shown (no-news-is-good-news type of thing)

> Should we have the logic for displaying messages in twig or in the formatted?
If we do it in twig, let's make a re-usable template that receives the branch. It could also be done via preprocess functions or in the places where we prepare the variables for the templates. As the logic is not complex, I don't see a problem with it being in twig.

> Is this kind of approach to do a per page type level direction is good or is there a good way to abstract this? (There will be lots of copies of this message otherwise, maybe it should be part of the navigation method/logic, which is added on all pages?).
Good point indeed. Maybe we can make it a block plugin. See the existing ones, as we have a method that would help $this->utilities->getProjectAndBranchFromRoute(). This would avoid duplication in templates.

> Is this markup what we want to use to wrap this message? Would this show fine on api.drupal.org (it looks quite bad on Olivero, but it should pop on api.drupal.org :D).
We can have a follow-up styling issue for bluecheese. But we can also use the "info", "warning", "error" messages styling. Don't worry too much about how it looks in Olivero as long as it's right.

The comment that I make reference to:
--
We could defo run things based on a variable (name to be determined) as that's easy enough to trigger or not full jobs (via rules) and to check in the very first part of the script for each job.

Example (pseudocode):

variables:
  NEW_VARIABLE: 1

.run-composer-steps: &run-composer-steps
- do the composer things

composer:
  rules:
    - if NEW_VARIABLE == 0
  script
    - *run-composer-steps

cspell (for example):
  # Drop the "needs" part
  script:
    - if NEW_VARIABLE == 1 OR no-asset-from-composer-found
        *run-composer-steps
      endif
   - rest of the job script

That way we could experiment and turn on/off as needed.

fjgarlin → created an issue.

We can also add a test for this. I know some of them are failing in HEAD but most are passing so we can create a new one that proves that it works as expected.

Left feedback in the MR. We should display the "Supported" status in the front-end via the templates that we set in this module.

Happy to show the "Supported" or "Unsupported", whatever makes more sense.

Updated the IS as we updated some of the token and headers names on both D7 and D10.

D10 setup: https://git.drupalcode.org/project/api/-/tree/2.x/demo?ref_type=heads

https://www.drupal.org/jsonapi/node/project_module?filter[field_project_... →

That initially had field_core_semver_maximum: 11000000, that's why it wouldn't show on Drupal 11.1.3.

I re-triggered the semver range calculation and we now have:

field_core_semver_maximum: 12999999,
field_core_semver_minimum: 8000000

So the project should appear now.

The reason for the 12999999 is is because release 2.0.5 of that project has core_version_requirement: ">=8" and we have D12 as max value here.

I think that the issue can be closed as there are no changes required in Project Browser.

Cool. Feedback applied.

Closed MR796 (thanks!!) in favor of MR802.

MR with approach suggested in this thread: https://drupal.slack.com/archives/C01UHB4QG12/p1747231711012519

aha! found it!

https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/src/Proj...

Two ways to go around it.
- We change that part, by adding sha1 around it.
- Or we add a “safe” ID https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/src/Plug...

I think option 1 is good as it will apply to all plugins.

Actually, we were already using md5 by calling getQueryCacheKey. I changed it to "sha1".

However, given the above output in the issue summary, this is not related to storing the "query:...", it's got to be something else.

I created an MR with the fix: https://git.drupalcode.org/project/project_browser/-/merge_requests/802/...

The MR does not solve the issue. The problem is that "keyValue" has a limit for the "name" column

The issue is in this line, in PB code: https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/src/Enab...

I suggest that we "md5" (or whatever technique is best) the "$cache_key".
Something like this:
$cache_key = md5($this->getQueryCacheKey($query));

https://www.drupal.org/jsonapi/index/project_modules?filter%5Bmachine_na... →

...
"title": "Google Cloud Text-to-Speech Augmentor",
...
"field_composer_namespace": "drupal/augmentor_google_cloud_text_to_speech-augmentor_google_cloud_text_to_speech",
...
"field_project_machine_name": "augmentor_google_cloud_text_to_speech",
...

The "field_composer_namespace" value is 82 characters long.
The data seems correct, so it must be something happening in the code.

No worries at all. I know it was a small sample size, but as it requires coordination and possible changes with the runner it can wait.

In that case. Let's close this for now and focus on the other issues.

Maybe we can try enabling this feature in the future in the gitlab runner, but for now, we're trying to keep changes to a minimum, and this is not a real issue that we are facing right now.

Thanks for the MR and the investigation.

Great. I'll do that then, so we can also rebase the other open MRs.

I think the diference should be noticed in the jobs using the artifacts.

But... I'm confused. It seems that the flag needs to be enabled on the runner (according to the documentation and this issue), as the default value seems to be "false".

But we haven't made any change to the actual running configuration in +1 year (other than the regular updates).

So my confusion actually comes from the core issue, where it says that it "just works": https://www.drupal.org/project/drupal/issues/3521894#comment-16090831 📌 Stop trying to cache node_modules in gitlab jobs Active . Maybe it's somehow enabled via the GitLab admin UI (I don't have access to this).

I want to say that given that this MR is so simple we should probably go ahead and merge it anyway. There is no negative effects and if (or when) the flag is enabled, we should notice some gains.

RTBC.

psf_ → credited fjgarlin → .

Yeah, once it's in beta phase, we should update to this CORE_NEXT_MINOR: 11.2.x-dev. Maybe we can do something in the script to check when 11.2.0-beta* is available.

Testing any pipeline that uses the legacy driver should be good enough, or any D7 pipeline.
I think the change should be pretty safe because it actually happened a few years ago https://github.com/SeleniumHQ/selenium/pull/11409 upstream.

I think that, other than the @todo, everything looks good. RTBC (pending the removal of the todo).

Oh cool, this is a nice improvement on GitLab side. Yeah, let's review URLs and backticks all over.

fjgarlin → created an issue.

nicxvan → credited fjgarlin → .

We should have a sort of before/after to see if it makes any difference.

fjgarlin → created an issue.

Added link to the project shown in the screenshot.

Maybe we can add the suggestions (eg: whitelist->allowlist). https://cspell.org/docs/api/cspell-types/interfaces/Settings#flagwords
That way it will not recommend derivatives of the words.

This was merged and a new tag was made, which is as well the new default-ref for all contrib.

Not in this case:
- (Contrib) https://git.drupalcode.org/project/gitlab_templates/-/blob/main/assets/....
- (Internal) https://git.drupalcode.org/project/gitlab_templates/-/blob/main/assets/i...

We'll need to add them to the internal one too.

The current redirection is something like this:
- "Create release link": https://git.drupalcode.org/project/config_notify/-/releases/new?tag_name...
- 301 to https://www.drupal.org/project/config_notify/releases/new → , which 404s

We could create a menu path like project/%machine_name/releases/new, and from that, fetch the project with that machine_name and redirect to https://www.drupal.org/node/add/project-release/NID →

The logic might be a bit trickier since machine_name does not need to be the same as the repository name.

---

Another option, since releases are fully managed via drupal.org, might be to turn off "Releases" for all projects (https://gitlab.com/gitlab-org/gitlab/-/merge_requests/96373)

NW for the chromedriver args. The rest looks good.