🇪🇸Spain @fjgarlin

Reviewed and merged, thanks!!

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

https://hub.docker.com/r/drupalci/php-8.1-ubuntu-apache

Thanks so much! This helps a great deal!!

I added a rule in a place that is used by multiple jobs, but I think it might actually be ok, as I'm not sure if we need these jobs to run on tags.
I'd say this is ready for review.

fjgarlin → created an issue.

No need to build anything if making aliases / tags of existing images is an option.

The goal here is that 8.1-apache and 8.1-ubuntu-apache (and the other versions listed in the issue) download THE SAME image.

Thanks so much, merged!!

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

Done.

Yup, no problem. I'll commit the patch. Thanks!

If aliases or tags have the same effect, those would be totally ok. I just don't know enough about dockerhub to know which solution might be easier. I'll edit the IS with this.

Yeah, it won't hurt to have it. If the other issue is fixed, we would still want people to review their integrations (if they look at the change records).

Created ✨ Release ubuntu images for PHP 8.1 and 8.2 Active

Created ✨ Release ubuntu images for PHP 8.1 and 8.2 Active

Linking related issues.

fjgarlin → created an issue.

I'd happily have "Change records" in issues when needed. We have the changelog, the documentation pages, and the deprecation jobs, but sometimes we can't foresee or cover all situations, so yeah, a change record that explains the change and what needs to be done would be good.

Maybe we can make the first "Change record" be the image change that caused some BC issues.

Merged, thanks!

Merged, thanks!

Thanks for the extra checks and explanations. Merged!

Thanks for putting the MR together.

Downstream pipelines for MR293: https://git.drupalcode.org/issue/gitlab_templates-3475974/-/pipelines/34...
The code there is way cleaner and we are not introducing any more variables/logic, so if that fixes the issue, I'd defo prefer that.

We might need to extend things to the test-only job and perhaps the nightwatch ones, so setting to "Needs work" based on that, but it's a clean and great approach.

Right now, we only have one development branch and we are not thinking about creating a new one or maintaining two versions of the templates (https://project.pages.drupalcode.org/gitlab_templates/help/contributing/). There are many improvements or things that we would have done differently if we were to start again, but I think the templates are stable enough for all contrib.

With the related issue, if it goes through, we'll have the ability to run tests as root or not, and that would solve problems like the environment variables.

I'm going to mark this as postponed as it'll either be a breaking change or might require maintaining multiple branches/versions, which we don't want at the moment.

Thanks for checking.

Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3487525/-/pipelines/33...

The code looks good and there are tests on #12, #15 and the downstream pipelines. Setting to RTBC.

Oh cool, even better that it detects new files!

Thanks for the testing. Given that the code changes are really minor and the tests included in #8, this is RTBC.

Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3487169/-/pipelines/34...

I was about to set it to RTBC in my last comment, so after the successful test in #7 plus the pipelines working, I can set it this time. Thanks!

That's the thing, we need to parse and reconstruct so all the links can be populated correctly, otherwise showing the code with no links would be somewhat useless. That parsing and reconstructing is currently the "heart" of the API project. I'm sure that there are better ways to do the things, but for now, that part of the module is unlikely to change. We "just" need to add support for new features.

@cmlara - does the current MR in this issue match what can be done today? Or is something else different needed?

I'd call this a duplicate of the related issues, and these are probably the most important issues that we currently have with the transition to PHP attributes. I won't close it so that when the work is done we can check issue by issue that all possible cases are fixed.

Issue to work on: https://github.com/drush-ops/drush/issues/4248

Agree with many of the points but i still think that providing a fix upstream to drush, if accepted, can be more beneficial to the community rather than writing the logic in here.

Ready for review: https://git.drupalcode.org/project/drupalorg/-/merge_requests/303/diffs

It wasn't sure if putting an absolute link or relative.

- Non-shared account: https://fjgarlin-drupal.dev.devdrupal.org/u/fjgarlin
- Shared account: https://fjgarlin-drupal.dev.devdrupal.org/u/asociacionespa%C3%B1oladrupal

Yeah, it makes sense that changes: doesn't include newly added files as those would actually be tested in the correct jobs (either PHPUnit or nightwatch), so maybe we don't need to worry about it.

We are trying to not have Drupal-specific in the templates.

We'd need to find the info file (which doesn't need to be on the root of the repo) and extract the dependencies. These dependencies could be in the old format (webform, ban) or new format (webform:webform, drupal:ban), those dependencies could have semver modifiers (linkit:linkit (>= 6)), etc.

That's a lot of logic that should not live in the CI templates. To me, it should live in the "drush" command where we enable the template (similarly to what the module enable does), or otherwise design an easy workaround that would not unblock common cases (that's why I suggested the variable, which is not ideal, but would unblock things).

