Drupal 9 compatibility related issues for Security Kit module

Seems we already had issue for this - https://www.drupal.org/project/seckit/issues/3042610

1. +++ b/src/Tests/SecKitTestCaseTest.php
   @@ -385,8 +385,8 @@ class SecKitTestCaseTest extends WebTestBase {
   +    $this->drupalGet('admin/config/system/seckit', [], ['Origin: ' . $base_root]);
   

   This seems not correct as drupalPostForm() submits the form while drupalGet() takes to that page.

2. +++ b/src/Tests/SecKitTestCaseTest.php
   @@ -400,8 +400,8 @@ class SecKitTestCaseTest extends WebTestBase {
   +    $this->drupalGet('admin/config/system/seckit', [], ['Origin: http://www.example.com']);
   

   Same here.

See more details in https://www.drupal.org/node/3030340

1. index d84a53a..d425d24 100644
   --- a/src/Tests/SecKitTestCaseTest.php
   
   --- a/src/Tests/SecKitTestCaseTest.php
   +++ b/src/Tests/SecKitTestCaseTest.php
   

   The test has to be moved to the /tests/src/Functional/SecKitTestCaseTest.php location

2. +++ b/src/Tests/SecKitTestCaseTest.php
   @@ -3,14 +3,14 @@
    namespace Drupal\seckit\Tests;
   

   Namespace also need to be changed

Per a Slack discussion with Gábor Hojtsy regarding usage of D9 tags (Drupal 9, Drupal 9 compatibility, Drupal 9 readiness, etc.), "Drupal 9 compatibility" should be used for contributed projects that need updating and "Drupal 9" was the old tag for D8 issues before the D9 branch was ready. Doing title and tag cleanup here based on that discussion.

Addressing #8 and #6 comments. attaching a new patch moving the file to test directory.

Blocked by #3081327: drupalPostForm lacks headers

There's no need to wait for a fix to core - also maybe a fix to core is not really necessary as there is a way to add headers to any request made by the test system. Here's a patch that converts the test

Should be green.

Yeah right @alexpott... third time's a charm.

Yay! Only 39 cs bugs left...

Well most of those exist in HEAD too... but I guess we can fix them here too. This should help... let's see where we are after this.

Clean-up leftovers

Getting below issue for this patch when running drupal-check on latest version from GIT

7/7 [============================] 100%

------ -------------------------------------------------------------------------
Line tests\src\Functional\SecKitTestCaseTest.php
------ -------------------------------------------------------------------------
Class PHPUnit\Framework\TestCase not found and could not be autoloaded.
------ -------------------------------------------------------------------------

[ERROR] Found 1 error

I believe that if you add drupal 9 as an option to the composer.json file in this project you can get a nice little Compatible with Drupal 9 badge in the Project information section on the module page.

Below is a code sample from a module that has the badge.

"require": {
        "php": "^7.1",
       "drupal/core": "^8 || ^9"
  },

It also looks like it may possible to get the badge by adding the 'core_version_requirement' key in the modules info.yml file, which, in turn, will add the version to the require section of composer.json. Example follows.

core_version_requirement: ^8 || ^9

Do you want to include this change in the patch here or open a separate issue?

Added core version in patch.

Patch #23 worked for me. It would be great if we can have it merged so we're able to use/test it on D9 projects :)

The composer.json file is not required in order to get the badge, see the relevant change notice for details.

FYI to add the D9 support badge you have to edit the project page and fill in the "Drupal 9 porting info" field; per Gábor, the intended automation around the D9 compatibility hasn't been completed, so we need to edit these fields for now.

Patch looks good, and since the composer.json change is not strictly necessary, I believe this is ready to go.

Patch does not apply with composer patches

Hmm, patch did apply but not in composer...
Re-uploading to see if it works. I don't need any credits.

It applies on latest dev and everything is green. So I can confirm this one is good to go.

Pushed this to 8.x-1.x initially; I understand we may want to create a new branch but this seemed to include some fixes/cleanup that are good to have in the existing branch too.

Fixed one typo in a comment on commit s/is/if.

Thanks everyone!

Ah, I'm not actually certain we want this in 8.x-1.x with:

-core: 8.x
+core_version_requirement: ^8.8 || ^9

...as that means 8.7 is out of luck, IIUC.

I will try to look at fixing that; probably with a new branch.

All major modules are doing this, so it's fine.

@mcdruid
Can you cut a new D9 supported release?

I made an RC1 yesterday on a new semver branch:

https://www.drupal.org/project/seckit/releases/2.0.0-rc1

I am still getting my head around how the new branches etc.. are supposed to work though, so please let me know if I've missed anything.

Thanks @mcdruid, I can confirm that the latest release of the Security Kit module can be added with composer require drupal/seckit:^2.0 and that it also passes the Upgrade Status Drupal 9 compatibility check, without errors and only two warnings:

Upgrade Status report

Contributed projects

Security Kit 2.0.0-rc1

Check manually

Thanks @ressa, however are those messages about PHPUnit a false positive?

See, for example: https://www.drupal.org/project/upgrade_status/issues/3137754

If it is an actual problem with the module, I'm not sure I understand what needs to be fixed.

I believe I didn't have drupal/core-dev installed at that point, @mcdruid. But since they are warnings, they are perhaps not that important, at least from a Upgrade Status point of view? I suppose you don't see the warnings, perhaps because you have PHPUnit installed?