🇪🇸Spain @fjgarlin

Those are really valid points.

I'm sure that we can also change the code to not requiring an absolute URL. If "http" is found we can use guzzle, otherwise try "file_get_contents" or something like that.

Setting back to "needs work".

Suggested MR. This is not ideal as it's a static snapshot from June 2024 but for now this is the best we have. It won't be full PHP support but it will still be best that no support.

I still need to test it manually.

Found a version from 2024: https://web.archive.org/web/20240625000039/https://doc.php.net/downloads...

I might add this as a fallback.

Merged. Thanks so much!

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

It seems more and more like a difference between Drupal core versions. Core is trying to clean up a folder where some files have been placed. Also, these are warnings and they are failing because core configuration makes the job fail on warnings: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/phpunit.xml.d...

This is something that we could potentially control if/when 🐛 beStrictAboutOutputDuringTests=true or failOnRisky=true causing test failures due to PHP8.4 Implicit Nullable deprecation Active is done. But it really seems like a core issue. I guess they are not affected because they are running the tests with "run-tests.sh" as opposed to this example that is using PHPUnit binary (more info on how to switch between these in here https://project.pages.drupalcode.org/gitlab_templates/jobs/phpunit/).

It'd be great to identify the exact file(s) being added to those folders. Then based on that, we can see if we need to do some tweaks or maybe report the issue in core.

I've just fixed the link to the change record. Well spotted!

This is a great improvement, thanks!! Publishing the change record.

I'll merge it to "main" now and announce it on the #gitlab channel, in case some people which have "main" as reference want to try it. I will not tag it yet, and when I do, it will be in 1.7.0 as this is a new feature.

Thanks so much for the work here and the testing, it's great to have this flexibility while preserving the previous behaviour.

The code looks good to me.

Downstream pipelines: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/398524

Patch in #74 to MR: https://git.drupalcode.org/project/drupal/-/merge_requests/10925/diffs

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

Yes, I see the value on having that list collapse. Let's make that change too.

The warning is perfect, tho I'd say that once you create the change record, the link should point there instead of this issue.

I left a small suggestion in the MR.

I'm also thinking that we will need a "Change record" here, not for the normal integrations, but for those mentioned in #15, so they can know what to do to use the improved version (aka define the env variable before calling the script).

#18 💙
#19 Thanks for the improvements.
#20 Agree.
#21 Thanks for the extra tests. I'm not sure we really need to cover more combinations, at least that I can think of right now.

I've checked the code and I think all the logic and changes are solid. I can't think of anymore feedback to add.

Downstream pipelines running here: https://git.drupalcode.org/issue/gitlab_templates-3425446/-/pipelines/39...

I'll set it to RTBC for now. This was a huge effort and it offers a lot of flexibility. Thanks so much!

Thanks for the proposed fix. Maybe we could add a settings.local.php file instead? We are using core's htaccess so we'd need to manipulate it.

fjgarlin → created an issue.

Merged. Thanks so much!

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

Actually, those branches (2.1.x nor 3.1.x) don't even seem to be on the code.

You don't have a dev release for 2.1.x nor 3.1.x. Once you create them, the results should show.

Due to the rebasing issues, I needed to create a new MR with the same code but a clean git history.

Merged now.

fjgarlin → changed the visibility of the branch 1.0.x to hidden.

Rebasing brought 7.x-3.x as the MR was initially created against it... I reverted that and everything is up to date now.
RTBC.

Merged. Thanks!

Reviewed. Merging shortly.

I'd say https://www.drupal.org/project/openid_connect_windows_aad → seems to currently have a green pipeline and +5K installs. I'd just create an issue in their queue and prepend [ignore] in the issue title. Then link this one in the issue description to explain that it's nothing on their end but that we need it to ensure that we keep their integration running as expected and that should do it.

I'm happy to jump in that related issue if needed, but I'm sure it'll be ok.

Tested locally = tested locally with the ddev plugin? If that's the case, then great (tho we will still need to create the issue upstream so the templates and the package behave in the same way, at least to make them aware of that situation).

Just to be extra cautious, let's create an MR in any of these projects (reduced list): https://git.drupalcode.org/search?group_id=2&repository_ref=main&scope=b... and then test.
These are directly downloading an using the PHP script, and obviously not creating the variable beforehand.

Left some feedback. The approach looks good in my opinion.

Merged.

Looks good to me as well, Thanks so much for all the back and forth, reviewing and the fix!

Merged!

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

We should provide a fallback where, if the variable is not defined, then it should contain exactly the same list as before (scandir minus a few files/folders), as otherwise it can break integrations: https://git.drupalcode.org/search?group_id=2&repository_ref=main&scope=b...

We can create a follow-up in the "ddev" package once we merge this with the changes so the original file list variable is created before running the script.

The changes look good to me, we are basically adding a few words which we know can be really common and the "SQL" and "fullstack" dictionaries, which also seem like good and generic enough to add to contrib.

Downstream pipelines.

RTBC from my end.

Merged to "main". I'll do a release probably today and push it to all contrib.
Thanks!

Thanks so much for creating the issue upstream and in here and for the fix and the test.
The downstream pipelines are running here too https://git.drupalcode.org/issue/gitlab_templates-3499267/-/pipelines/39...

RTBC.

The description of the issue is already bigger than the conversation itself.

It was something like:

@catch: Does anyone know off the top of their head - when project browser sorts by usage, is that Drupal 8/10+ usage or does it also include Drupal 7 usage?
@fjgarlin: ...quick investigation... yes
@catch: here is an issue https://www.drupal.org/project/drupalorg/issues/3498849

Cool. As long as there is agreement on the logic then it should be fine.

We can exclude 7.x and 6.x prefixes and add up all the others. If this logic sounds good then it should be easy.

Checking compatibility of every single branch (branch name is not enough to determine compatibility with D8+, we’d need to check against updates.drupal.org XML and then only add some values.

Question about this: what about D8, D9 branches? These are technically EOL as well.

penyaskito → credited fjgarlin → .

Reviewed and merged. Thanks!

We should probably ship 🐛 EnabledSourceHandler should segment its storage more strongly Active before tackling this.

Suggested code, if the above is merged:

    if (empty($results->error)) {
      // Store the query results as a set of arguments to ProjectsResultsPage only if there are no errors.
      $storage->set($cache_key, [
        $results->totalResults,
        array_column($results->list, 'id'),
        $results->pluginLabel,
        $source_id,
        $results->error,
      ]);
    }

Merged.

Looks good. Thanks.

I’m having a look at the code and leaving some comments in the MR. Either I’m not understanding the logic too well or I think we are losing too much “cached” information with this approach. I’ll leave a comment on the issue to have better track of things.

Right now (before this issue), we are storing all results for all plugins for a particular query in a keyvalue object, all under the same key, for all plugins.

With this MR, we are storing under that same keyvalue object, only the results of one plugin, so if we are going back and forth between plugins, we'd need to recalculate things.

I think it'd make more sense to have the "key" of the object being specific to the query and the source. If the call is successful, then store it, and if it isn't (ie: $results->error is not empty), then don't store it (so it goes through the full external query again when the same data is required).

Possible approach:
getProjects says Returns projects that match a particular query, from all enabled sources.
We start and check the cache there, and I think it's wrong, because maybe one plugin was not working at the time.
We'd need to loop through the sources (quick) and then get the results, which may or may not be cached. ==> this is where we need to introduce the caching, per plugin, not globally.

Downstream pipelines with max php running: https://git.drupalcode.org/issue/gitlab_templates-3497525/-/pipelines/38...

The follow-up is this 📌 Split results storage in keyValue per plugin instead of global Active

fjgarlin → created an issue.

As the related issue was just committed, doing this is very straightforward. Ready to test.

In order to test, we could, in `DrupalDotOrgJsonApi.php`, at line 144, add any of the following:
throw new Exception("Exception Test", 0);
throw new RequestException("RequestException Test", 1);
throw new GuzzleException("GuzzleException Test", 2);

Then this should show into the front-end.

Agree, but as this is the very first plugin to implement this it made sense to do it here. We are not sure which conditions could trigger an error on other plugins, but yeah, in any case, a follow-up can address this if needed.

Feedback in the MR for 3.

The tests on #5 were made before the last round of changes, so we need to check again before and after, but things look good.

1. Yes
2. .idea is the folder that PHPStorm editor creates. We can leave it or remove it, no big deal. Happy to keep excluding it.
3. Correct, no symlink needed for build.env

Happy to continue improving this job (and its configuration). Thanks for the title change, I think it makes sense to see what needs improving rather than focussing on just a few variables.

I think we should evaluate options, like having variables that can be set per variant and that might override settings in a file, or maybe it's time we provide a contrib-specific PHPUnit file(s) per variant adapted to the context.

I updated the issue description but kept the original one.

Closing this as we don't know if it's an issue with the current set up for the GitLab templates. The issue happened in the old DrupalCI system.

Yeah, now that we have more words it make sense to have our own dictionary with the words instead of a massive array. We'll need to expose it as artifact too.

Adde a test and altered the fixture regeneration to cater for the new parameter. Ready for review again.

Re tests failing, all nightwatch tests seem to be failing in 2.0.x: https://git.drupalcode.org/project/project_browser/-/jobs/3935754

We won't be able to do a FunctionalJavscript test unless we fake the Drupal version as the query to the endpoint sends the "current" installed Drupal version.

It's not possible to change the value of a constant (Drupall::VERSION in this case) so I'm not sure which approach to follow.

Yup, I actually had that already staged locally

# GitLab Templates Changelog

## 1.6.14 - 2025-01-xx

#3496181 - Add `allow_failure: true` to max PHP variants.\
#3497842 - Bump core versions to 11.1.1/10.4.1.

## 1.6.13 - 2024-12-27

#3496181 - Update PHP_MAX variables to 8.4.\
#3439240 - Cspell: sanitize suggested words for dictionary.
...

I'd rather not add "cpp" and add a set of words. "cpp" seems to contain nearly 40k words (https://github.com/streetsidesoftware/cspell-dicts/blob/main/dictionarie...).

We could grab the dictionaries from D10 and D11 and add the diff, or at least see which is the diff and then decide what to do. Very similar, if not almost the same, to what you did in #17 ( https://www.drupal.org/files/issues/2025-01-06/words-in-D11.0-dropped-fr... → ).

I'd need to force-push, so it's not an option. I can revert and redo the commit, but that'll make the commit history even more messy. I'll try to be more careful in the future if there are several commits within the same issue.

That was my bad, I should have changed it before hitting the merge button. I don't know if we can do git commit amend. I can try it.

I like that approach. Get a "snapshot" of files/folders before and then copy those over. We need to make sure that hidden files are included in the list too.

Yeah I'm just trying to understand/identify the possibilities, which might be reflected via documentation.

In any case, we've been adding features and improvements as well as trying to mirror many of the checks that Drupal core does, not just for PHPUnit but for other jobs too, so I don't think that using (a slightly modified version of) core's PHPUnit file is a regression, as it was something that could have been done since day 0 but just wasn't (same as allowing PHPUnit vs run-tests.sh). I know that some people can see as a new feature, but some other could even argue that it was a bug that needed to be fixed.

The gitlab_templates have been since day 0 a "starting point" for GitLab CI for contrib modules, not a static template that will make things work now and forever. It'll make it easier, but it can't solve all the problems as the software itself move forwards. At any point, you can or could have taken a snapshot and then modify as needed.

The array syntax is weird, but if the CI check is happy, I'm ok with it.

The changes look good to me. I just added a comment about modifying a comment and maybe recovering a word back, just for clarity.

You got it: https://git.drupalcode.org/project/migrate_tools/-/merge_requests/76/diffs

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

Updated link.

Duplicate of 🐛 PHP function list download no longer exists Active . I could not find a local copy of that json file on my machine, which would at least cover most functions.

Wouldn't setting the default value of _PHPUNIT_EXTRA to --no-configuration be equivalent to what we had before the change?

Bump for review/test on this.

If we are able to keep BC that would be ok, but if we are also going to work on this, how about also adding a variable to set all linting jobs to allow failure or not? I see that most maintainers will want to allow all or none, rather than individually, so if we are going to add the ability we can aim for both granular and global, while keeping BC.

I'm in the same position here. I see arguments for either side.
Since we are allowing failure to non-default variants, I am going to merge this. Thanks.

Merged.

fjgarlin → created an issue.

Happy for those six names to be added too and for that to happen on this issue. I'd like to see how the core issue develops separately, but we can defo add the suggested fix to the contrib templates regardless.

The logic to symlink is here https://git.drupalcode.org/project/gitlab_templates/-/blob/main/scripts/... and we are skipping a few things, so at the very least we can add those three files into that array.

@jonathan1055 - thanks so much for the detailed investigation and insight. I'd be happy to add those dictionaries as soon as you say it's ready if it's going to help the rest of contrib with all these cspell issues.

@tr - you can easily modify your .gitlab-ci.yml file to point to the fork created for the MR of this issue. See this: https://project.pages.drupalcode.org/gitlab_templates/info/testing-mrs/

PB won’t be included as stable and part of core until D11.

In any case, this is not relevant for this issue. This issue is about querying the d.org api endpoint and relaying an error message, if present. Changing the message and/or versions to show the message, would be a drupal.org issue.

I agree with that approach. This MR would be ready and the other issues can be tackled separately

Sorry I'm away from the computer most of the time so I'm just doing quick checks. Yeah, in this case, because it's a new file only introduced in this MR, it might need setting the other variables too, as explained here https://project.pages.drupalcode.org/gitlab_templates/info/testing-mrs/#... (feel free to make suggestions on how to improve that if needed).

Re #31 questions:

* How to properly merge PHPUnit arguments when using the coverage job? At the moment, it concats the PHPUNIT_COVERAGE_ARGS with PHPUNIT_EXTRA. This may lead to duplications, which result in PHPUnit warnings.
--> I'd just document this behaviour in the variable description and the documentation page (still missing in the MR), that way we'd only add values in PHPUNIT_COVERAGE_ARGS that are not present in PHPUNIT_EXTRA.

* How to enable Xdebug or PCOV properly? I see multiple variants in this issue, the comments and other contrib modules. Note that "line coverage, branch coverage, and path coverage data will be collected, processed, and reported. This requires a code coverage driver that supports path coverage. Path Coverage is currently only implemented by Xdebug."
--> Given that path coverage is only available in xdebug, I'd opt for that. This article is useful too: https://thephp.cc/articles/pcov-or-xdebug?ref=phpunit
--> Maybe we can code it in such a way where we can use either tool, but I'd consider this out of the scope of this issue, but could be done in a follow-up.

* What are the correct coverage definitions in phpunit.xml? I saw some concerns in 🐛 #3480767 Causes break in behavior of tests configuration Active . Currently, it points to the project files in web/modules/custom/... and excludes tests folders.
--> I'd opt for a "sensible" default, and we can discuss it in the MR if needed.

* How to treat modules requiring another test bootstrap? I am specifically thinking about existing site tests.
--> Can you give a more detailed example about a case that would need this and not the core bootstrap file?

* How does PHPUnit know about the database connection - although it is not defined in the phpunit.xml?
--> PHPUnit extends .phpunit-base, which extends .testing-job-base, which includes the "with-database" service, and also extends ".test-variables" which contain the DB credentials to use.

✨ Display warnings/errors from DrupalOrgJsonApi backend Active is ready for review and introduces a way to send error messages to the front-end which we may or may not use here, but I'd like validation of that approach before continuing here.

This is ready for review.

I don't think there are any new errors provoked by these changes. The errors shown in the CI pipeline are not related to this issue:
- CSpell: words in files that are not modified by this MR
- Stylelint: CSS files that are not modified by this MR
- Nightwatch: failures already present in 2.0.x
- PHPUnit: failures already present in 2.0.x

Tested on Drupal 10.3.2:

The message shown comes from https://www.drupal.org/drupalorg-api/project-browser-filters?drupal_vers... →

When tested on Drupal 11.0.1 the message does not appear.

Endpoint to query: https://www.drupal.org/drupalorg-api/project-browser-filters?drupal_vers... →

Rebasing might fix the curl issue as well.

Rebasing the fork will eliminate one error.

Then see the suggestion for the other

PHP_EXTENSIONS_SUBDIRECTORY=$(ls -d "$PHP_EXTENSIONS_DIRECTORY"/* | head -n 1)
                              ^-- SC2012: Use find instead of ls to better handle non-alphanumeric filenames.
For more information:
  https://www.shellcheck.net/wiki/SC2012

MR: https://git.drupalcode.org/project/bluecheese/-/merge_requests/154/diffs
Tested here: https://fjgarlin-drupal.dev.devdrupal.org/project/tfa_migration/releases...

Side by side d.o vs my own copy of d.o:

It happens when the screen size is under 420px. I will open an MR.

The markup in the html is

<a href="https://ftp.drupal.org/files/projects/tfa_migration-1.0.3.tar.gz" class="download">
  Download 
  <span class="filename">tfa_migration-1.0.3.tar.gz</span>
  <span class="extension">tar.gz</span>
</a>

So it must be something showing/hiding the class.

Merged. Thanks!

https://www.drupal.org/docs/getting-started/system-requirements/php-requ... →

10.4 and 11.1 are marked as PHP8.4 compatible. The MR looks good. RTBC and merging shortly.

Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3496181/-/pipelines/37...