🇪🇸Spain @fjgarlin

Merged! Thanks so much!

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

Maybe - !reference [ .with-database ]

@chrisfromredfin - we've changed the chrome driver to use for "next major" to match the one that Drupal core is following.

The "nightwatch (next major)" initially reported no longer happens, and there is an error that might be a legit one.
However, "phpunit (next major)" started failing.

See the history of changes made in here https://www.drupal.org/project/gitlab_templates/issues/3462681#comment-1... →

I wonder if going ahead with the new driver is a good idea and we try to address the issue of failing tests from Project Browser as a follow up. I know that we are at an edge case due to svelte/ajax, but it's hard to debug where the fault is at.

Right now, Drupal core latest "almost" forces the new chrome driver, but with that, we have fails that we don't with the legacy driver.

I've tried pinning the version to a specific one, setting the window-size, and both things together. None of them make the "phpunit (next major)" any happier.

I wonder if this is a "project_browser" specific issue due to AJAX loading elements. We don't get the issue with the legacy driver, but Drupal core is shifting to the new one anyway, so I guess it's better that they face this issue sooner rather than later.

Not sure what to do about the code in this issue. All the other projects we tried in seem to work well.

Trying pinning the version as per 🐛 Nightwatch and Functional JavaScript fails since selenium/standalone-chrome:128 Fixed

Pinning version as per 🐛 Nightwatch and Functional JavaScript fails since selenium/standalone-chrome:128 Fixed

oh, that's totally right! let me change it as well for PHPUnit first and retry and then we see about the variable (great suggestion btw).

No luck. Reverting the change in the issue.

Maybe adding some options to the driver can help: https://stackoverflow.com/questions/78436451/i-am-facing-javascriptexcep...

The nightwatch error has changed as you point out in the PB issue, but the "phpunit (next major)" error happens with this MR but not on the 2.0.x branch: https://git.drupalcode.org/project/project_browser/-/pipelines/274761

I just triggered a brand-new pipeline against 2.0.x: https://git.drupalcode.org/project/project_browser/-/pipelines/280185

I was referring/comparing it to core mostly on the volume of code being analyzed.

Core has thousands of files and contrib modules tend to have a few dozens or hundreds of files.

So in terms of performance or gains, the contrib jobs are minimal. But I’m happy if we want to try things here, by all means.

I think that in this project, more than any other one, we are “feeding” from core as much as core has been “feeding” from us.

It might actually be a great addition to core pipelines if we figure this one out.

Reopening. And updating the issue description.

Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3462681/-/pipelines/28...

Some of the Project Browser tests were failing, I just retriggered a run in case it was a one-off.

The code looks good to me.

Updated the title and the description.

I also see that the above project is using the very old 1.0.x version of the templates. Please try to update to the recommended version as per https://project.pages.drupalcode.org/gitlab_templates/info/setup/#using-...

No concerns so far, so closing this. We can reopen if it becomes an issue.

#3463894: Update templates so 11.0 is the default/current branch → will shift versions and D9 won't be available by default.

The snippet has been refactored a few times, and made more generic here #3463894: Update templates so 11.0 is the default/current branch → .
It also shows a diff of what's changed.

We are in the same situation now with Drupal 12 and also have the "Upgrade status" job, where it provides fixes for the module.