Change record created, I'm not sure if this is the expected format: https://www.drupal.org/node/3488133 →

The @deprecate tag seems to require a change record node to be created.

It would be ideal if the drush command would have a "enable dependencies" flag or something.

Not ideal, but we could introduce a new variable just for this job called _UPGRADE_STATUS_MODULES_TO_ENABLE, and then do something like this: vendor/bin/drush --root=$_WEB_ROOT -y en $_UPGRADE_STATUS_MODULES_TO_ENABLE right before trying to enable the module/theme.

I made MR suggestions for the code to address my feedback and @alexpott's feedback.

Yeah, there is no clash because the names are different, so that made it easier. We didn't think of this because to be honest, it doesn't make too much sense to start several services knowing that you'll only pick up one, but on the other hand, having core 11.0.7 and 11.1.0-dev using a different one was something we didn't anticipate, so it's a good workaround/fix.

Also, that means that we are overriding one less part per variant, so the "inheritance/extension" will work better, not needing to re-define new sections per variant ("services" in this case) when they differ from the norm.

From my point of view, this is RTBC. I'm only curious about the performance impact, how much longer, if any, takes job creation as we are now adding one more service to the mix. It'd be great to have a before/after. I don't think that the impact will be big, but we'd need to take that into account somehow. I guess that enabling CI_DEBUG_SERVICES can give you useful info to know how long it takes to create the service.

On first inspection, the code looks good. I'm glad we are actually simplifying the process and that we don't need to keep calling the snippet to recalculate the variables.

Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3487169/-/pipelines/33...

I'm happy that both sets of variables are covered in this issue, it's also good to have two snippets writing to build.env to make sure that the env file is built correctly and the all needed variables are set.

Not setting RTBC as we'll want some tests on other contrib modules, but it's looking good so far.

I made a comment in the MR, it still needs some minor clean up.

Yeah I think we can follow the same expression as the phpunit-tests-exist-rule rule.

chrisfromredfin → credited fjgarlin → .

Sorry, the rebase button in the Gitlab UI did something silly, commit history is in the MR is messed but the changes look good. I'm merging this shortly.

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

Oh, this is a good suggestion. Starting services that we won't need is not ideal, but it's a consequence of the changes in core. The code looks much simpler and it also means we don't need to override that part for variants.

Downstream pipelines for decoupled_pages which has nightwatch tests: https://git.drupalcode.org/issue/gitlab_templates-3487525/-/pipelines/33... (you can always open an issue/MR there so you don't depend on me to run the downstream pipelines in this issue).

Yeah, I really think that this should be a drush issue that we can open upstream.

drush theme:install theme_name should behave as drush pm:install module_name and install the dependencies automatically, but it's not the case.

The rebase was long, I removed a duplicated part of the code and also forced yarn to install via the "node_modules" folder as some of the SASS still depends on it.

After doing all that, I merged the MR. Thanks!

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

Merged!

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

Merged!

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

Merged! Thanks.

Adding @spicywerewolf so credit can be given.

This might actually be a Drupal or drush thing.

It seems that when we do drush -y en module_name, it automatically installs the dependencies.
But when doing drush -y theme:enable theme_name, it does not.

We should try that in isolation, I think that's the culprit here, because the files are present (see composer job log).

Useful links:
- https://stackoverflow.com/questions/12427449/elasticsearch-boosting-rele...
- https://opensearch.org/docs/latest/query-dsl/compound/function-score/

Giving credit to people that helped me with some very useful insights.

fjgarlin → created an issue.

True! It's 100% that. We are in a weird state where some versions of core 11 use an older yarn version and some others use a newer version of yarn, and they require different chromedrivers.

If this is backported we'd be able to remove that override and your code would work, but for now it's not possible, so yeah, for now I suggest following @jonathan1055 advice.

Maybe you can get extra logs enabling CI_DEBUG_SERVICES? https://project.pages.drupalcode.org/gitlab_templates/info/common/#gathe...

I'd probably change the issue title to "Rethink weighting of projects and credits" or something like that, because it's not just the group module affected in here.

This is my personal opinion, not DA-developer opinion:

A project sitting at 190K would have the same weight as 100K and it's almost twice as much utilized. Also, being honest, almost any project with +10K installs is a really good and robust module, because +10K site builders wouldn't have chosen it otherwise.

So I would change the algorithm slightly.

Option 1:
Have a different scale for this. 100K might have been meaningful in D6/7 days, but I think that's a big bar to get to. Also 100K to 200K seems to be a really big gap.

So I'd probably consider a smaller bar (5k?) and smaller gap (5K?).

