🇪🇸Spain @fjgarlin

1.x is only used in api.drupal.org
7.x-2.x is used in www.drupal.org

Did the code you added here work? https://git.drupalcode.org/project/sessionless/-/blob/3.x/.gitlab-ci.yml...

That, together with the other examples, would help us improve this section of the documentation, but I want to make sure that it's working before assuming anything.

@jonathan1055 - re #32: that goes a long way to reviewing the changes. I usually check the commit by commit and the issue comment and that helps me a lot. Then at the end of the issue I do an overall review again just in case. I appreciate the clarity of information/changes and tests between reviews.

Happy for the follow-up to be open and worked on.

--

@grimreaper - no worries, even opening useful issues and providing possible fixes or code snippets you're also helping, so we appreciate that too.

Merged to main. Thanks so much!

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

I reviewed the code again and it looks good. RTBC.

is the badge purely related to the opt in status

Yes.

I agree that if the concept of security status does not exist in recipes, we should just not show it, regardless of where the recipe comes from.

We can differentiate between true/false and null to get over the technical hurdle in the code. This way, we won't need to refactor too much. Just a check for null, and ignore in that case.

I simplified a bit some of the cosmetic changes. I haven't tested anything yet so I don't know how close we are to a solution.

It's defo a good beginning, I made it into an MR so we can continue there, give feedback, etc.
https://git.drupalcode.org/project/project_browser/-/merge_requests/651/...

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

RTBC+1 on the D11 branch.

However, it doesn't apply to 10.4.x, so I added another MR (https://git.drupalcode.org/project/drupal/-/merge_requests/10627/diffs) hoping that it gets ported to 10.4.

Keeping the RTBC status of the issue per D11.

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

Do you have an open issue in your module where we can maybe investigate / try things?

