Account created on 26 February 2013, almost 12 years ago
#

Merge Requests

More

Recent comments

🇪🇸Spain fjgarlin

Also, if LocalModules is not extending DrupalDotOrgSourceBase, why do we even need to split DrupalDotOrgJsonApi into two classes?

🇪🇸Spain fjgarlin

I see great progress on this. Right now there is still a lot of code duplication between src/Plugin/ProjectBrowserSource/DrupalDotOrgJsonApi.php and src/Plugin/DrupalDotOrgSourceBase.php.

I assume that we are moving a big chunk to DrupalDotOrgSourceBase and that everything is still WIP, so I'll hold of until I see it's marked as "Needs review".

🇪🇸Spain fjgarlin

Hi @cedrxc, thanks for the interest in working on it but I had already started and I'm just finishing. You are welcome to see the commits in the MR. I just made the most and decided to include GitLab CI in this issue, as it will also check the composer dependencies.

🇪🇸Spain fjgarlin

Tested the actual sending with drupalpod

Merged. Thanks.

🇪🇸Spain fjgarlin

Rebasing d7-basic on the fork should trigger the behavior we are after, but for whatever reason there is a conflict and cannot be done via the UI.

🇪🇸Spain fjgarlin

That drush dl date issue where it offers options is one the reasons why D7-api is failing: https://git.drupalcode.org/project/api/-/jobs/4315997

I think it might be due to D7EOL actions on the modules I guess (no stable releases).

In any case, yes, not related to this issue.
--

Thanks for adding all the test cases in the issue description.

🇪🇸Spain fjgarlin

We can create the block plugin here, like these two: https://git.drupalcode.org/project/drupalorg/-/tree/1.0.x/src/Plugin/Blo...

Then we'll worry about how to place it.

Right now we are in an interim situation where the content of the new site is managed by a small group of people, so probably the link should go to this issue queue and then we can triage and redirect from here.

🇪🇸Spain fjgarlin

Oh, I saw the commit fixing it in the other issue but I thought that it was part of that MR only, I didn't realize it came from here. I'll merge the other one soon anyway, and if I get a RTBC on this one 🐛 Tags contained in manual rebase of issue fork triggers pipelines Active , I can even make a new release.

🇪🇸Spain fjgarlin

I think the downstream API test failures are unrelated to this change

Yes, probably due to 🐛 PHP function list download no longer exists Active . I know that seeing those red lights is not great but I still think that they are good projects to test things in. I need to find a gap and try to fix those tests for D7 (which is a new one) and D10.

🇪🇸Spain fjgarlin

Suggestions to try to remove some more /custom/ around the code:

Calculate the destination path as: $DRUPAL_PROJECT_FOLDER/../

    # These links are created in the folder above modules/custom/$CI_PROJECT_NAME and will be used in addition to the project's own .eslintrc.json.
    - ln -s $CI_PROJECT_DIR/$_WEB_ROOT/core/.eslintrc.passing.json $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/.eslintrc.json
    - ln -s $CI_PROJECT_DIR/$_WEB_ROOT/core/.eslintrc.jquery.json $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/.eslintrc.jquery.json

Calculate the relative path to pass to run-test.sh. Not sure how to do it in bash but it'll be something like (untested)directory=${DRUPAL_PROJECT_FOLDER//"$CI_PROJECT_DIR/$_WEB_ROOT/"/}:

        # if _PHPUNIT_TESTGROUPS is --all then add --directory modules/custom/$CI_PROJECT_NAME
        # Otherwise add $_PHPUNIT_TESTGROUPS (without the --group)
        WHAT_TO_RUN=$([[ "$_PHPUNIT_TESTGROUPS" == "" ]] && echo "" || ([[ "$_PHPUNIT_TESTGROUPS" == "--all" ]] && echo "--directory modules/custom/$CI_PROJECT_NAME" || echo "$_PHPUNIT_TESTGROUPS"))

I'm thinking more an more to leave these "leftovers" for a follow-up issue, so I'm actually setting this to RTBC.