Option 2:
Change to a logarithmic-like approach.
eg:
- 100 installs -> 1 credit (floor)
- 1000 installs -> 2 credits
- 10000 installs -> 3 credits
- 100000 installs -> 4 credits
- 1000000 installs -> 5 credits (almost no project would fall here)
- Core + strategic projects -> 10 credits

This would still put projects like pathauto or token with 4 credits per issue, but really good projects like group would have 3, useful projects would have 2 and new/unknown/very-specific modules would have 1.

Figuring these numbers or algorightms will be tricky but might help boosting contribution on good modules and companies might have a real incentive to sponsor development on them.

Adding link with useful info https://www.drupal.org/drupalorg/docs/marketplace/contribution-credit-we... → and editing some bits of the issue summary.

fjgarlin → created an issue.

It was added here 🐛 _GITLAB_TEMPLATES_REF value in variables needs updating, and provide override variables for Curl Fixed

Test comment [ignore]

Don't worry, this was a hard one to foresee as we don't often use those variables and the MR testing requires us to set them up.

We can definitely try that, calculate them in the composer jobs, and them make them available to the rest of the jobs. Working on that other issue sounds good.

Giving credit as the issue reported in #28 was reported in slack by @mortim07

I've merged the hotfix.

Not sure why this didn't come up in the tests, but somebody reported this job https://git.drupalcode.org/project/data_pipelines/-/jobs/3349914 and we can see the _CURL_* variables empty.

I'm doing a fix for that.

Merged. Thanks!

Merged. Thanks!

Great, thanks so much for reviewing all the code and getting it to follow the standards close to core.
Merged.

I'm happy with the change to 120. Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3439021/-/pipelines/33...

RTBC unless you want to do any other improvement.

I left feedback in the MR.

From https://github.com/twigphp/Twig/compare/v3.14.0..v3.14.2

I guess the method ensureToStringAllowedForArray might be the culprit (it's a loop and it's recursive) as some Drupal objects are definitely quite big. They tried to fix infinite recursion in 3.14.2, but the fact that objects are traversed might cause the performance issue.

The logic looks good and the tests also prove that it works. RTBC.

I'd say exploring that other only if necessary. For now, letting core provide the right one is the easiest option. If we see that we start fighting it more than benefiting from it, we can go down the route of internal files, but not for now.

I'll merge this shortly.

You might need to hard refresh, but the logo is there already:

Indeed, it seems that you were copying the file to the docs folder after running mkdocs on your MR.

We have two options:
- Copying the file to docs before running mkdocs
- Copying the file to public after running mkdocs

Maybe we need to copy it to the public folder? I guess we can copy it after the mkdocs command.

I think you're testing here: https://git.drupalcode.org/project/scheduler/-/merge_requests/209/diffs
Can you change:

  logo: ../logo.png
  favicon: ../logo.png

Back to:

  logo: logo.png
  favicon: logo.png

Thanks for the MR! Merged directly.

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

The two files should always be the same, there is no reason for a different logo version in the documentation site

Maybe there is a reason for the logo to be different. For example, you might want to use a color variation as default but then another variant of it that matches with the default blue of mkdocs. I think that if the maintainer has put a logo in the folder, we should respect it.

We can add this logic to the documentation or even add some output to the job so maintainers who check it see that it's a possibility.

I would have thought that the cp would have a permanent effect as the public directory is built from there. I don't think we need to play with the URLs.

Yes, we could add a section for this in this page https://project.pages.drupalcode.org/gitlab_templates/jobs/pages/

Given that this is also a special file, we could even consider copying it directly during the CI job. If the file exist in the root of the repo and does not exist in the docs folder, then copy it.

Code wise it looks great! and the tests show it working as expected. I'm going to give it a RTBC but it'd be great if @cmlara also gives us the go-ahead.

Thanks!

The logo needs to be placed inside the docs folder. For gitlab_templates, we are just duplicating the image.

I think the main point that I'm trying to raise is that some basic styling from the front-theme is bleeding into the navigation bar and therefore affecting how it looks.

Not all themes (even heavily used Drupal ones) follow BEM rules.

I could reproduce again with Olivero following the instructions in the issue summary, so I'm removing the tag.
See the attachment here https://www.drupal.org/files/issues/2024-11-12/Screenshot%202024-11-12%2... →

Did you follow the steps I added to reproduce it, specially the one where the CSS of the button element is altered?

Maybe prepare-phpunit-xml.php?

Code looks good to me and the test in #14 shows that we're fixing the issue that we were trying to address.

I left a couple of comments in the MR, mostly to clean up (debug) code that is no longer needed. The rest look good.

Merged! Thanks.

Thanks for the code and also for the reasoning on #9.

I made a question in the MR that might slightly change the logic.

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

Merged! Thanks so much.