Untested code (as I don't have a copy of security.drupal.org) in the MR.

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

Agree, happy to add the verbose option to the prepare-cspell.php script.

Other than this, it looks good. Just setting to NW to clean up some of the debug and to put some behind a verbose flag.

Not sure what you mean with The ".phpunit-base" in current docs is wrong in general case.

There are two examples provided in the docs, which come from actual modules:

You can use apt-get to install new packages. For example, imagemagick needs extra packages:

.phpunit-base:
  before_script:
    - apt-get update
    - apt-get install -y --no-install-recommends imagemagick

Or Redis needs the redis PHP extension:

.phpunit-base:
  before_script:
    - apt-get update
    - apt-get install -y --no-install-recommends $PHPIZE_DEPS
    - pecl install redis && docker-php-ext-enable redis

So it shows how to add a package and an extension, viewing the basic apt-get, pecl and docker-php-ext-enable usage.

I'm not sure that we can cover all cases, but if you add an example here we can definitely add it to the docs.

Merged. I'll tag a new release and make it default-ref.

Looks good to me. Thanks so much!

As per #7, I'm happy if a follow-up issue is created to move those array of words into a separate file, where we can improve this and test it properly, but without the pressure of getting it out quick that this issue has.

The related issue was merged to "main". I will create a new release later today or tomorrow and make it the default, which should make this error go away.

I'm closing as duplicate.

Thanks all! Merged. As soon as we get the cspell issue ( 📌 Add new dictionary to cater for common words not in core dictionaries Active ) in, I'll do a new release and make it the default.

Not all extensions are installed in the same way, if you see the second example, after installing via apt-get, you might need to enable it via pecl or docker-php-ext-enable, so this might be the case for your extension (see this for example https://stackoverflow.com/a/64445115).

See the redis module for example: https://git.drupalcode.org/project/redis/-/blob/8.x-1.x/.gitlab-ci.yml?r...

Documentation changes are more than welcome, as the docs live in the code of this repo too.

My bad. Probably a multicursor edit without noticing. Back to Needs review.

We can see it collapsed in here: https://git.drupalcode.org/project/keycdn/-/jobs/3759045

Thanks for running the tests. With the external file approach, we'd need to include it as well in the artifacts, and for example, in the second run, the cspell.json file has "path": "./extra-dictionary.txt", but the artifact is not in there.

I think that if we see the number of words growing to more than 15-20, we can do the extra file/artifact in a follow-up, but for now it might just be easier to add the files to the array as we had "ddev" and "lando". The only reason that I'm pushing for this simpler approach is that the solution would be lower risk and we can merge it and create a new release before tomorrow.

I added the last run of stylelint in a collapsible section. Ready for review again.

It's due to a core update that shipped into 11.1.0. This issue should solve it, and see the related as well 🐛 Stylelint jobs are failing due to upstream updates Active

Related issue.

Correct. I even reviewed the order today 🤦

I’d prioritize the other first and then reevaluate this if needed.

I’m thinking that while we have a handful of them only we might want to keep them in the array that will also populate the artifacts, instead of as a separate file.

That way if maintainers see the cspell config file with those words they might choose to keep them or not.

If or when we have more words we might consider a separate file but if we want to do a quick-fix I’d do it in the most simple way. Thoughts?

Quick change hopefully. Need to be tested on older versions of Drupal before the change happened. It might be compatible

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

Not a problem. Yup, once 11.2.x is out we should swap to that one.

Deployed to prod.

These are the 4 scenarios we are covering so far:

1. No Drupal version passed
https://www.drupal.org/drupalorg-api/project-browser-filters →

{
  "active": [
    "e767288c-9800-4fb4-aeb8-8c311533838a",
    "219c1cf2-dd7f-474b-9dd5-a26643fbc699"
  ],
  "maintained": [
    "089406ad-304d-4737-80d1-2f08527ae49e",
    "cee844e2-68b5-489d-bafa-6a0ade2b6dfd",
    "09a378d2-fd35-41f3-bff0-10d9801741a4"
  ]
}

2. Drupal 9 version
https://www.drupal.org/drupalorg-api/project-browser-filters?drupal_vers... →

{
  "active": [
    "e767288c-9800-4fb4-aeb8-8c311533838a",
    "219c1cf2-dd7f-474b-9dd5-a26643fbc699"
  ],
  "maintained": [
    "089406ad-304d-4737-80d1-2f08527ae49e",
    "cee844e2-68b5-489d-bafa-6a0ade2b6dfd",
    "09a378d2-fd35-41f3-bff0-10d9801741a4"
  ],
  "drupal_version": {
    "supported": false,
    "message": "Not sure how you are using project browser with Drupal 9 or earlier"
  }
}

3. Drupal 10 version
https://www.drupal.org/drupalorg-api/project-browser-filters?drupal_vers... →

{
  "active": [
    "e767288c-9800-4fb4-aeb8-8c311533838a",
    "219c1cf2-dd7f-474b-9dd5-a26643fbc699"
  ],
  "maintained": [
    "089406ad-304d-4737-80d1-2f08527ae49e",
    "cee844e2-68b5-489d-bafa-6a0ade2b6dfd",
    "09a378d2-fd35-41f3-bff0-10d9801741a4"
  ],
  "drupal_version": {
    "supported": false,
    "message": "Upgrade to Drupal 11 to continue using Project browser, <a href=\"https://www.drupal.org/project/drupalorg/issues/3494314\">details are in this announcement</a>."
  }
}

4. Drupal 11+ version
https://www.drupal.org/drupalorg-api/project-browser-filters?drupal_vers... →

{
  "active": [
    "e767288c-9800-4fb4-aeb8-8c311533838a",
    "219c1cf2-dd7f-474b-9dd5-a26643fbc699"
  ],
  "maintained": [
    "089406ad-304d-4737-80d1-2f08527ae49e",
    "cee844e2-68b5-489d-bafa-6a0ade2b6dfd",
    "09a378d2-fd35-41f3-bff0-10d9801741a4"
  ],
  "drupal_version": {
    "supported": true,
    "message": "Supported version."
  }
}

From #10

11.2.x is available, which won't be for months.

The moment that 11.2.x is created, we will get an alert, and 11.2.x will be branched from 11.x when it happens, so I think it is ok like this in the interim.

I did a small change as the array key wasn't still correct and reapplied the changes. I also left some feedback in the MR about a small thing and some other part that I don't think it's needed at all.

Bit things are looking really good already.

Test URL: https://kbentham-drupal.dev.devdrupal.org/project/drupal/issues/3464771

https://docs.gitlab.com/ee/ci/jobs/job_logs.html#custom-collapsible-sect...

That might be a good compromise. We can run the last command inside the collapsible section.

Combined request

// https://drupalorg.ddev.site/drupalorg-api/project-browser-filters?drupal_version=10

{
  "active": [
    "db118813-d01c-4a58-aee4-0fe61092b85b",
    "dff295b3-122b-4ca0-85a7-777901b003cc"
  ],
  "maintained": [
    "034d3c61-5bd4-4878-b37c-501e6f12a6db",
    "cd604554-9bcb-4e9e-9d8e-da09e60dd13a",
    "ce5ceae9-cf50-4b66-af58-0c000716c63d"
  ],
  "drupal_version": {
    "supported": false,
    "message": "Upgrade to Drupal 11 to continue using Project browser, <a href=\"https://www.drupal.org/some/announcement\">details are in this announcement</a>"
  }
}

or without the parameter:

// https://drupalorg.ddev.site/drupalorg-api/project-browser-filters

{
  "active": [
    "db118813-d01c-4a58-aee4-0fe61092b85b",
    "dff295b3-122b-4ca0-85a7-777901b003cc"
  ],
  "maintained": [
    "034d3c61-5bd4-4878-b37c-501e6f12a6db",
    "cd604554-9bcb-4e9e-9d8e-da09e60dd13a",
    "ce5ceae9-cf50-4b66-af58-0c000716c63d"
  ]
}

Commit with the changes: https://git.drupalcode.org/project/drupalorg/-/commit/164f8709e37cba8f96...

I left feedback in the MR.

As this change is just isolated to phpstan and also only affects the artifacts generated, I think it's safe to merge.
We can get wider testing if it's in contrib and we can always revert if needed, but I think the change is pretty solid (and so was the testing).

We can reopen if needed (and revert if needed) or create follow-ups if new things come up.

Merged!

I made a minor comment in the MR. I think we can remove the debug and it should be good to RTBC. But NW for now just for the previous things.

I think this one is good to go.

Thanks for the test. It's great and it works, but I think the output of the job can be improved.

Right now we have:

$ $CI_PROJECT_DIR/$_WEB_ROOT/core/node_modules/.bin/stylelint --ignore-path ./.stylelintignore --config $CI_PROJECT_DIR/$_WEB_ROOT/core/.stylelintrc.json ./**/*.css --custom-formatter=@gitlab-formatters/stylelint-formatter-gitlab --output-file=$CI_PROJECT_DIR/gl-codequality.json $_STYLELINT_EXTRA
[{"type":"issue","check_name":"length-zero-no-unit","description":"Unexpected unit (length-zero-no-unit)","content":{"body":"Error found in length-zero-no-unit. See https://stylelint.io/user-guide/rules/length-zero-no-unit for more details."},"categories":["Style"],"location":{"path":"modules/sobki_admin/assets/css/tabs.css","lines":{"begin":2,"end":2},"positions":{"begin":{"line":2,"column":18},"end":{"line":2,"column":20}}},"severity":"major","fingerprint":"f0019a3a5b3ffaedd4ab6fe3f08a1439"},{"type":"issue","check_name":"value-keyword-case","description":"Expected \"NO-REPEAT\" to be \"no-repeat\" (value-keyword-case)","content":{"body":"Error found in value-keyword-case. See https://stylelint.io/user-guide/rules/value-keyword-case for more details."},"categories":["Style"],"location":{"path":"modules/sobki_admin/assets/css/tabs.css","lines":{"begin":18,"end":18},"positions":{"begin":{"line":18,"column":47},"end":{"line":18,"column":48}}},"severity":"major","fingerprint":"a104c38322af0b689ac191cbd22f4639"}]

Whereas before we were capturing that output into a file and therefore there was no output:

$ $CI_PROJECT_DIR/$_WEB_ROOT/core/node_modules/.bin/stylelint --ignore-path ./.stylelintignore --formatter verbose --config $CI_PROJECT_DIR/$_WEB_ROOT/core/.stylelintrc.json ./**/*.css --color --custom-formatter $CI_PROJECT_DIR/$_WEB_ROOT/core/node_modules/stylelint-junit-formatter $_STYLELINT_EXTRA > $CI_PROJECT_DIR/junit.xml

So I wonder if we can make it not output anything? We can dump the output to "/dev/null" or to a file that we just ignore, as long as the artifact is correctly generated.

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

I thought I triggered the downstream pipelines, but clearly didn't (I did it in some other issues), so here they are: https://git.drupalcode.org/issue/gitlab_templates-3397162/-/pipelines/37...

I'm happy to wait for other people to review here too.

Good suggestion. Implemented in here: https://git.drupalcode.org/project/drupalorg/-/commit/361bb7ef267f66b631...

See the full logic in this block: https://git.drupalcode.org/project/drupalorg/-/blob/1.0.x/src/Controller...

The code looks good to me and all downstream pipelines work well. We'd just need to test it in a project where the "current" is D10.

I somehow missed the notification for this. It needed rebasing, which I did (and I needed to revert a commit along the way).

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

I've reviewed the commits after #47 and they make total sense. Having the variable would be useful for the cases where names will differ. Thanks for the additional testing. I'm setting this RTBC.

Ready for review.

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

We’d add this in settings.all.php

// Drupal versions supported for Project Browser endpoints.
// Semver format. Could have multiple ranges.
$settings['drupalorg_jsonapi_supported_drupal_versions'] = ['^11.0.0'];

Request would be (not deployed): https://www.drupal.org/drupalorg-api/project-browser-check-version?drupa... →

{
  "supported": true,
  "message": "Version 11.0.8 is supported."
}

Or https://www.drupal.org/drupalorg-api/project-browser-check-version?drupa... →

{
  "supported": false,
  "message": "Invalid version."
}

fjgarlin → created an issue.

@e0ipso @bbrala - is this MR something that you could review and give feedback about? The rest of us can look at it but won't have such deep understanding of what's in core and in jsonapi_extras.

Reapplied the MR in https://kbentham-drupal.dev.devdrupal.org/project/drupal/issues/3464771.

We are getting:

Notice: Array to string conversion in drupal_attributes() (line 2516 of /var/www/dev/kbentham-drupal.dev.devdrupal.org/htdocs/includes/common.inc).
Notice: Array to string conversion in drupal_attributes() (line 2516 of /var/www/dev/kbentham-drupal.dev.devdrupal.org/htdocs/includes/common.inc).
Notice: Array to string conversion in drupal_attributes() (line 2516 of /var/www/dev/kbentham-drupal.dev.devdrupal.org/htdocs/includes/common.inc).
Notice: Array to string conversion in drupal_attributes() (line 2516 of /var/www/dev/kbentham-drupal.dev.devdrupal.org/htdocs/includes/common.inc).

And markup:
td #attributes="Array Array"

I'm adding a few suggestions and a question to the MR.

The reason is not the username change, the reason is because you didn't do any comment in the d.o issue itself, only in the MR.

The code trying to match username to profile links is:
- https://git.drupalcode.org/project/drupalorg/-/blob/7.x-3.x/drupalorg/js...
- https://git.drupalcode.org/project/drupalorg/-/blob/7.x-3.x/drupalorg/in...

When it doesn't find a match, it just renders the name. When getting the list of users, it queries the d.o comments.

The amount of work to get this correct for this case might be too much, as we'd need to do API calls before render to get the activity, or additional calls to match users that aren't matched yet, and all that for some contextual JS functionality, so not sure it's worth.

So to reiterate, it's not your user, it's just that you didn't do any comment on the issue itself.

Merged.

I created the MR from the above commits from @hestenet. As of right now 11.1.0 is still not released. Also, 11.2.x is not created yet and it might take a bit to be created.

We'll also need to update scripts/check-versions.php with the new tags. But pipelines aren't failing yet, so I'd just wait as the failed pipeline will suggest what to change.

Setting to NW based on those things.

Thanks for porting the changes to PHP5.6 and for the additional tests.

I've re-checked the code and I think this is ready to be merged.

Re IID, there is some code that needs to be tested. I just added it to the MR as a comment for clarity.

We will have an opt-in for projects, and initially, we have mechanisms to revert the migration in case something goes really bad. But yeah, we will need to test with a handful of projects and then test references and a few other things before doing more projects.

Once I come back to work on this, I'll mention it here and we can see what the next steps are.

I also like the idea of a new on-demand job. Adding that new feature will be easier than modifying this key part of the CI system.

@simonbaese - there was no additional work since September and the issue is not assigned so I'd say feel free to work on the existing MR, or create a new one as needed. I've rebased the "main" branch from the fork, because the other branch (linked to the existing MR) was giving a conflict. Feel free to create a new branch and MR and borrow as much as you need from the existing MR taking into account all the feedback above.

Thanks for creating the issue and the MR! Code looks good.

I applied the MR to this d.o copy (mine had changes about the new system): https://kbentham-drupal.dev.devdrupal.org/project/drupal/issues/3464771

I can see that \n\n\n is getting into the message.

It's a great beginning!! I made some comments about the emails and about the above issue.

I've just seen that the current file handling that uses jQuery: https://git.drupalcode.org/project/drupalorg/-/blob/7.x-3.x/drupalorg/js...

So leave the code as is for now then

We’re trying to not use jquery on js code if possible unless there is a solid reason to use it. I can try the MR for a POC but we’d want jquery removed in the final version.

WIP so take with a grain of salt, but yes, I'm implementing conventional commits in the new UI ever since I saw this issue.

Twig: https://git.drupalcode.org/project/contribution_records/-/blob/1.0.x/tem...
JS: https://git.drupalcode.org/project/contribution_records/-/blob/1.0.x/js/...

There are some TODOs in the code, but the idea is there.

It also needs the readme changes as that step shouldn’t be needed anymore.

fjgarlin → created an issue.

Confirmed, it's hardcoded into the module here: https://git.drupalcode.org/project/entity_pager/-/blob/2.0.x/tests/src/F...

That pipeline is testing Drupal 11.0.8 and that version does not have that file: https://git.drupalcode.org/project/drupal/-/tree/11.0.8/core/modules/sys...

So the question is, why is the test trying to download that one? That seems to be module specific I guess.

I'd say the main version is good to go, so we need to apply the same for D7. I left some comments in the MR in case we want to make the PHP script compatible with old PHP versions. NW for the D7 part. Great work so far.

Agree, which version of the module you get will be based on the composer.json configuration. Right now the opt-in is a project-wide setting, so I'm not sure we can do something about this. Perhaps Package Manager can detect the composer.json configuration that would allow for non-stable packages and throw a warning in the page?

Good point, let's use that other variable instead when calling the PHP script (we can leave the default value, when empty, inside the PHP script).

Feedback addressed. I couldn't use tokens as the search form is a solr one that we are building in drupalorg_browse_projects_form, but I could replace the category in the URL instead.

Merged. Thanks!

Merged. Thanks both for working on this and reviewing/testing.

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

Please read this before offering to co-maintain more modules https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →

Especially the first point.

Please read this before offering to co-maintain more modules https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →

Please read this before offering to co-maintain more modules https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →

Please read this before offering to co-maintain more modules https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →

Please read this before offering to co-maintain more modules https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →

larowlan → credited fjgarlin → .

As per internal slack conversation (https://drupalassociation.slack.com/archives/C07AN2SMVNZ/p1732726546624469), investigate if we can add specific source migraiton for the paragraph lookup plugin calls.

Maybe we can do an internal task to check that. I found this which could be useful: https://github.com/tcort/markdown-link-check
This could be done in a follow-up as this issue is for documentation updates.

Yeah. maybe we just need to tweak the language slightly about which ones get updated. It might not be a full set of warnings for some, but it will be for most using the common configuration.

My biggest concern is to go into a rabbit hole where we start checking dictionaries, as a project can have many, and those dictionaries (core, lorem ipsum...) might have unused words.

If we stick to only the project ones, how can we tell the project's dictionaries (which can be named differently) from any other ones, and then see how it mixes up with the words variables and/or configuration file.

For me, the work done so far is good enough for a RTBC as it is an improvement, but I'd let you comment on my above concerns to see if we draw the line here or we continue.

Mergin this. Thanks!

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

I'm going to merge the current MR as it's a good round of improvements. As usual, we can open new ones. Great improvements!

Thanks for the related issue and for the documentation update. RTBC.

I think that it should just take the same status as the project that those recipes come from. We are looking for recipes in core and contrib modules in here: https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/src/Plug...

Recipes from core can be safely put as "Covered", and the ones from contrib should be able to change depending on the module.

When navigating through the above recipes (gathered from multiple sources), we already have an "if/else" block for core vs not-core in here: https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/src/Plug...

So I suggest doing the calculations there and then adapting the code below with the calculated value.

Good discovery!! I don't think it was obvious to anyone!

I think that the MR and the tests look good. There is a suggestion to clean up debug code (and there are more debug code I think), so let's clean this up before merging. I'd set it to RTBC but setting to NW for the clean-up.

Thanks for reporting back. As drupal/upgrade_status is a external dependency that is not needed here, I think there is still nothing to do here.

I left a question in the MR about the GITLAB_TEMPLATES_VERSION variable. The rest of the code looks good, but I wonder if we can detect this differently rather than having another variable that we'd need to update.

Merged. Thanks!

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

Everything is new code for the new component, so it's safe to merge. Thanks!

Re #18, while we could do that, it kind of sounds like a feature that should be in "cspell", same as the one in "phpstan". A quick google search shows this https://www.npmjs.com/package/cspell-check-unused-words, which we could try to apply.

Agree that we just need to add an explanation about the files in the documentation, but the code overall looks good.

Back to NW for the documentation and just in case we want to try that plugin (if it works great, if it doesn't maybe a follow-up).

Thanks for adding the logo. I created the related bluecheese issue and is ready to review.
I also added the categories, which in themes are just "development status" and "maintenance status".

All this can be seen again here: https://fjgarlin-drupal.dev.devdrupal.org/project/project_theme?f%5B0%5D...