Consider using contrib instead of custom for module path

It is my understanding 'contrib' is commonly used for modules that are downloaded from D.O while 'custom' is used when they are "local" modules not distributed on D.O.

I would opine that the GitlabCi template aligns with this as the code being tested is not guaranteed to be in the official repo and as such is closer to a 'custom' module.

Note: this would probably need to wait until a 2.x branch if it is accepted as it will change path locations impacting those who implement and extend off the templates.

I would opine that the GitlabCi template aligns with this as the code being tested is not guaranteed to be in the official repo and as such is closer to a 'custom' module.

I don't quite follow here -- Drupalci on gitlab is specifically for testing code being hosted on Drupal.org. While the specific MR under test may not be guaranteed to be committed, the module/theme/etc which is receiving the MR is, by definition, not a custom module.

Modules/themes/etc hosted on Drupal.org expect to be installed in the contrib folder, not the custom folder, and thus the test system is not performing an accurate test. Modules that need to differentiate between contrib and custom modules will fail tests when using gitlabci.

As for a 2.x version -- While thats technically correct (due to .composer-base changing), I haven't ran into a case where paths are being manipulated after a composer build. Certainly possible though. However, this issue is currently breaking our tests, so we are depending on this fork/ref until its adopted.

the module/theme/etc which is receiving the MR is, by definition, not a custom module.

In general I’m contending once you patch/modify/change the code of the official module it can be perceived as a custom module (an uncommitted MR is custom code).

End of the day it does not make a huge difference which of the paths we test under, just giving some arguments from my mind of why custom makes sense and perhaps why it was never questioned when it was implemented.

I’m not against this as long as it waits to a 2.x branch of the templates, I just equally don’t see the value as it sounds like more like brittle tests than a true fault of the Ci environment.

Modules/themes/etc hosted on Drupal.org expect to be installed in the contrib folder, not the custom folder, and thus the test system is not performing an accurate test.

These deployment paths are to my knowledge just convention and are not mandatory by core correct? My recollection is the parser looks recursively for all *.info.yml files in modules and that you could find your module in any path correct?

Technically the module is not even installed in the custom folder with the current template, it is in the GitLab build root directory, and is symlinked from the custom folder (I’m not a fan of this since Drupal does not officially support this and symlinks have caused me issues with pcov)

I was discussing the other day in Slack how we may want to populate DRUPAL_ROOT as part of UnitTestBase, and I imagine a VENDOR_ROOT too to make these tests a bit more position agnostic.

haven't ran into a case where paths are being manipulated after a composer build.

I run several projects that generate code coverage reports and part of this is normalizing the paths so they render in the Gitlab UI. This change would break that normalization. In fairness I could have been more robust in the sed globs I used to support more robust path locations, however it was not foreseen at the time since this was job specced.

I agree with many of the points from both sides.

