Drupal 9 compatibility

D9 compatibility was on my to-do list, but of course I am happy to have some help. I will review this.

We can fix the test failures as part of #3124616: Fix coding standards in Typogrify.

@Wim Leers, thanks again for working on this issue.

I reviewed the change to the .info.yml file. LGTM

I tested with Drupal 9.0.0-beta2 and the Umami installation profile. Without the patch from #2, I get errors from various drush commands: si, cr, en typogrify. I also get errors when visiting some admin pages: /en/admin/modules and /en/admin/config/development/performance.

After applying the patch, those errors go away and I can enable the module. I did a basic test of functionality: add the Typogrify filter to the Basic HTML format and create a node with "Basic HTML" in a text field. As expected, I get the markup Basic <span class="caps">HTML</span>.

I am setting the status to RTBC, but I will not fix this issue (commit the patch) yet because then the dev version will not work with Drupal 8.7, which is still supported. I will release another alpha version today, and I plan to make a full release very soon. I will target this issue for 8.x-1.1.

I installed Upgrade Status and checked the Typogrify module. There were three warnings. I will deal with one on #3124616: Fix coding standards in Typogrify; one is already fixed by this patch; but the third should also be fixed here:

A drupal/core requirement is not present in composer.json. Having a composer.json is not a requirement for Drupal 9 compatibility but if there is one, it should include a drupal/core requirement.

I am setting the status back to NW.

BTW, even if the patch is easy, recognizing the need, creating the issue, and so on are all work that needs to be done. It really is appreciated!

Add in composer.json has well

Thanks for helping out!

+++ b/composer.json
@@ -17,5 +17,7 @@
...
+    "require": {
+      "drupal/core": "^8 || ^9"
+    }
...
+++ b/typogrify.info.yml
@@ -2,7 +2,7 @@ name: Typogrify
...
-core: 8.x
+core_version_requirement: ^8.8 || ^9

Let's use the same version requirement in both places. But first, I looked into the composer.json requirement some more.

We do not need to update composer.json

After some discussion on the #d9readiness channel in Slack, we agreed that the Upgrade Status module is wrong in generating that warning, and I added #3126949: Missing requirement in composer.json should not generate a warning.

Is it still a good idea?

Argument in favor:

• Drupal core and contributed modules should try to follow Composer standards.

Arguments against:

1. DRY
2. It seems unlikely that anyone will composer require drupal/typogrify outside the context of a Drupal site.
3. If someone really does want to do that, should we stop them? What if someone makes a fork of Drupal, just as Backdrop is a fork of Drupal 7?
4. There is no mention of requiring drupal/core in the guide Add a composer.json file.

Should we remove core: 8.x from the .info.yml file?

In my initial test with Drupal 9.0.0-beta2 (see #4) it seemed that we need to remove the core key . But today I re-tested, and I was able to install 9.0.0-beta2 from scratch or upgrade from 8.9.x, so maybe something else was going in in my initial test. If we keep core: 8.x and use the version constraint from #7, then I will be able to commit this patch before releasing 8.1.0.

We have to remove core key if we specify drupal 8.8

We have to remove core key if we specify drupal 8.8

Why is that? According to https://www.drupal.org/docs/8/creating-custom-modules/let-drupal-8-know-... and the change record https://www.drupal.org/node/3070687, it is the other way around: if we remove core: 8.x, then we have to specify ^8.8 (or maybe just ^8.7.7).

I tested with 8.9.x and 9.0.0-beta2 without removing the core key, and I was able to install the module. Should I re-test?

Yes please, because its not best practice to have both in info.yml if you specify 8.8

In my initial tests (see #4) I had core_version_requirement: ^8.8 || ^9, and in my re-test (see #8) I had core_version_requirement: ^8 || ^9. I tried again with the original version constraint ( the one in the patch from #2) and got the error message

The 'core_version_requirement' constraint (^8.8 || ^9) requires the 'core' key not be set in modules/contrib/typogrify/typogrify.info.yml

This is consistent with #11. Looking in core/lib/Drupal/Core/Extension/InfoParserDynamic.php, I see that this comes from the lines

        // If the 'core_version_requirement' constraint does not satisfy any
        // Drupal 8 versions before 8.7.7 then 'core' cannot be set or it will
        // effectively support all versions of Drupal 8 because
        // 'core_version_requirement' will be ignored in previous versions.
        if (!$supports_pre_core_version_requirement_version && isset($parsed_info['core'])) {
          throw new InfoParserException("The 'core_version_requirement' constraint ({$parsed_info['core_version_requirement']}) requires the 'core' key not be set in " . $filename);
        }

I guess that means we have two options:

1. Keep core: 8.x and use a version constraint like ^8 || ^9.
2. Remove core: 8.x and use a version constraint like ^8.7.7 || ^9 or ^8.8 || ^9.

In #8, I suggested Option (1). Are you saying that you prefer Option (2)?

I will update https://www.drupal.org/docs/8/creating-custom-modules/let-drupal-8-know-....

No, I will not edit that page. It already says,

The core: key must not be used here to make sure that versions of Drupal before 8.7.7 will not install the module. Adding both core and core_version_requirement with anything other than core_version_requirement: ^8 || ^9 will result in an exception.

Here is a patch that implements Option 1 from #12. I am not going to bother with an interdiff.

Thanks all for working on this!

It is funny how a 1-line patch generated so much discussion. The bigger change is oon the Upgrade Status module: #3126949: Missing requirement in composer.json should not generate a warning. I hope that saves other maintainers from going through the same discussion (or at least part of it).