Difficult migration from Drupal 9 to 10

Thanks for the link @lostcarpark. I'm with you and happy to review a MR, if someone prepares and tests it.
We already invested a lot of open source time (=money) into the module as a very small company, so this is a bonus community job.

Upgrading works, like you described, but it's not that convenient, due to the technical incompatibilities.

Thank you.

Thanks @lostcarpark indeed that was one of the upstream Symfony incompatibilities. There might be more, I'm unsure.

Finding the same, can 2.x not be made compatible with Drupal 9?

Just tried suggestion #6. Seems to work without problems on 9.5.9.

It would be better if we could commit the #7 patch as a prerelease for example 2.1-alpha, because the composer dependency error is triggered before the composer patches step was invoked.

I've created a temporarily alternative package to make module upgrade more easier and here are the steps to follow:

1 - Remove existing drupal/captcha package: composer remove drupal/captcha

2 - Require bkhouy/captcha package: composer require bkhouy/captcha:^2.0

3 - Continue upgrading the rest of your project...

4 - Once you finish the upgrade and your project is running on Drupal 10 version on production, then you need to rollback to drupal/captcha module by following those steps:

=> Remove bkhouy/captcha package: composer remove bkhouy/captcha
=> Require drupal/captcha package: composer require drupal/captcha:^2.0

Woah, sorry I haven't been getting notifications here -- I did NOT see what happened to captcha. But basically, this is a classic case for not following best practices for major versions:

https://medium.com/jakob-on-drupal/dont-go-making-major-version-changes-...

I'll see if we can prioritize fixing the 1.x and 2.x issues for Drupal 10. Acquia has an interest in making sure this module is compatible.

@japerry thanks for your feedback and support. We're very happy about experienced developers who'd like to help.
But please ensure to stay friendly and keep in mind that each of us invested a lot of time here to push things forward as good as possible. Talk is cheap and it's easy to criticize each other. On the other hand, first please take a look at how long there was no progress in this module, lots of bugs and the codebase is ancient in large parts.

If you have clever ideas how to fix things here, let's do it. But also let's appreciate each other's hard work. Nobody intentionally did bad things here, and if you take a deeper look at the reasons, you may see that. And of course we're willing to learn!

PS: I'd be super grateful if Acquia helps with CAPTCHA again. We're just a small company and there's a lot of work of any category in this module to do.

Yes, I apologize that we haven't been more involved. We're a small team, so we're not able to get to all the modules that I either co-maintain as fast as I'd like. That said, please reach out if you have questions regarding module compatibility, as this is probably the area we have the most expertise.

That said -- there are some red flags here on the 1.x to 2.x changeover. First, while there was an incompatibility in the StreamedResponse classes, they aren't really written correctly in the first place. I've made a PR that moves most of that logic into the existing Render Service (which only had one static method.. huh).... and a callable StreamResponse instead of extending the Response classes. This will make Captcha compatible with Drupal 9.4+ and concievably 8.9, but I don't have tests for that, and its been long enough that I'm fine with upping the minimum version to 9.4 from 8.9. Interesting side effect, moving the extended Response into a service has found more deprecations, which are fixed in this PR as well.

FWIW: There shouldn't have been a new version. 8.x-1.x would have been just fine. But that ship sailed... so instead, I'm suggesting we do what ctools does, which is get 2.x and 8.x-1.x to be the same codebase and make 8.x-1.x as supported, not recommended. This will allow 8.x-1.x and 2.x to support Drupal 9 and 10, with 8.x-1.x dropping support for Drupal 11. This will also allow modules that depend on captcha (8.x-1.x) time to upgrade to the 2.x branch in their composer files, allowing a more seamless upgrade to Drupal 10 and get captcha into a good spot when Drupal 11 comes out (which shouldn't require 3.x)

I have a working version for 1.x and 2.x users on Drupal 9 and 10. Some of the PHP8+ code has been reverted until enough time has passed for users to get off 7.4. I also added an upgrade hook for the move of modules in 8.x-1.x to 2.x. This works well and takes care of a hopefully rare case when someone has the test modules enabled (Drupal won't find them, but they shouldn't be installed anyway).

While there might be some question about the PHP 8 reverts, specifically code related to #3075256: [2.x only] Error calling _captcha_insert_captcha_element -- the method validtion (array|null) isn't going to fix the underlying problem anyway, and just leads to incompatibility.

With this last MR push, things should work correctly on both versions, resolving the need to maintain 8.x-1.x and 2.x separately.

Thanks @japerry for your clever fixes! I agree, we should have seen that it would have been possible to make 8.x-1.x Drupal 10 compatible that way. And I have to admit, that it was not really simple to get into the (partially) code that was from the early days of Drupal 8 or even Drupal 7 and before. Things like

Render Service (which only had one static method.. huh)

were from 2015 for example. So I still hope we made a lot of things better here and not worse with our maintainership.

So thank you very very much for cleaning things up and helping here with your huge experience! :)

What are the next steps? The MR is against 2.x, should we prepare the 8.x-1.x changes (as of #14) here as separate MR? Or what would you suggest?

PS: Regarding the red flags and legacy code, also see the other issues and #3326584: [META] Plan for CAPTCHA 2.1.x release. There's surely a lot that can be improved in the CAPTCHA module, and I'm very much looking forward to doing that as a team. Once again, very happy to see more progress in this very widely used module.

All tests pass, I don't see anything I'd suggest to change. Nice and RTBC from my side.

@japerry thanks for your cleanup! Changes LGTM, I simply dropped the D8 compatibility (as it has reached its EOL). Apologies for the inconvenient module upgrade, but as @Anybody already said, we are a tiny team of 3 developers (me and @Anybody are the only ones active in Drupal issue queues) and we simply do not have the resources to refactor most of the legacy code inside CAPTCHA.

Hence, we tried keeping the old code alive and building / adjusting on top of it. When it came to the Drupal 10 changes, we simply wanted to conform to the new Symfony method changes and couldn't think of a better idea, than dropping the support for older Drupal versions. Of course, stuff like #3351472: Image not shown, PHP error shouldn't slip through, but it occasionally does!

But as already mentioned by @Anybody, we didn't change the StreamedResponse logic at all, we just made sure it worked with D10!
See #3320867: [META][2.x] CAPTCHA 2.0.x release for a list of most changes we did here!

Nevertheless, great to have you on board again @japerry! A seasoned Acquia developer is always appreciated! 🙂👍

RTBC +1

This related issue #3375351: Problem after updating to 2.0.1 is possibly caused by this issue.

Setting this fixed again, let's please proceed in #3375400: captcha_update_8906 hook updating core.extension.yml incorrectly