Drupal 11 compatibility

@adamps, are you sure that you want to drop supporting for d9? This decision looks more like a major change and then it will be necessary to support 3.x branch (port patches from 4.x to 3.x) and if you look at the statistics, 3.x is needed for 6k users.

A very strange story with the module drupal/monitoring. The module is specified as a dev dependency, but there are places in the code (plugins, tests) that need this module, which makes it mandatory to use.

We need to decide what to do with this module.

If we specify it as a mandatory dependency, we will be blocked by 📌 Automated Drupal 11 compatibility fixes for monitoring Needs work and we will need to wait until it becomes compatible with the d11.

If we leave it as a dev dependency, we will need to remove all places related to it.

Plugins and tests aren't mandatory. Dev dependencies specifically exist to run tests.

You can use lenient and sed tricks to run CI, see paragraphs for example

Dropping D9 is also not a major change and can be done in a minor release. D9 is long EOL

@berdir,

Plugins and tests aren't mandatory. Dev dependencies specifically exist to run tests.

Regarding the use of dev dependencies fo autotests, I can agree, since tests are not launched on production, but regarding plugins - I don't agree.
The module provides a plugin for integration with drupal/monitoring and does not specify the drupal/monitoring as an explicit and mandatory dependency. Running composer install with --dev on prod is bad practice.

A more correct solution here would be to move this integration to a submodule and specify an explicit dependency on the module for the submodule.

Dropping D9 is also not a major change and can be done in a minor release. D9 is long EOL

This is about the 4.x branch, not the core. If we drop supporting d9 for the 4.x branch, then only 3.x branch will be compatible with d9, but based on description on the main page of the module, 3.x will not be supported anymore - It is not receiving new fixes.

Now back to 4.x. Considering that support for Drupal 9 is currently declared, many users may be using the 4.x version on Drupal 9, and after dropping support in a minor release, users will face the problem that they will not be able to upgrade to the new minor version due to core and code incompatibility.

Now let's see what semver (https://semver.org/) tells us.

Given a version number MAJOR.MINOR.PATCH, increment the:

...
MINOR version when you add functionality in a backward compatible manner (
...

As a result, we get that any minor release within a major version guarantees compatibility, otherwise it is a major change.
Of course, we can do what we think is right for the contrib module, but that does not mean it is a good approach.

In any case, these are just recommendations, let the maintainer decide what solution he wants to see.

Plugin classes are like hooks, if they are not called/discovered, their code doesn't run. They absolutely are an optional dependency. See for example https://git.drupalcode.org/project/drupal/-/tree/11.x/core/modules/node/..., node.module doesn't depend on search or migrate or views.

I don't really understand your argument on D9, as you said, it is valid to increase the required version in a minor release, so 4.1.0 can require D10. D9 is EOL and unsupported.

And while I mostly leave it to @AdamPS to maintain this project, I am also a maintainer.

@berdir, the example with nodes is not appropriate here, since the modules with which the node has integrations are part of the core and are installed explicitly when the core is installed.

I'm just saying that it's bad practice to do this in contrib modules and with semver versions. If you as a maintainer don't see anything wrong with this, then none of this matters.

I'm returning it back to the status "needs work" because the tests are failing - https://git.drupalcode.org/issue/simplenews-3477438/-/jobs/2896810

> @berdir, the example with nodes is not appropriate here, since the modules with which the node has integrations are part of the core and are installed explicitly when the core is installed.

There are two different kinds of "install". One is the files being present with composer install, the projects/packages. But that has little impact on runtime. Drupal will only load files of modules that are actually installed/enabled.

Whether or not search in this case is a core module is not relevant from a functional perspective. A plugin class of a plugin type that isn't being discovered will never be loaded. It's perfectly safe to do this and IMHO preferred over submodules because every installed module adds overhead at least on cold cache scenarios/rebuilds.

If you want contrib examples, then there is entity_reference_revisions with a diff plugin.

The plugin system even allows for a scenario where for example monitoring provides a sensor plugin for another module that is optional, by specifying the provider explicitly. There are some edge cases here that don't work or are less reliable, for example when said plugin uses a base class/interface or trait of the optional module.

+1 to adding monitoring as a require-dev dependency as you did. The reason this isn't done yet is that it's added as a test_dependency in simplenews.info.yml, which is kind of the same thing and how it was done in earlier iterations of DrupalCI. It's no longer recommended and I'd suggest to remove that then.

Thanks @davps for working on this issue, as it's clearly important.

Thanks @berdir, I agree. In summary:

1. This module works with monitoring, it does not mandate monitoring. I guess that most sites using the module won't want to be forced to install monitoring, but also plenty of them do use it (and they will not run composer with --dev on, they will explicitly require monitoring separately). No problem, both groups are happy.
2. We only support Core versions that are supported. Maintainer time is very limited, barely enough to keep the project updated. If an old site is stuck on D9 which is not receiving fixes then what difference if also stuck on simplenews 4.0?

Strange the tests are failing, as I thought you had just fixed them???

Actually the tests failed 3 days ago but passed 1 day ago, and this issue now has a green tick, so back to needs review

Great thanks I made some simple suggestions in the MR

📌 Automated Drupal 11 compatibility fixes for monitoring Needs work is now merged and the workarounds can be removed.

Great thanks @berdir

Error encountered when subscribing:

Fatal error: Declaration of Drupal\simplenews\Plugin\Validation\Constraint\SubscriberUniqueFieldConstraint::validatedBy() must be compatible with Drupal\Core\Validation\Plugin\Validation\Constraint\UniqueFieldConstraint::validatedBy(): string in /opt/drupal/web/modules/contrib/simplenews/src/Plugin/Validation/Constraint/SubscriberUniqueFieldConstraint.php on line 20

Thanks @joseminosa this needs to be added to the MR

Automatically closed - issue fixed for 2 weeks with no activity.