Create a composer.json file

i'll do the review

I tried to do the composer install and it returns me a problem:

- Root composer.json requires drupal/monitoring, it could not be found in any version, there may be a typo in the package name.

"repositories" is missing that's the reason of the problem.

Edit: Actually, there is no need of that "repositories", I was running composer install in a wrong way. The composer.json file seems to be similar to files from another modules. So, if the tests here passed, I think it all right.

Composer.json is only needed if you have dependencies in external libraries. In particular, it is NOT needed just for a test_dependency on another Drupal module.

The message tells you what you need to do - it doesn't say add a composer. json

It's not wrong to add one, but then you have to maintain it and keep it consistent with the .info.yml

The version in the MR is a little excessive, and not really consistent with the rest of contrib.

I think TR is right - we don't need a composer.json, and it would create work for the future.

See review above.

I agree with #10 that we should have a composer.json for the module otherwise the tests will fail with MR approach every issue which is bad for people that are contributing because they will think the tests are failing because of their changes.

As the dependency on the monitoring module is necessary for tests only, we can add the dependency in the composer.json file as a dev dependency.

IMO @marcos_lima can close MR !9 and keeping working on branch 3247780-create-a-composer.json.

Looks good.

Thanks for the explanations. So if I understand correctly then any MR will fail without this patch? In which case, please can someone make a MR that fails to demonstrate the problem? I guess you can just add a space to an existing file or something.

OK so there is an example failure at https://www.drupal.org/pift-ci-job/2231832. It reports the following error:

PHPUnit\Framework\Exception: Unavailable module: 'monitoring'. If this module needs to be downloaded separately, annotate the test class with '@requires module monitoring'.

Have we definitely tried that? Is this error message outdated then, and not applicable to using a MR?

@AdamPS the MR for #3087462: \Drupal calls should be avoided in classes, use dependency injection instead is failing because of this error.

As to the "@requires module" in the test, while a good idea it is somewhat outside the scope of this issue. Note that the documentation for requires module specifies that "if your test requires additional contributed modules to be installed, beyond the module's direct dependencies, see the Managing dependencies Composer page for instructions."

Also see:
https://www.drupal.org/docs/develop/using-composer/managing-dependencies...
https://www.drupal.org/docs/creating-custom-modules/let-drupal-know-abou...

The proposed composer.json works perfectly well from what I can see, so RTBC++. Please merge this so that maybe the tests in #3087462: \Drupal calls should be avoided in classes, use dependency injection instead stop failing, as they currently fail on the lack of the monitoring module.

Thanks @jcnventura

There is a core issue for @requires #2728579: Explicitly skip @requires module in PHPUnit Kernel and Browser tests.

I agree the proposed solution works. I am trying to confirm that it is necessary to create a composer.json in order to solve this issue. The links you gave were very useful because they suggest another possibility: we could instead add a test_dependencies: line to the info.yml. Seeing as we don't yet have a composer.json, this feels like a neater option to me.

It is not. test_dependencies is annoying to use, most importantly, it needs to be committed first, you can not change it in merge requests/patches. And I don't think it works in merge requests at all.

This is the right thing to do, and without things that can become outdated (like authors before), I don't think this causes meaningful maintenance overhead. Almost all modules have this now. And drupal core is discussing to move at least some information of the .info.yml files into composer.json, reducing the duplicating in that direction.

And the requires core issue is just about not running that test then, we do want to run it.

OK thanks @Berdir. It seems like there is some outdated documentation with recommendations that no longer apply, so I'm happy that you can clarify.

Please can someone explain something to me about the MR as I haven't used them. I would like to squash all the commits in the issue-branch into a single commit with the "standard" message - as if I had selected the git command from the bottom of the page. Is there a way to do that please?

Use the UI here in the issue, where you can select the MR you want to merge. That will do a squash merge and commit it with the recommended commit message.

To reproduce that manually, use git merge --squash.

RTBC++. Regarding the test_dependencies line in the info.yml referenced in #20, it is already there: https://git.drupalcode.org/project/simplenews/-/blob/3.x/simplenews.info...

Thanks everyone for the advice, sorry I'm behind the times😃.