In fact, we could technically change the paths here, as long as it's in inside the "modules" folder:

        "installer-paths": {
            "web/core": [
                "type:drupal-core"
            ],
            "web/modules/contrib/{$name}": [
                "type:drupal-module"
            ],

From the "/modules/README.txt":

ORGANIZING MODULES IN THIS DIRECTORY
------------------------------------

You may create subdirectories in this directory, to organize your added modules,
without breaking the site. Some common subdirectories include "contrib" for
contributed modules, and "custom" for custom modules. Note that if you move a
module to a subdirectory after it has been enabled, you may need to clear the
Drupal cache so it can be found.

We wouldn't even need to have "contrib" or "custom". So I'd really say that this is kind of working as designed and it's more of a feature request.
I think a module should work regardless of whether it's inside a "contrib" or "custom" folder, as Drupal has some methods to get the path of the module.

In any case, something that could be done is just to have the path to the "modules" under a variable. For D9+ it'd be "modules/custom" and for D7 "sites/all/modules/custom" as default values. This would ensure that the behavior does NOT change at all compared to what we currently have, but it will also allow maintainers to change it if they desire. I think this kind of feature could be accepted within the current version and won't need to wait for a 2.x scenario.

We wouldn't even need to have "contrib" or "custom". So I'd really say that this is kind of working as designed and it's more of a feature request.
I think a module should work regardless of whether it's inside a "contrib" or "custom" folder, as Drupal has some methods to get the path of the module.

While technically true, the recommended/supported way is for community modules to exist in contrib and site specific modules to exist in custom. Many templates, including the official drupal recommended project organize things this way, so I don't think our tests should be deviating from that.

The reason why custom vs contrib is important is that you may use .gitignore on the 'contrib' folder, and in our case, differentiate between 'community' and 'site specific' code when running automated scans. If we cannot use the standard setup, our tests fail.

In general I’m contending once you patch/modify/change the code of the official module it can be perceived as a custom module (an uncommitted MR is custom code).

Thats somewhat antithetical to the whole concept of contrib testing. The whole goal is for an MR to become part of the official code, and should be tested as such. Otherwise, why make the issue in the first place? It also isn't true--the official and recommended way modules are to be installed is via composer, which uses the scaffold, which recommends using contrib. If you patch a contrib module it will still be in the contrib folder because its being updated by composer, not being committed directly to the site's folder, as you'd have with custom. While its not released yet, core will be making contrib essentially a requirement as part of composer ready templates.

That all said, I'd be all for making the module path a variable so the 1.x version can remain the way it is while 2.x has this fixed.

Added the variable. Note, its just the custom/contrib switch because themes, profiles, and modules should be observing this pattern, and having a matrix of these variables for D7 and D8+ doesn't make much sense.

There are a few places where the variable is still not used and "contrib" is hardcoded. I think I left a comment in all the instances so I can review once changed.

Went down a rabbit hole trying to fix the themes/profiles symlink mess. My latest update to the MR puts these in the correct place, but some of the scripts still need updating.

There is now question as to whether this is a breaking change or not -- themes and profiles are symlinked as modules, which is not intentional and likely cause issues for these types of projects. Using the built in composer symlink functionality and moving the project under test to its own folder fixes some of the oddities the existing build system has, including its bias towards modules.

themes and profiles are symlinked as modules, which is not intentional and likely cause issues for these types of projects.

Wow.... that sends this issue down a rabbit hole. It seems like some work needs to be done.

There is now question as to whether this is a breaking change or not

As it relates to the overall template changes of massively rewriting the basic composer.json:
In my opinion yes it is a a BC break.

As it relates soley to making sure Themes(and profiles) under test are not installed in the modules folder:
I know we say themes "must" be in the Themes folder, however I have never tested if the auto loader would actually work if a Theme was not in the the Theme directory. If not the argument becomes a lot stronger for claiming a bugfix, though I do not think we could rule it out as a non-breaking change for modules that do no run PHPUnit tests either.

I keep saying we should start on a 2.x branch plans... Maybe as this relates to themes/profiles is what pushes us to do so.

including its bias towards modules.

Looking back at #3261803: Using GitLab CI instead of Drupal CI → at least one Theme enrolled, though it doesn't appear to be running any sort of PHPUnit tests on itself and it never updated to use the templates when we converted away from Spoons.
https://git.drupalcode.org/project/gin/-/blob/8.x-3.x/.gitlab-ci.yml?ref...

Looks like we missed a chance to catch some of this early on.

Yeah, themes / profiles can benefit from all the linting steps, but not tests in this case.

themes and profiles are symlinked as modules

Maybe if we don't touch anything regarding that it will be less of a rabbit hole. let's only address here the "contrib" vs "custom" change and not change anything else. we can have a follow-up issue just for themes and profiles and decide on that other issue whether it can be achieved without breaking changes.

I think I found a happy medium. By adding 'modules' to the DRUPAL_PROJECT_PATH, it allows the system to default to modules, but provides themes and profiles the ability to change the variable to what works for them. In a perfect world I still think moving the project under test to its own folder and using composer is better, but that should be moved to another issue (despite me originally arguing the opposite).

Hopefully this latest round of changes is more acceptable. It allows for BC, it also fixes problems for themes/profiles, and allows modules to move into the more 'standard' location expected during tests.

This is looking a lot more like what I envisioned in #6 and #12.

I made some minor comments/suggestions in the MR and a point to test the `sed` command, as I think that it will not work as the "/" character might need escaping (there is another version of this logic in the phpstan-base job which does not do the escaping, so maybe we want to try that). I triggered the D7 and 3xD10 downstream pipelines here https://git.drupalcode.org/issue/gitlab_templates-3460115/-/pipelines/22...

I haven't checked the results of the pipeline in detail but wanted to run to have extra information.

It'd be great to test it in another project (https://project.pages.drupalcode.org/gitlab_templates/info/testing-mrs/), with "upgrade status" enabled as I think it's not in any of the downstream pipelines.

Setting to NW based on the small feedback and the above comments but very happy with the progress here.