Automated Drupal 10 compatibility fixes

This is still "needs work."

Got that Greg, Thanks for having time to follow up on this issue.
Important notes from #12

Noticed Pierre's notes about

modules/contrib/genpass/genpass.module:
┌────────┬──────┬──────────────────────────────────────────────────────────────┐
│ STATUS │ LINE │                           MESSAGE                            │
├────────┼──────┼──────────────────────────────────────────────────────────────┤
│ Fix    │ 323  │ Call to deprecated method getImplementations() of class      │
│ later  │      │ Drupal\Core\Extension\ModuleHandlerInterface. Deprecated in  │
│        │      │ drupal:9.4.0 and is removed from drupal:10.0.0. Instead you  │
│        │      │ should use ModuleHandlerInterface::invokeAllWith() for hook  │
│        │      │ invocations or you should use                                │
│        │      │ ModuleHandlerInterface::hasImplementations() to determine if │
│        │      │ hooks implementations exist.                                 │
│        │      │                                                              │
└────────┴──────┴──────────────────────────────────────────────────────────────┘

Oh the last code was a static return!

Researched a bit on what other modules are doing in this case
https://git.drupalcode.org/project/scheduler/-/commit/1bb684d#66fb3c85a3...

Got the point.
Testing custom module with a hook_password to be listed in the return of the genpass_algorithm_modules function.

/**
 * Return an array of all modules which implement hook_password.
 */
function genpass_algorithm_modules() {
  // Fetch a list of all modules which implement the password generation hook.
  // To be in this list, a module must implement a hook, and return random
  // passwords as strings.

  $return = [];
  \Drupal::moduleHandler()->invokeAllWith('password', function (callable $hook, string $module) use (&$return) {
    $return[$module] = $module;
  });
  return $return;
}

Not sure if I have a push access over the issue fork.
Hoping for Pierre to commit his suggested change.

All hook invocation delegated to Module Handler service →
📌 Delegate all hook invocations to ModuleHandler Fixed

https://git.drupalcode.org/project/drupal/-/commit/8f1c7a8#9a1c5d1dd1901...

Use ModuleHandlerInterface::hasImplementations() to determine if hooks implementations exist.

Suggesting to change to use has Implementations:

/**
 * Return an array of all modules which implement hook_password.
 */
function genpass_algorithm_modules() {
  // Fetch a list of all modules which implement the password generation hook.
  // To be in this list, a module must implement a hook, and return random
  // passwords as strings.

  $all_modules = \Drupal::moduleHandler()->getModuleList();
  $return = [];
  foreach ($all_modules as $module) {
    if (\Drupal::moduleHandler()->hasImplementations('password', $module)) {
      $return[$module] = $module;
    }
  }
  return $return;
}

If we are using moduleHandler()->hasImplementations, we can't keep the D8 compatibility, as this method was introduced in Drupal 9.4.

Furthermore, while running the tests locally, I got the following deprecation notice:

3x: user_password() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Password\PasswordGeneratorInterface::generate() instead. See https://www.drupal.org/node/3153113 →
3x in GenpassTest::testGenpassConfigsAndCreateUsersByAdmin from Drupal\Tests\genpass\FunctionalJavascript

But I can not seem to find any code using, user_password() and I have no idea what is causing this deprecation notice. Maybe one of the maintainers can rerun the tests using PHP >= 8.1, to see if that problem also appears on remote?

Nice, good point Joshua.
To me it feels a new 2.0.x branch would keep deprecation and what new class/function/service to use. and support for PHP 8.1, PHP 8.2, later on.

• Ditching old ways is good
• Adopting new better ways is better

Managing between that is a tricky business
Let us see how the final changes will look like.

@Rajab Natshah, sounds good! :)

@greggles, could you perhaps create that 2.0.x as of #20 and #21? So we can proceed here :)

Hello @Anybody I have created 2.0.x branch in this branch fork

a44be3f0

What would the path to upgrade be for someone on Drupal 9 with this new 2.0.x that's proposed? Do they need to disable genpass as part of the core upgrade process?