🇪🇸Spain fjgarlin

We can change the ones that we are sure about here and now, and for the others, we can either leave them as they are or create follow-ups.

As we are touching a good number of jobs, I triggered all downstream pipelines here: https://git.drupalcode.org/issue/gitlab_templates-3505585/-/pipelines/42...

I will try to review more in-depth tomorrow as I'm logging off earlier today.

🇪🇸Spain fjgarlin

After triggering the update and reindex, the data seems to be ok: https://www.drupal.org/jsonapi/index/project_modules?filter[machine_name...

I created Track changes on migrations or update more regularly Active so we can take care of these things in the endpoint.

🇪🇸Spain fjgarlin

Also, we still seem to have some references to the "old" composed path (look in the code for /custom. It'd be great to see if we can simplify those with this variable, if possible.

🇪🇸Spain fjgarlin

The MR looks really good. I made really minor suggestions to the documentation page to make it more explicit that they are just examples and what the purpose of the given examples might be for.

🇪🇸Spain fjgarlin

Happy with DRUPAL_PROJECT_FOLDER. And yeah, +1 to mentioning it somewhere in the documentation.

🇪🇸Spain fjgarlin

We are not running migrations with --update. I’d expect that Drupal migrations catch the update at the source and propagate nodes that have been updated. But maybe this is not happening.

Migrations run once per hour, but just drush migrate:import drupalorg_migrate_project_module
The migration for projects is this one.

Due to our setup, we cannot use high water marks, but maybe we can use track_changes or run with --update once a day only. I just triggered a migration with --update manually in any case right now.

🇪🇸Spain fjgarlin

Since this is "the path of the project inside the Drupal site", maybe we can call it DRUPAL_PROJECT_FOLDER or DRUPAL_PROJECT_DIR. I know that we could add "drupal" to many variables (and I'm actually not a big fan of naming drupal-things with "drupal"), but in this case I think it makes a lot of sense.

PROJECT_FOLDER is kind of generic and could easily be confused with being the same as CI_PROJECT_DIR. The MR looks good, so I think it's just deciding on the name.

🇪🇸Spain fjgarlin

There is a single field_active_installs_total field, with just one value, usage can be tracked in https://www.drupal.org/project/usage/webform .

The cron job runs weekly and grabs data from the previous week parsed. Right now we can see in https://www.drupal.org/project/usage/webform how the last week parsed was February 2nd, so we're grabbing the data from the week prior to that, as the data needs to be be synced from D7 to D10. This is a temporary situation while usage data is being managed by D7 drupal.org

🇪🇸Spain fjgarlin

This looks good to me RTBC. Thanks for the tests. This gives us a lot of flexibility and the possibility of adding new test cases as we find new bugs in the "GitLab Templates" project.

Feel free to do the merge and the cherry-picking (when possible) to other branches.

🇪🇸Spain fjgarlin

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

🇪🇸Spain fjgarlin

1. That's a great idea! We can separate by space, or comma, or pipe.
2. 100% with that, I like it better this way, it's more generic and still easy to understand.

🇪🇸Spain fjgarlin

This should have already worked but the values are not updated, at least for the project linked in the issue description.

I added some extra debug so I can check logs to make sure that everything is running as expected: https://git.drupalcode.org/project/drupalorg/-/commit/7dcb07197b2268a795...

I'll continue monitoring it.

🇪🇸Spain fjgarlin

But just saw that you already transferred the commit!! Great!

🇪🇸Spain fjgarlin

Ouch! I had the MR ready, was just waiting for the pipeline to finish.

🇪🇸Spain fjgarlin

I think it was just to replicate what was done in here: https://git.drupalcode.org/project/drupalorg/-/blob/e31465608d1380345834...

ie: https://www.drupal.org/project/ckeditor

From the API endpoint is just a taxonomy term value, and then it’s really up to PB what to do with it. I think it can be cleaned up, and if we want to implement it, it can be done from scratch taking into account what is it we really want.

Currently, PB and filters only care about: active status and maintained status. In the early days, we were checking each possible value, and marking/showing some as "negative", but that changed with the introduction of other Plugins and it makes no sense anymore.

Should we want to show the descriptions of the values for the taxonomies, we could always do it, without the need to show a warning triangle.

🇪🇸Spain fjgarlin

I commented on the other issue, I still think that this should be done as author (person that created the node) is not the same as maintainers.

As mentioned in the issue description

The person that created a module might or might not be a maintainer of the module, and they might even cancel their account and the module would still exists.

🇪🇸Spain fjgarlin

Yeah, I think both issues can be valid.

Author, as it is (which is creator of the project) shouldn't be relevant.
What we want is maintainers, and this issue is the follow-up that Remove author from Projects Active was referring to.

So, I'd still remove author, clean up code, and then start afresh with maintainers. Note that this part of the API is NOT ready on d.o yet, so it won't be possible to do this yet.

🇪🇸Spain fjgarlin

Fecha tentativa para hacer esto: 17 febrero 2025 @ 17:00 CET / 10:00 CST

🇪🇸Spain fjgarlin

Oh cool, I think I understand now fully the whole logic. I saw yesterday's commit but I think I misunderstood it a bit.

So from the gitlab_templates (or UI with GTD), we can set PHPCS_BEFORE_SCRIPT to be a string value, and then we will use this value in the GTD project before_script to run certain options/tests/changes.

I left some small feedback on MR1.

I don't think we need to investigate the separate script idea. This is good as the logic lays in GTD, and then we can control which variable to use in gitlab_templates.

🇪🇸Spain fjgarlin

With the above approach:
- Let's not worry about the active directory at all. If a script needs running, it should include the necessary logic to go to the right place. That way the code will be simpler. We know that before_script will always start at $CI_PROJECT_DIR.
- With that syntax, it'd be great to try running a command, but also making reference to a script in the downstream pipeline (eg: .gitlab/before_script.sh - where that is the relative path within the repo).

If this work, maybe that's all we need really, as we could either run commands directly or call a script present in the downstream pipeline, so we don't need if/else logic or other edge cases.

🇪🇸Spain fjgarlin

there's a facility to write these plugins in a separate contrib module right now

100% - creating a new "source" or "plugin" via a contrib module is extremely easy.

🇪🇸Spain fjgarlin

Re #30. Correct, nothing is done so far and we'd need to add that functionality to the code in order to keep them.

🇪🇸Spain fjgarlin

Filters and category filtering might prove tricky unless you bring up all the information locally and then do all the filtering based on that.

I'm not sure that filtering would be needed if the module is already in the codebase. If we need/want some, I'd just do what the "core" plugin is doing, which is checking the "package" property for the categories.

🇪🇸Spain fjgarlin

Everything looks good and works as expected, thanks for the additional testing.

Let's clean up and prepare everything for commit. Setting to "Needs work" for the clean up. We also need to write a bit of docs on this but that can be done here or in the documentation issue.

🇪🇸Spain fjgarlin

Great!

It'd be cool to try and see what happens with the following syntax:

workflow:
  rules:
    - !reference [ .default-workflow ]

And then try:
- A merge request ===> should trigger
- A commit in the default branch ===> should not trigger on fork (it would in the canonical project)
- A pipeline triggered via the web UI ===> should trigger

🇪🇸Spain fjgarlin

The code looks good. I triggered the downstream pipelines https://git.drupalcode.org/issue/gitlab_templates-3504083/-/pipelines/41... and it looks good too.

I'd put it RTBC but we're waiting for some testing on other external projects and also testing the usage of the reference.

🇪🇸Spain fjgarlin

Thanks for that, I'm more interested in knowing if the actual emails were being sent. Also, can you try with a mix of valid and invalid emails and see what happens.

The code looks good.

🇪🇸Spain fjgarlin

Why don't we try adding to the repo a .gitlab/before_script.sh which we call under a variable switch (for example PHPCS_BEFORE_SCRIPT).

phpcs:
  before_script:
    - |
       if [[ "$PHPCS_BEFORE_SCRIPT" == "1" && -f $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/$CI_PROJECT_NAME/.gitlab/before_script.sh ]]; then
        $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/$CI_PROJECT_NAME/.gitlab/before_script.sh
      else
        echo "No before script actions."
      fi

In this case, the variables inside the script should expand accordingly and we can just turn on/off the script execution.

We could even expand on this for multiple cases with switches and scripts, but wanted to run the idea past you first to see your thoughts.

🇪🇸Spain fjgarlin

Sounds like a good idea. We'll need to test it well to figure out the syntax but definitely doable.

🇪🇸Spain fjgarlin

Thanks so much for the core issue and MR. Hopefully it's a quick one and it will go through, but if it takes longer we can always set the env variable for the nightwatch job in this part of the code: https://git.drupalcode.org/project/gitlab_templates/-/blob/main/includes...

I'd rather have that fixed in core, so leaving as postponed and the above would be a plan B.

🇪🇸Spain fjgarlin

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

🇪🇸Spain fjgarlin

instead of renaming the file (which has complications due to the symlinks and directory paths)

Which complications? Doesn't a phpcs:before_script where we rename the file solve the issue?

That eval makes me a bit nervous.

🇪🇸Spain fjgarlin

I'm happy with (a) as it'd clearly show there is a conflict with the codebase and the setup and that it needs to be fixed one way or another. Having a "web" folder means something for a project, but we can't know what that is, so we can just inform about it and offer a workaround.

There is also the argument for (b), but I think that doing so could just hide potential issues, which (a) would force to fix (either change the value of the variable for CI or rename that folder in the code).

🇪🇸Spain fjgarlin

Merged. From now on all new code will have lintin checks.
For phpstan, a baseline was created with the current issues. These can be done as follow ups as they'd need extra manual testing.

🇪🇸Spain fjgarlin

That's a great find!!

Something that we could do in the templates is to check early in the "composer" step if _WEB_ROOT folder already exists in the codebase (before we create it) and then throw a warning either:
- Asking them to change that value and exit with an error.
- Informing them (via job output) that the templates will use another value, then change it and persist it via build.env

🇪🇸Spain fjgarlin

fjgarlin changed the visibility of the branch 3503990-fix-gitlab-ci to hidden.

🇪🇸Spain fjgarlin

Rebasing / updating fork is going wrong in all branches. Closing this issue altogether and will open a new one.

🇪🇸Spain fjgarlin

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

🇪🇸Spain fjgarlin

Thanks for investigating in the other issue. Once you get to the bottom of things we may or may not need to alter things in our scripts, but regardless we can include a section or page in the documentation to talk about "distributions", or even "themes", as the templates are optimised for "projects". In this new section we can do some recommendations like "include their own phpcs.xml file with the right ignore paths", etc.

🇪🇸Spain fjgarlin

Thanks for confirming it works on this version.

We will investigate further on this, for which we might open an issue in your project to investigate and be able to trigger pipelines.

🇪🇸Spain fjgarlin

Noting that this is a distribution, not a module.

PHPStan seems to be complaining about symlinks:

  RecursiveDirectoryIterator::__construct(/builds/project/droopler/web/module  
  s/custom/droopler/web/modules/custom/droopler/web/modules/custom/droopler/w  
  eb/modules/custom/droopler/web/modules/custom/droopler/web/modules/custom/d  
  roopler/web/modules/custom/droopler/web/modules/custom/droopler/web/modules  
  /custom/droopler/web/modules/custom/droopler/web/modules/custom/droopler/we  
  b/modules/custom/droopler/web/modules/custom/droopler/web/modules/custom/dr  
  oopler/web/modules/custom/droopler/web/modules/custom/droopler/web/modules/  
  custom/droopler/web/modules/custom/droopler/web/modules/custom/droopler/web  
  /modules/custom/droopler/web/modules/custom/droopler/web/modules/custom/dro  
  opler/web/modules/custom/droopler/web/modules/custom/droopler/web/modules/c  
  ustom/droopler/web/modules/custom/droopler/web/modules/custom/droopler/web/  
  modules/custom/droopler/web/modules/custom/droopler/web/modules/custom/droo  
  pler/web/modules/custom/droopler/web/modules/custom/droopler/web/modules/cu  
  stom/droopler/web/modules/custom/droopler/web/modules/custom/droopler/web/m  
  odules/custom/droopler/web/modules/custom/droopler/web/modules/custom/droop  
  ler/web/modules/custom/droopler/web/modules/custom/droopler/web/modules/con  
  trib/search_api/tests/modules/search_api_test_excerpt_field/config/install)  
  : Failed to open directory: Too many levels of symbolic links    

And PHPCS seems to be analysing core code, which is somehow placed inside the "module" custom folder: https://git.drupalcode.org/project/droopler/-/jobs/4201053 (see how "comment" module is being looked at)

Not sure how yet, but I think 🐛 PHPCS error in contributed module caused by core recipe.README.txt Active might be a better candidate for the issue that introduced the issue.

🇪🇸Spain fjgarlin

The related task only affected projects without composer.json file.

🇪🇸Spain fjgarlin

I don't see how the related issue could cause this to be honest. This problem seems to be different.

You can easily test that if you change the version of the templates to use (https://project.pages.drupalcode.org/gitlab_templates/info/templates-ver...) in your .gitlab-ci.yml file.

Right now, we are on 1.7.1. You could change it to 1.6.14 and see if the issue happens.

So you'd need to change from ref: $_GITLAB_TEMPLATES_REF to ref: 1.6.14. You might need to set this too:

variables:
  _CURL_TEMPLATES_REF: 1.6.14
🇪🇸Spain fjgarlin

As for the composer error. We don't get Drupal 7 via composer, so the line here ( "drupal/core": "^7") fails.

We can just try it to bring external dependencies, like here on the "API" project or drupal modules, like here in the "Scheduler" project. Anything except for core D7 🙂

🇪🇸Spain fjgarlin

I think it looks cool to just have the minimal readme on main, and then each module just have the module's code itself. I don't think we need per-branch readme at all. Thanks for that.

Happy to get that daily task from GTD. Maybe we can set up the file in main to trigger the 4 branches automatically (downstream style), that way we only need to set up one daily task.

🇪🇸Spain fjgarlin

My suggestion: Downstream: Project DrupalVersion [suffix]

Example:

- Downstream: API D7
- Downstream: API D10+
- Downstream: GTD D7 Basic
- Downstream: GTD D7 Composer
...

Or maybe we want to shorten Downstream with D, or Test, or even an emoji . The idea of the same prefix is that they are all next to one another.

🇪🇸Spain fjgarlin

Maybe we can use this issue to unify somehow the names of the downstream pipelines for the way they are presented in the UI

I believe that the order of the jobs presented there is alphabetical

🇪🇸Spain fjgarlin

The changes look great and this could already even be RTBC for those two branches, but as we are creating more branches, I know that this will need extra commits.

🇪🇸Spain fjgarlin

It's a great start!

Feedback:
- "main": it doesn't probably need a ".gitlab-ci.yml" file, unless we want to trigger the branch pipelines from there (maybe a daily scheduled task). I'd make this branch the default branch, it shouldn't interfere with running pipelines as/when needed on the other branches.
- Then each branch's readme file should be about that branch/project only and explain briefly the case/s that it covers.
- Each branch's "gitlab-ci.yml" file might have specific overrides for when to run the pipeline if the default setup won't trigger then on commit.

🇪🇸Spain fjgarlin

Great to see the two projects connecting and proving that we can control multiple cases from this project.

🇪🇸Spain fjgarlin

it is useful to have one of the active branches as default to that a pipeline is run whenever a push is made.

We can tweak/override the rules on any branch so the CI runs on push for that branch.

Production build 0.71.5 2024