Prepare the module for Drupal 9

I am going to work on it.Assigning to myself.

Thanks for starting this, the composer.json should also get the drupal/core dependency with the same definition.

There also still lots of deprecated code usages, ->url() calls, Unicode:: method calls, drupal_set_message(), simpletest tests and so on.

Tagging for ContributionWeekend2020 as this may be a good candidate for making progress this weekend.

Adding Compatibility patch only for info and composer file !

Hi,
The patch is working fine. Please review.

Patch looks good to me. Moving to RTBC.
Thanks

I'm still seeing a few deprecation issues here:

$ bin/drupal-check -d docroot/modules/contrib/captcha
 32/32 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ --------------------------------------------------------------------------------------- 
  Line   src/Tests/CaptchaSessionReuseAttackTestCase.php                                        
 ------ --------------------------------------------------------------------------------------- 
  16     Class Drupal\captcha\Tests\CaptchaSessionReuseAttackTestCase extends deprecated class  
         Drupal\simpletest\WebTestBase:                                                         
         in drupal:8.8.0 and is removed from drupal:9.0.0. Instead,                             
         use \Drupal\Tests\BrowserTestBase. See https://www.drupal.org/node/3030340.            
  70     Call to method setUp() of deprecated class Drupal\simpletest\WebTestBase:              
         in drupal:8.8.0 and is removed from drupal:9.0.0. Instead,                             
         use \Drupal\Tests\BrowserTestBase. See https://www.drupal.org/node/3030340.            
 ------ --------------------------------------------------------------------------------------- 

Hi Folks, Removed the deprecated usage in captcha module test. Please review it.

Test failures need to be resolved:

1) Drupal\captcha\Tests\CaptchaSessionReuseAttackTestCase::testCaptchaSessionReuseAttackDetectionOnCommentPreview
Error: Cannot use object of type Behat\Mink\Element\NodeElement as array

/var/www/html/modules/contrib/captcha/src/Tests/CaptchaSessionReuseAttackTestCase.php:189
/var/www/html/modules/contrib/captcha/src/Tests/CaptchaSessionReuseAttackTestCase.php:289

2) Drupal\captcha\Tests\CaptchaSessionReuseAttackTestCase::testCaptchaSessionReuseAttackDetectionOnNodeForm
Error: Cannot use object of type Behat\Mink\Element\NodeElement as array

/var/www/html/modules/contrib/captcha/src/Tests/CaptchaSessionReuseAttackTestCase.php:189
/var/www/html/modules/contrib/captcha/src/Tests/CaptchaSessionReuseAttackTestCase.php:331

3) Drupal\captcha\Tests\CaptchaSessionReuseAttackTestCase::testCaptchaSessionReuseAttackDetectionOnLoginForm
Error: Cannot use object of type Behat\Mink\Element\NodeElement as array

/var/www/html/modules/contrib/captcha/src/Tests/CaptchaSessionReuseAttackTestCase.php:189
/var/www/html/modules/contrib/captcha/src/Tests/CaptchaSessionReuseAttackTestCase.php:372

Moved CaptchaSessionReuseAttackTestCase to the tests directory and fixed failing tests.

Reviewed and tested.

Running the tests still result in a couple of deprecation notices:

Remaining deprecation notices (70)

  46x: Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was captcha_pre_render_process. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725
    8x in CaptchaTest::testCaseInsensitiveValidation from Drupal\Tests\captcha\Functional
    6x in CaptchaCacheTest::testCacheTags from Drupal\Tests\captcha\Functional
    6x in CaptchaPersistenceTest::testPersistencePerFormInstance from Drupal\Tests\captcha\Functional
    5x in CaptchaPersistenceTest::testPersistenceAlways from Drupal\Tests\captcha\Functional
    5x in CaptchaPersistenceTest::testPersistencePerFormType from Drupal\Tests\captcha\Functional
    4x in CaptchaPersistenceTest::testPersistenceOnlyOnce from Drupal\Tests\captcha\Functional
    3x in CaptchaAdminTest::testUntrustedUserPosting from Drupal\Tests\captcha\Functional
    3x in CaptchaTest::testCaptchaOnLoginForm from Drupal\Tests\captcha\Functional
    2x in CaptchaTest::testCaptchaOnLoginBlockOnAdminPagesIssue893810 from Drupal\Tests\captcha\Functional
    1x in CaptchaAdminTest::testCaptchaPlacementCacheClearing from Drupal\Tests\captcha\Functional
    1x in CaptchaTest::testCaptchaDescriptionAfterCommentPreview from Drupal\Tests\captcha\Functional
    1x in CaptchaTest::testCaptchaSessionReuseOnNodeForms from Drupal\Tests\captcha\Functional
    1x in CaptchaTest::testLongFormId from Drupal\Tests\captcha\Functional

  20x: Drupal\Tests\BrowserTestBase::$defaultTheme is required in drupal:9.0.0 when using an install profile that does not set a default theme. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.
    1x in CaptchaPersistenceTest::testPersistenceAlways from Drupal\Tests\captcha\Functional
    1x in CaptchaTest::testCaptchaOnLoginBlockOnAdminPagesIssue893810 from Drupal\Tests\captcha\Functional
    1x in CaptchaTest::testCaptchaSessionReuseOnNodeForms from Drupal\Tests\captcha\Functional
    1x in CaptchaTest::testCaptchaDescriptionAfterCommentPreview from Drupal\Tests\captcha\Functional
    1x in CaptchaTest::testCaseInsensitiveValidation from Drupal\Tests\captcha\Functional
    1x in CaptchaTest::testCaptchaOnLoginForm from Drupal\Tests\captcha\Functional
    1x in CaptchaPersistenceTest::testPersistenceOnlyOnce from Drupal\Tests\captcha\Functional
    1x in CaptchaPersistenceTest::testPersistencePerFormType from Drupal\Tests\captcha\Functional
    1x in CaptchaPersistenceTest::testPersistencePerFormInstance from Drupal\Tests\captcha\Functional
    1x in CaptchaCronTest::testCron from Drupal\Tests\captcha\Functional
    1x in CaptchaAdminTest::testAdminAccess from Drupal\Tests\captcha\Functional
    1x in CaptchaCacheTest::testCacheTags from Drupal\Tests\captcha\Functional
    1x in CaptchaAdminTest::testCaptchaPointAdministrationByNonAdmin from Drupal\Tests\captcha\Functional
    1x in CaptchaAdminTest::testCaptchaPointAdministration from Drupal\Tests\captcha\Functional
    1x in CaptchaAdminTest::testCaptchaPlacementCacheClearing from Drupal\Tests\captcha\Functional
    1x in CaptchaAdminTest::testXssOnCaptchaDescription from Drupal\Tests\captcha\Functional
    1x in CaptchaAdminTest::testUntrustedUserPosting from Drupal\Tests\captcha\Functional
    1x in CaptchaAdminTest::testCaptchaAdminLinks from Drupal\Tests\captcha\Functional
    1x in CaptchaAdminTest::testCaptchaPointSettingGetterAndSetter from Drupal\Tests\captcha\Functional
    1x in CaptchaTest::testLongFormId from Drupal\Tests\captcha\Functional

  4x: Support for asserting against non-boolean values in ::assertFalse is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use a different assert method, for example, ::assertEmpty(). See https://www.drupal.org/node/3082086
    3x in CaptchaCacheTest::testCacheTags from Drupal\Tests\captcha\Functional
    1x in CaptchaAdminTest::testCaptchaPointAdministration from Drupal\Tests\captcha\Functional