The only thing that we are not doing is to trigger an error, because in situations like this (Drupal 12 doesn't exist yet), it wouldn't make sense.
Note that we also have the option to have an empty value for "next major", which will not run the variant.

All in all, I think that the changes made in several issues made the current approach good enough. I'll close this issue unless we feel very strongly about throwing the error.

I'm grooming issues in the queue. I think that if we want extra checks to be made in check-versions, these can go as follow-ups.

I think the commit-code-check.sh script is now only use for local checks and/or pre-commit hooks ( https://www.drupal.org/docs/develop/automated-testing/run-core-developme... → ), but the Drupal core GitLab CI pipelines do run the checks on all the code.

So I think we will leave things as they are and close the issue.

I only made a minor request in the MR, to change the name of a variable. They called it like that in core, but we need way more logic for the different variants and I think the suggested name change will be consistent with the rest of the naming we've been using all over (aka "legacy").

The rest looks good and the pipelines seem happy. I also think that the addition in the documentation is good and it's easy enough. I wonder if we should also add it to the PHPUnit page as those jobs could also be affected and would require the same workaround.

Back to "needs work" based on the two above things, but things are looking good and solid.

Thanks so much for the additional changes.
Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3462681/-/pipelines/27...

I'm reviewing the code now.

I haven't tested this yet. But my idea for the code is in the MR.

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

https://www.drupal.org/project/drupalorg/issues/3382690#comment-15441835 📌 Limit number of "Categories" on the Drupal.org Project page Fixed

We can see that there are still a number of modules that have more than 3 categories saved. We cannot make a script where we just truncate them because the script won't have context about the module.

In this case, @chrisfromredfin, the best way to proceed is to contact the maintainers and to ask them to review the categories and save the node, even if they see only 3.

The number of allowed choices was limited to 3, but that's only on the UI. If modules have more categories, these will be kept.

That query was in the DB replica. We also ran it in the live server and the result is the same. 5 results.
I will revert the update flag for now.

So the error might be in "drupalorg" D7. There are 5 values in the database, but only 3 displayed in the edit form.
I'm moving the issue to that queue instead.

I'm not sure I'd know how to begin. We'd need to mock remote data, set an integration and settings in it (like disallow registrations) and then test the workflow. I don't see any test doing something similar yet.

For context, I found this bug integrating the new D10 version of www.drupal.org with the new keycloak system. We want registrations to always go via D7, therefore not allowing registrations on D10.

When working with existing users, everything works well, but when the user does not exist yet in D10, then we get the above watchdog messages and behaviour.

If I get some boiler plate code for the above from any of you that know the module better I could finish it off.

Umm... that didn't fix the issue, which made me look into the data.

MariaDB [drupal]> select entity_id, taxonomy_vocabulary_3_tid from field_data_taxonomy_vocabulary_3 where entity_id=1316346;
+-----------+---------------------------+
| entity_id | taxonomy_vocabulary_3_tid |
+-----------+---------------------------+
|   1316346 |                        57 |
|   1316346 |                        58 |
|   1316346 |                     20224 |
|   1316346 |                        59 |
|   1316346 |                        67 |
+-----------+---------------------------+
5 rows in set (0.002 sec)

That's 5 results, so the migration is actually working. I need to dig a little deeper on this.

The changes are ready for review.

fjgarlin → created an issue.

I updated the migration to run with the --update flag.
Marking RTBC.

We should see, within one hour, only 3 categories instead of 5 in here: https://www.drupal.org/jsonapi/index/project_modules?filter%5Bmachine_na... →

Thanks for reporting this Chris. The ongoing migrations do not use the --update flag as Drupal should recognize a change and then update the record, but it's clearly not doing it, so I'm going to change it to always use the --update flag.

Why not just adding the *.patch to the $ignore_files array? It'd be a one-line change and we introduced that variable for this purpose?

Re #25 - the challenge is that we can’t have if/else in services.

—

Re #26 - we are keeping the existing name for the new driver and we should definitely document the process for the new driver, so if modules have issues we have the workaround ready. The new driver is supposed to work as well in most cases, specially phpunit.

The BC change comes from core, we’re just reacting to it. Ideally, in a few versions time we might even not need the “legacy” tweaks anymore (or only for D10).

As per @alexpott via slack

I think we could have instructions for how to change it back because the legacy mode is still supported on 11.x - it will get deprecated at some point

So there are some cases where even 11.x can support the legacy driver (I guess that's what we saw in PHPUnit jobs).

In any case, let's add some documentation for this, probably in the https://project.pages.drupalcode.org/gitlab_templates/jobs/nightwatch/ or/and PHPUnit page.

Gotcha. Makes sense.

I ran the downstream pipelines and it seems good: https://git.drupalcode.org/issue/gitlab_templates-3462681/-/pipelines/27...

I am happy with the code and the results so far, so I'll mark it RTBC but it'd be great to have extra eyes from @alexpott or @longwave on the code.

Once merged, we'll have a conflict in the D11 issue MR, but that's fine and expected as we'll need to tweak what gets "legacy" and what doesn't.

Fully agree with #3.

Yeah, I'm happy to keep this open and even experiment with the possible changes and workarounds.

The question is still the same as the other issue really, whether to carry over customizations from "phpunit" to the other variants or not, as that's the way that a lot of people expect to behave.

We know that the same could easily be done with ".phpunit-base", but this would add a level of complexity and knowledge of the templates that maintainers might not have or need. And if they have the internal knowledge, then they should also be able to design easy workarounds.

From a code-design perspective, I'd love to do this (that's the reason why I opened the other issue indeed), but yeah, from a "nearly 2000 integrations and adding a disruptive change" point of view, I have my reservations.

Projects with the word phpunit in their .gitlab-ci.yml file: https://git.drupalcode.org/search?group_id=2&repository_ref=main&scope=b...

The code looks much cleaner this way. Thanks for the refactoring. I asked a small question in the MR.
Once that question is replied or addressed and the @todo in the code is restored back to what it should be, I think it might be ready.

Setting to needs work based on that.

Just resaving node as it shows without alias. 

Found it: #3425373: Extend from base jobs to keep personalisations at job level →

I can’t remember the issue where we talked about this, but we left it like this on purpose because it is very likely that maintainers would want any customizations made to “phpunit” to be in the other variants too.

Think of groupings (like scheduler) or concurrent jobs (like api) or any other customization that you’d do to “phpunit”. If we change the design then those customizations would need to be done to the other variants too.

The changes proposed here would be easy, the workarounds would also be easy, but the level of disruption in contrib could be pretty high, which is why we didn’t change it in the end I think.

I’ll try to find the issue where we discussed it.

I made a quick change that might be good enough. I have not tested it though, so it'd be great if you can test it and report back.

I've reviewed the code and it looks good. I love the source plugin if/else variations!!
I've merged the MR and I'm testing everything locally.

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

From what I see in the module, the easiest way here would be to add a link to the tab where the PR can be generated as part of the notification. I wouldn't go as far as creating anything, as that's the purpose of that other module, but we can definitely offer a link to the PR page.

Great suggestion!

No worries. I said something wrong in #32 and it's good to be questioned on these cases. I made a commit to the docs to fix some formatting and hopefully clarify a bit more what is what.

I changed the approach slightly based on some of the feedback.
Downstream pipelines: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/275511

I'm not sure about the why for that 10.3.4 vs others. Let's run another test and see if we can find out why.

100% on the above comment. Thanks for clarifying @cmlara.

I’m on the phone and with the kids and my mind is not in the right place for neither the issue nor the kids.

“main” is not a tag, so yeah, it can have commits not present in “1.x-latest”.

We need to improve the documentation page so it’s clearer.

No worries.

As long as we don’t have a 2.x, then 1.x-latest and main will be the same thing.

We just added it to the list of moving tags because it was easy, but it’s effectively the same as “main” as we don’t have short term plans for a 2.x release (breaking changes).

The documentation page needs to correct some spacing https://project.pages.drupalcode.org/gitlab_templates/info/templates-ver...

drush migrate:import drupalorg_migrate_project_module --update
Seems to have done the trick.

I can see only 3 categories here https://www.drupal.org/jsonapi/index/project_modules?filter%5Bmachine_na... → and also the update description.

I will take this one as a one-off, but if we have the problem again, we'll need to do a deeper investigation on why the records on D10 weren't updated. If that were the case, please reopen this issue or create a new one.

The other blocker is ready for review.

I will try next running the migration with the --update flag.

Query and results in database:

select * from field_data_taxonomy_vocabulary_3 where entity_id=980666;
+-------------+----------------+---------+-----------+-------------+----------+-------+---------------------------+
| entity_type | bundle         | deleted | entity_id | revision_id | language | delta | taxonomy_vocabulary_3_tid |
+-------------+----------------+---------+-----------+-------------+----------+-------+---------------------------+
| node        | project_module |       0 |    980666 |    13428179 | und      |     0 |                        53 |
| node        | project_module |       0 |    980666 |    13428179 | und      |     1 |                        57 |
| node        | project_module |       0 |    980666 |    13428179 | und      |     2 |                        58 |
| node        | project_module |       0 |    980666 |    13428179 | und      |     3 |                        64 |
| node        | project_module |       0 |    980666 |    13428179 | und      |     4 |                        59 |
+-------------+----------------+---------+-----------+-------------+----------+-------+---------------------------+
5 rows in set (0.004 sec)

Yet I see via the UI 3 elements.

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

In D7, the paragraph starting with "When using batch mode, it is important to sort the view..." was added recently, but it's not propagated to D10.

As we are working of a live DB "replica", I'm going to check there first, to see if the D7 data is correct.

I’d say still “needs work” as there are actions/feedback not addressed yet.

This is ready to review now.

See downstream pipeline running successfully in all cases: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/273652
* The legacy driver and variables are used in versions prior to 11.0.x
* The new driver and variables are used in version 11.x

If the changes are backported to previous Drupal versions ( 📌 Use selenium/standalone-chrome instead of our chromedriver image Needs work ), like 11.0.x or 10.4.x, then we can just remove the overrides for those jobs.

Once all supported versions use the new driver only, we can remove all the "legacy" related code.

Could it be related to the erros in phpstan somehow? https://git.drupalcode.org/project/domain/-/jobs/2511587

I guess it might be related to some error outputting making the job fail but not dumping the information, like suggested here https://stackoverflow.com/questions/69006060/dumping-variables-in-phpuni...

This might even come from core, but in any case, I'd try to address the phpstan errors first to make sure that it's not getting in the way.

I like the ideas and suggestions given. It'd be hard to put the alias in the class (or trait) itself since any other class can use another alias, but yeah, we need to find a way of "finding" them and link to the right places.

Adding some testing cases to the description so we can cover some of the cases.

One of the blockers was committed now. Updating description.

Merged the MR. For now, only maintainers with "main" or "1.x-latest" in the ref value (https://project.pages.drupalcode.org/gitlab_templates/info/templates-ver...) will get this change.

It will be part of the next release. Thanks all for the back and forth. Happy for improvements to be made in follow-ups.

It actually ran in the very first time, so the output there was the second run, where the 0/0/0/0 is expected if there isn't anything new.
Fixed. Thanks!

I've merged the changes and setup the migrations. Thanks!

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

Thanks so much for working on this. In the provided phpunit.xml file, there is reference to a class that no longer exist in Drupal 11.
Setting to "Needs work" based on that.

We've had some more reports via slack about this situation. While the solution is not perfect, it fixes the immediate issue, so I'm setting this to RTBC and I will commit it shortly.

I'm happy for follow-ups to be created, if needed.

Merged. Thanks!

Thanks for the changes and the tests.
Downstream pipelines are running here too https://git.drupalcode.org/issue/gitlab_templates-3470918/-/pipelines/26...

Everything looks good. I'll merge shortly, release 1.5.7 and make it the default ref.
Thanks.

I like that. It a good workaround so we can keep the allow_failure:true setting for next major.

True!! I forgot about that part. 🤦‍♂️

Well, again, this "allow_failure: true" is just temporary, so again, I'm not too worried because we happen to be right in the middle of the transition period.

So if the "needs" vs "dependencies" won't make a difference, the action here might be to do nothing and wait a week or two, as the errors are being reported anyway. We might need to give some extra support via here or slack but we know where to look at now after the investigation here in this issue.

In any case, if we find out after the investigation here that having "allow_failure: true" in the "composer (next major)" is not really beneficial, we can remove it, as we still have it in the testing jobs.

It might still be the "allow_failure: true" that makes it run.

If you remove it:
- with "needs" it should still run, but only after "test a"
- with "dependencies" it would not run

I'm not 100% sure tho.

Yup. Maybe we can use this issue to experiment a bit on this.

This is only temporary though: https://git.drupalcode.org/project/gitlab_templates/-/blob/main/includes...

And even earlier, in the composer next major job: https://git.drupalcode.org/project/sobki_profile_bootstrap/-/jobs/2601434

I think that’s the culprit

I left some comments in the MR and @drumm did so in this issue as well. Before we move forward with architectural decisions (in both CSS and Drupal content types configuration), let's try to agree on what would be valid vs not.

This might be then a change with the run-tests.sh core script, so the issue might be there.

A workaround while the issue is worked on is to rename the file to, for example: MyModuleTestBase.php

> Make the rewriter more intelligent, in some way

Maybe we can do it less intelligent. Just a rewrite of the whole line into what’s expected for the job. This was suggested by @cmlara in the previous issue (i think) and it might be the way to go.

So just rewrite to:
core_version_requirement: ^12

I don’t think we should overcomplicate the logic too much.

Happy to reuse any existing job elsewhere. Note that this is just an internal task, not for contrib space.

I think I found the reason for this. The use of <!--break--> in the description changes the behaviour and make it act as a "summary", which is not trimmed.

The display is set to "teaser" https://git.drupalcode.org/project/drupalorg/-/blob/7.x-3.x/drupalorg/dr...

And that teaser description is set to "summary or trimmed" with 600 characters as limit, which is obviously not respecting.

I checked the "Barrio Bootstrap 5 Theme" node to see if it had a long summary but it's not the case, it's empty.

I made a request to our marketing team to see what they can do about it. We don't control "The Weekly Drop" but we sometimes send them blog posts for consideration. "The Drop Times" already posted about it.

From my point of view, as I tested this part yesterday. I'd say it's good to go. If any other palantiry wants to RTBC+1 then great, but happy from my side.

Agree. That's why we used the word "expected".

It'll depend on whether new issues/bugs are reported in the next week or two. But yeah, I think that these two are now requirements before Step 2 happens:
- #3458238: CI installs wrong Drupal version → (nearly there)
- #3462681: Add W3C compliant JS testing → (not there at all, and this one is more crucial)

I'd say let's focus on the above two first, watch the queue, and play it by ear. We have some flexibility in the dates and we don't need to force things, but also, if everything goes well, we can also stick to the plan.

I updated the issue description with the above issues.

Re first part of #30

In core/lib/Drupal/Core/Test/TestDiscovery.php:

    // Sort the groups and tests within the groups by name.
    uksort($list, 'strnatcasecmp');
    foreach ($list as &$tests) {
      uksort($tests, 'strnatcasecmp');
    }

Then in core/scripts/run-tests.sh, all the calls are like $groups = $test_discovery->getTestClasses($args['module']); and the result is accessed like foreach ($groups as $group => $tests) {...

Merged. Thanks so much!

I merged the MR. Thanks so much for this.

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

Yeah, I ran the script and it ran well. I also tried forcing an error and it found it.

Good point about the feedback on how to fix them. Happy for you to do that commit and it can go straight back to RTBC.

I tested the changes but no data for the files was migrated. Is this expected?

$ drush migrate:import drupalorg_migrate_blog_post_files
 [notice] Processed 0 items (0 created, 0 updated, 0 failed, 0 ignored) - done with 'drupalorg_migrate_blog_post_files'
$ drush migrate:import drupalorg_migrate_blog_post_media_documents
 [notice] Processed 0 items (0 created, 0 updated, 0 failed, 0 ignored) - done with 'drupalorg_migrate_blog_post_media_documents'
$ drush migrate:import drupalorg_migrate_blog_post_media_images
 [notice] Processed 0 items (0 created, 0 updated, 0 failed, 0 ignored) - done with 'drupalorg_migrate_blog_post_media_images'

I do get data for the blog posts:

$ drush migrate:import drupalorg_migrate_post
 538/964 [===============>------------]  55%

I merged the changes and renamed a couple of small things, so if there are any further changes needed, please open a new MR.

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

The script looks good and the output is very clear too. Thanks for this.
RTBC.

Do you need to revert the text that seems to be removed in the MR or is it really not needed?
I'll let you either fix it or comment here with whatever is next. The script looks good and we can start using it as it is an internal tool.

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

#3458238: CI installs wrong Drupal version → could be merged after it is RTBC.

#3462681: Add W3C compliant JS testing → should ideally be sorted before we do the final switch.