Suggested to do this in two steps:
2.0.x for ^9 || ^10 ( only new code - ditching any ~8.0 or ~9.0 code using a logic with version compare !version_compare(\Drupal::VERSION, '10.0.0', 'lt')
8.x-1.x for ^8 || ^9 || ^10 ( with old code and new code - only needed for the upgraded process to 2.0.x )

Drupal 8 and 9 sites could update to latest version on the genpass ~1.0
then after the update to Drupal 10, only change in the composer from ~1.0 to ~2.0

• No disable for the module
• No drush updb on the switch from ~1.0 to ~2.0

New features could only be committed to 2.0.x
and freeze new features on the 8.x-1.x branch.

Hoping that this would be a good plan.
Greg, do you find this plan logical? for sure know more needed cases to cover.

Thanks for laying that out, @Rajab Natshah. That mostly makes sense to me. 2.0.x being for 9 or 10 works for me, but I think you maintain a bit more compatibility than needed.

Drupal 8 and 9 sites could update to latest version on the genpass ~1.0
then after the update to Drupal 10, only change in the composer from ~1.0 to ~2.0

No disable for the module
No drush updb on the switch from ~1.0 to ~2.0

New features could only be committed to 2.0.x
and freeze new features on the 8.x-1.x branch.

That plan means that 2.0.x only needs to work with 10.x, right? That seems easier to do.

Another option:

Make 8.x-1.x compatible with both 8.x and 9.x (at this point I don't think we should invest much time in 8, honestly).
Make 2.0.x compatible with both 9.x and 10.x.
The upgrade flow is for 8.x-and 9.x to get on 9.x core with 8.x-1.x. I believe this is already possible and we can say that 8.x-1.x is no longer actively developed. Then once core is at 9.x people should upgrade to genpass 2.x and can upgrade core to 10.x.

Either of these 3 plans (yours, my first modification to yours to drop 10 support from 8.x-1.x branch, the 3rd one here to drop 10 support form 8.x-1.x and adjust the upgrade flow) works for me.

I'm also happy passing on maintainership if someone else wants to become maintainer - please file an issue with links to your activity in this module's queue showing creation of patches, reviews, etc. that demonstrate the work of a maintainer.

I am ok, with either keeping the 2.0x branch D9 and D10 compatible OR D10 only.

Although if we want to keep the D9 compatibility we have to define core_version_requirement: ^9.4 || ^10, as explained in #19:

If we are using moduleHandler()->hasImplementations, we can't keep the D8 compatibility, as this method was introduced in Drupal 9.4

Thanks, Greg, I agree that your modification plan is better.

Either of these 3 plans (yours, my first modification to yours to drop 10 support from 8.x-1.x branch, the 3rd one here to drop 10 support form 8.x-1.x and adjust the upgrade flow) works for me.

2.0.x will have ^9 || ^10
8.x-1.x will have ^8 || ^9

I'm also happy passing on maintainership if someone else wants to become maintainer - please file an issue with links to your activity in this module's queue showing creation of patches, reviews, etc. that demonstrate the work of a maintainer.

Love to help in maintaining the Generate Password module.
💬 Offering to Support and Maintain the Generate Password module Fixed

Greg, Thank you for time and consideration.

Starting with 📌 Start a 2.0.x branch for the Generate Password module to support Drupal ^9.4 || ^10 Fixed

As listed in #17

Having the following back to be committed to the 2.0.x branch.

Use ModuleHandlerInterface::hasImplementations() to determine if hooks implementations exist.

/**
 * Return an array of all modules which implement hook_password.
 */
function genpass_algorithm_modules() {
  // Fetch a list of all modules which implement the password generation hook.
  // To be in this list, a module must implement a hook, and return random
  // passwords as strings.

  $all_modules = \Drupal::moduleHandler()->getModuleList();
  $return = [];
  foreach ($all_modules as $module_name => $module) {
    if (\Drupal::moduleHandler()->hasImplementations('password', $module_name)) {
      $return[$module_name] = $module_name;
    }
  }

  return $return;
}

Thank you, Rajab for bringing this issue towards an end! :)

What are the remaining steps to get this committed to 2.0.x? As I am aware, we only need to test your changes (especially the "hasImplementations" part)?!

Or is there anything else to do, as you changed the status to "Needs work"?

Rebase failed: Rebase locally, resolve all conflicts, then push the branch. Try again.

📌 Start a 2.0.x branch for the Generate Password module to support Drupal ^9.4 || ^10 Fixed

• ✅ Automated unit testing coverage
• ✅ Automated functional testing coverage

Development version: 2.0.x-dev → updated 7 Feb 2023 at 09:35 UTC

• Testing result: PHP 8.1 & MySQL 8, D10.0.1 1 pass → all results →

Great work! Thanks for your time! :)

Thank you all, for the nice collaborative work on this issue for this important module.

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

ELC → closed merge request !2

ELC → closed merge request !1