1. +++ b/captcha.info.yml
   @@ -3,6 +3,7 @@ type: module
    core: 8.x
   +core_version_requirement: ^8 || ^9
    configure: captcha_settings
   

   some of the remaining deprecations require 8.8. If we do fix them there and not create a follow-up then this needs to be changed to ^8.8 || ^9.

2. +++ b/composer.json
   @@ -1,5 +1,12 @@
   +  },
   +  "require": {
   +    "drupal/core": "^8.0 || ^9.0"
   +  }
   

   there is no neeed anymore for this, it's easier to skip composer.json changes.

I am working on this.

If we are using min version 8.8 then we need to remove core key from info file right ? See https://www.drupal.org/node/3070687

If we are using min version 8.8 then we need to remove core key from info file right ? See https://www.drupal.org/node/3070687

Yes but the CAPTCHA module will support the earlier versions of Drupal 8. So we need to keep the core key in the info file other wise module will not detected on Drupal Core Version < D8.7.7.

Why not commit this in a separated branch? (I can create it)

It's not allowed to have ^8.8 and the old core key, if you require 8.8 then it is by design that older versions can't detect that anymore because they're not compatible.

I would vote against an 8.x-2.x branch for this, that creates considerable long-term maintenance overhead, and marking the old one as unsupported soon-ish would then force users to update as they get unsupported warnings. My approach is simply to wait until I decide to drop 8.7 support, which I plan to do for my modules that require 8.8 around march.

I was actually thinking of creating a 9.x branch not 8.x-2.x

That's not possible, d.o does not allow that. Instead, the 8.x- prefix will eventually go away and you'll be able to create an 2.x.y branch. But that will take some more time to become available.

Got it, so let's leave it the way its.

Thanks for your help Berdir

@Berdir, I agree with your point. Thanks for the clarification.

Found a pre render callback that we need to move to a trusted callback, that just works on the Captcha class because it's a render plugin and all their methods are trusted becuase inherits from \Drupal\Core\Render\Element\RenderCallbackInterface through ElementInterface. I did keep the old function place in case someone calls it directly, that doesn't seem to be the case in contrib: grep.xnddx.ru/search?text=captcha_pre_render_process, so feel free to remove it if you don't think that's an official API from this module.

Also added the defaultTheme property and some assertTrue/False fixes, this is now passing all tests locally without remaining deprecation messages. I did improve some asserts related to those changes, e.g. using find() instead of xpath with a selector that works with the stark theme.

Two modules were missing the core_version_requirement key.

Looks like a previously closed, duplicate issue was merged anyway.

Rerolling this patch to remove the duplicated code, I hope I caught everything.

@Neslee Canil Pinto can you please post interdiff files? Without them, it makes it difficult to see what's changed between patches.

This is just a re-roll of #32 I don't know what the patches following that are doing.

Automated tests passed and drupal-check found no problem. What's still needed for RTBC?

Upgrade status scan doesn't find any more issue, patch on #40 applies nicely to 1.0 too. RTBCed.

I think we need to put a composer require section into the composer.json for ^8.8 || ^9.0. Otherwise someone might try to install the site on 8.7 and run into issues where composer didn't know about the stricter requirements that Drupal has outlined in the info.yml file.

Same as #3042808: Drupal 9 Deprecated Code Report, that's not necessary unless it already has a conflicting definition. Not doing it avoids mistakes in the future, like i I did for token.module where I forgot to update it when changing .info.yml :)

I understand that the code passed RTBC and the last comment isn't required. Who can move this forward as a release for D9? It might be an issue for many sites as they will try to upgrade to D9.

Ran through the tests, this works great. Committed just in time for D9!