Automated Drupal 10 compatibility fixes

Final patch looks good!

The final patch does not look that good since the tests for D10.1 failed (see #17).
Working on this, hoping I can provide an updated patch with the tests fixed.

Tests are green again, this is ready for a review now.

Seems like we too landed at the chicken-egg problem.

In order to have the module be applicable as a patch (via cweagans/composer-patches) to released versions (e.g. 1.3.0), we need to drop any mods to composer.json or comment_notify.info.yml, regarding drupal core version requirements. This is the current state of the MR.

This latest version however does not allow running the automated tests on D.O. and is still not installable on D10 unless combined with mglaman/composer-drupal-lenient (for composer installation) and Backward Compatibility ( https://www.drupal.org/project/backward_compatibility → ) for enabling on Drupal.

The previous version of the MR that was changing the info.yml file (as well as composer.json) could be used as a patch via git, namely by requesting the composer dependency as in:
composer require drupal/comment_notify:1.3.0 --prefer-source
After that, the module was fetched using git and the MR was applicable as a patch (via git apply patch) without hiccups.
That version was allowing running tests on D.O.
Attaching that version as a patch for reference.

I have reviewed and checked Patch #22 this works for drupal 9.5 and drupal 10.0. and 10.0.1
Screenshots of the compatibility on 9.5 and the Config on 10 have been attached for reference:

@KlemenDEV could you describe your review as you set to RTBC?

I've done a brief review and am inclined to commit and fix things in followups, but would be good to confirm what QA steps have been done.

@Stefanos Petrakis thanks for the work on the MR and patch! If you are interested in becoming a co-maintainer that could be a good idea :)

Hi @greggles; thanks for the kind words. I could imagine co-maintaining, sure!

Most changes look good to me. I have 2 remarks about the patch:

1. +++ b/tests/src/Functional/CommentNotifyNotificationsTest.php
   @@ -18,12 +18,23 @@ class CommentNotifyNotificationsTest extends CommentNotifyTestBase {
   +  /**
   +   * Exempt from strict schema checking.
   +   *
   +   * This is due to saving configuration with a missing schema for terms.
   +   *
   +   * @see \Drupal\Core\Config\Development\ConfigSchemaChecker
   +   *
   +   * @var bool
   +   */
   +  protected $strictConfigSchema = FALSE;
   

   This change looks wrong to me. Lets just add the config schema.

2. +++ b/comment_notify.info.yml
   @@ -6,4 +6,4 @@ dependencies:
   -core_version_requirement: ^8 || ^9
   +core_version_requirement: ">=8"
   
   +++ b/composer.json
   @@ -23,7 +23,6 @@
   -        "drupal/core": "^8 || ^9",
   

   I am not happy with this change. It is now implying that this version will work with all future version of Drupal core. Only we have now idea what changes will be in future versions of Drupal core.

@KlemenDEV could you describe your review as you set to RTBC?

I have marked it based on the 10.0.1 testing of @yashingole and I myself patched the module on the 9.5.3 website, verified configuration (site-wide and user form) still worked, and left some comments (two different comment types), and made sure the email was sent properly. Also, no errors or warnings were logged by the PHP (8.1.11).

Back to NR after responding to @daffie 's comment in #27 (thanks for the extra eyes!)

It looks good to me.
All my remarks have been addressed.

@KlemenDEV thanks for comment in #28. I appreciate that extra detail.

@stefanos.petrakis and @daffie thanks for the extra review and changes! I've now merged it so I think this issue can be marked as fixed.

I filed 📌 New release, new co-maintainer Fixed about new comaintainer and new release. Thanks!

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