Deprecate ModuleHandlerInterface::getName()

Here's fixes and clean-up

The last submitted patch, 79: 2926070-79.patch, failed testing. View results →

Fix broken test

Applied patch #81

Searched for "Drupal::service('module_handler')->getName" only instance I found was in the test.

Change record appears to list those classes that have been updated with a new argument.

Think this is good.

1. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -46,13 +48,13 @@ protected function processDefinitionCategory(&$definition) {
   -    $list = $this->getModuleHandler()->getModuleList();
   -    // If the module exists, return its human-readable name.
   -    if (isset($list[$provider])) {
   -      return $this->getModuleHandler()->getName($provider);
   +    try {
   +      return $this->getModuleExtensionList()->getName($provider);
   +    }
   +    catch (UnknownExtensionException $e) {
   +      // Otherwise, return the machine name.
   +      return $provider;
        }
   -    // Otherwise, return the machine name.
   -    return $provider;
   

   With these changes \Drupal\Core\Plugin\CategorizingPluginManagerTrait::getModuleHandler is unused so we should deprecate it.

2. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -70,6 +72,21 @@ public function getModuleHandler() {
   +  /**
   +   * Returns the module extension list used.
   +   *
   +   * @return \Drupal\Core\Extension\ModuleExtensionList
   +   *   The module extension list.
   +   */
   +  public function getModuleExtensionList(): ModuleExtensionList {
   

   Why public? I think the old getModuleHandler() was public by mistake. This doesn't feel like it should be a public method of a plugin manager.

3. +++ b/core/modules/language/language.module
   --- a/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   
   +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -76,11 +77,17 @@ class ReviewForm extends MigrateUpgradeFormBase {
   -  public function __construct(StateInterface $state, MigrationPluginManagerInterface $migration_plugin_manager, PrivateTempStoreFactory $tempstore_private, MigrationState $migrationState, ConfigFactoryInterface $config_factory, ModuleHandlerInterface $module_handler) {
   +  public function __construct(StateInterface $state, MigrationPluginManagerInterface $migration_plugin_manager, PrivateTempStoreFactory $tempstore_private, MigrationState $migrationState, ConfigFactoryInterface $config_factory, ModuleHandlerInterface $module_handler, protected ?ModuleExtensionList $moduleExtensionList = NULL) {
        parent::__construct($config_factory, $migration_plugin_manager, $state, $tempstore_private);
        $this->migrationState = $migrationState;
        $this->moduleHandler = $module_handler;
   +    if ($this->moduleExtensionList === NULL) {
   +      @trigger_error('Calling ' . __METHOD__ . '() without the $moduleExtensionList argument is deprecated in drupal:10.1.0 and will be required in drupal:11.0.0. See https://www.drupal.org/node/3310017', E_USER_DEPRECATED);
   +      $this->moduleExtensionList = \Drupal::service('extension.list.module');
   +    }
   

   The module handler can be deprecated here - use a union id ModuleHandlerInterface|ModuleExtensionList $module_handler and the DeprecatedServicePropertyTrait. This is better than doing constructor promotion because it means less change in the future. It does mean you'll have to add the typehinted property the ModuleExtensionList but it means we don't have a NULL argument and we don't have to go about removing the module handler later.

4. +++ b/core/modules/user/src/Plugin/views/access/Permission.php
   @@ -57,11 +58,17 @@ class Permission extends AccessPluginBase implements CacheableDependencyInterfac
   +   * @param \Drupal\Core\Extension\ModuleExtensionList|null $moduleExtensionList
   +   *   The module extension list.
       */
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, PermissionHandlerInterface $permission_handler, ModuleHandlerInterface $module_handler) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, PermissionHandlerInterface $permission_handler, ModuleHandlerInterface $module_handler, protected ?ModuleExtensionList $moduleExtensionList = NULL) {
        parent::__construct($configuration, $plugin_id, $plugin_definition);
        $this->permissionHandler = $permission_handler;
        $this->moduleHandler = $module_handler;
   

   We need to deprecate passing in module handler here. We can use a union on the last argument to pass in either a module handler or a module extension list and do the right thing. We can also use the deprecatedServicePropertyTrait to provide BC.

5. +++ b/core/modules/user/src/Plugin/views/field/UserData.php
   --- a/core/modules/user/src/Plugin/views/filter/Permissions.php
   +++ b/core/modules/user/src/Plugin/views/filter/Permissions.php
   
   +++ b/core/modules/user/src/Plugin/views/filter/Permissions.php
   @@ -44,12 +45,18 @@ class Permissions extends ManyToOne {
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, PermissionHandlerInterface $permission_handler, ModuleHandlerInterface $module_handler) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, PermissionHandlerInterface $permission_handler, ModuleHandlerInterface $module_handler, protected ?ModuleExtensionList $moduleExtensionList = NULL) {
        parent::__construct($configuration, $plugin_id, $plugin_definition);
    
        $this->permissionHandler = $permission_handler;
        $this->moduleHandler = $module_handler;
   +    if ($this->moduleExtensionList === NULL) {
   +      @trigger_error('Calling ' . __METHOD__ . '() without the $moduleExtensionList argument is deprecated in drupal:10.1.0 and will be required in drupal:11.0.0. See https://www.drupal.org/node/3310017', E_USER_DEPRECATED);
   +      $this->moduleExtensionList = \Drupal::service('extension.list.module');
   +    }
   

   Module handler is not longer used... we can use a union and to the same as above here.

I think better to parent the issue to 📌 ModuleHandler should not maintain list of installed modules now that ModuleExtensionList exists Needs work according to #42

needs summary update for newAPI changes at least

And re-roll for 10.2 as beta phase + minor clean-up

The last submitted patch, 85: 2926070-85.patch, failed testing. View results →

Unrelated failure

Can't believe we are already in 10.2.

Any idea when a 10.2.x-dev will be opened? This ticket should probably be postponed on that.

But the deprecation looks good.

Moving to NW for the issue summary update you tagged in #84.

Updated IS.

IS looks good!

Do we have 10.2 tag?

The last submitted patch, 85: 2926070-85.patch, failed testing. View results →

random

Needs a rebase on 11.x

re-roll after context change in 📌 Enable more service autowiring by adding interface aliases to core modules Fixed

Reroll looks good

The last submitted patch, 95: 2926070-95.patch, failed testing. View results →

Thank you for re-roll and updating deprecation to 10.3

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

dimitriskr → changed the visibility of the branch 11.x to hidden.

Updated the versions on the CR

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased on 11.x. Back to RTBC

Needs another rebase.

Rebased

Rebase seems good.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

There is something wrong here. ExtensionList is marked as internal. We should not recommend using it in change records.

From the ExtensionList docs:

therefore there are no guarantees that the
 *   internal implementations including constructor signature and protected
 *   properties / methods will not change over time.

This doesn't apply to the existing public methods.

As long as it has class level @internal tag the whole class is considered as internal by static analyzers.
We might need to move the @internal tag to particular methods in that class.

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