Deprecate ModuleHandlerInterface::getName()

I will work on this.Could you tell me where is specifically used? I cannot see anywhere. Thanks

Updated IS with some instructions. Thanks for taking it on, @yo30.

Hi, I've create a patch with the change after the update of 8.6.x, I hope it works.

Thanks. Looking pretty good except for a few things.

1. --- a/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   

   Still needs to @trigger_error() in the implementation.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   @@ -388,6 +388,11 @@ public function getModuleDirectories();
   +   * @see https://www.drupal.org/node/2926070   ¶
   

   Trailing whitespace, and the change record is https://www.drupal.org/node/2709919

Updated patch with changes addressed in comment #11 & also added interdiff.

Thanks @alexpott for pointing this issue mentioned in comment #11.1, also added an interdiff.

Few changes in patch & PHPLint error removed.

Sorry, my bad missed one line to remove & added updated patch with an interdiff.

Added an update in PermissionHandler::getModuleNames()

I hope changes help reduce bugs in tests

Test injection into PermissionHandler

Updated to prevent error when PermissionHandlerTest

Hi @zahord.

Thanks for your patch!

I reviewed the patch and found a few things that need to be fixed before to be ready.

I'm going to work on those later this week unless you wanted to do it :-)

let me know.

David.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   @@ -389,6 +389,11 @@ public function getModuleDirectories();
   +   * @see https://www.drupal.org/node/2709919
   
   +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -103,7 +103,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -      '#title' => $this->t('This link is provided by the @name module. The title and path cannot be edited.', ['@name' => $this->moduleHandler->getName($provider)]),
   +      '#title' => $this->t('This link is provided by the @name module. The title and path cannot be edited.', ['@name' => \Drupal::service('extension.list.module')->getName($provider)]),
        ];
   

   Here the extension.list.module can be injected.

2. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -49,7 +49,7 @@ protected function getProviderName($provider) {
   -      return $this->getModuleHandler()->getName($provider);
   +      return \Drupal::service('extension.list.module')->getName($provider);
   

   Here the service can't be injected but we might want to emulate what was done with the moduleHandler.

   I mean, create a function called getExtensionListModule.

3. +++ b/core/modules/breakpoint/src/BreakpointManager.php
   @@ -244,7 +244,7 @@ public function clearCachedDefinitions() {
   -      $label = $this->moduleHandler->getName($group);
   +      $label = \Drupal::service('extension.list.module')->getName($group);
   

   Here the extension.list.module can be injected as well.

4. +++ b/core/modules/help/src/Controller/HelpController.php
   @@ -114,7 +114,7 @@ public function helpMain() {
   -      $module_name = $this->moduleHandler()->getName($name);
   +      $module_name = \Drupal::service('extension.list.module')->getName($name);
   

   Here we should inject it as well.

5. +++ b/core/modules/help/src/Plugin/HelpSection/HookHelpSection.php
   @@ -60,7 +60,7 @@ public static function create(ContainerInterface $container, array $configuratio
   -      $title = $this->moduleHandler->getName($module);
   +      $title = \Drupal::service('extension.list.module')->getName($module);
   

   Here as well, it should be injected.

6. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -153,7 +153,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -          '#markup' => $this->moduleHandler->getName($provider),
   +          '#markup' => \Drupal::service('extension.list.module')->getName($provider),
   

   here as well.

7. +++ b/core/modules/user/src/Plugin/views/access/Permission.php
   @@ -115,7 +115,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -      $display_name = $this->moduleHandler->getName($provider);
   +      $display_name = \Drupal::service('extension.list.module')->getName($provider);
   

   Here as well.

8. +++ b/core/modules/user/src/Plugin/views/field/UserData.php
   @@ -73,7 +73,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -      $names[$name] = $this->moduleHandler->getName($name);
   +      $names[$name] = \Drupal::service('extension.list.module')->getName($name);
   

   as well.

9. +++ b/core/modules/user/src/Plugin/views/filter/Permissions.php
   @@ -70,7 +70,7 @@ public function getValueOptions() {
   -        $display_name = $this->moduleHandler->getName($provider);
   +        $display_name = \Drupal::service('extension.list.module')->getName($provider);
   

   Here as well

10. +++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
    @@ -62,6 +75,11 @@ protected function setUp() {
    +    // Initialize modules ModuleExtensionList object. ¶
    

    Remove this extra space at the end of the line.

Here my first try.

I removed the moduleHandler in the plugins where it wasn't used anymore.

I used this line to make sure that we removed all the old calls:

grep -HirnE modulehandler\-\>getName .

And I fixed all the points from #35

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -747,6 +747,7 @@ public function getModuleDirectories() {
+    @trigger_error('getName($module) is deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\Extension\ModuleExtensionList::getName($module). See https://www.drupal.org/node/2709919.', E_USER_DEPRECATED);

Should be __METHOD__ . 'is deprecated....' like you see in parseDependency().

Also, we're up to 8.7.x now, so we have to change that in both of the deprecation messages.

Hi @Mile23

Thank you for your review.

Ok New patch, I did the following changes:

• There was a couple uses of the getName in the language.module used directly from the Drupal::service method, so I updated them.
• removed the classed added at #31 ( TestModuleExtensionList) and instead I mocked the ModuleExtensionList.
• #38 was addressed
• I fixed the MenuLinkDefaultFormTest.

And let's see what the bot says at this time :-)

The commands used to check if all the methods were updated are:

grep -Hrin "module_handler.*getName" .
grep -HirnE modulehandler\-\>getName .

Finally green and yay! few things needs work

1. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -71,6 +71,21 @@ public function getModuleHandler() {
   +    if (isset($this->moduleExtensionList)) {
   +      return $this->moduleExtensionList;
   +    }
   +    return \Drupal::service('extension.list.module');
   

   var never set - use `!isset()` condition to get service to static cache

2. +++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
   @@ -461,3 +474,7 @@ public function formatPlural($count, $singular, $plural, array $args = [], array
   +
   +if (!defined('DRUPAL_MINIMUM_PHP')) {
   +  define('DRUPAL_MINIMUM_PHP', '5.3.10');
   +}
   
   +++ b/core/tests/Drupal/Tests/Core/Menu/MenuLinkDefaultFormTest.php
   @@ -51,3 +53,7 @@ public function testExtractFormValues() {
   +
   +if (!defined('DRUPAL_MINIMUM_PHP')) {
   +  define('DRUPAL_MINIMUM_PHP', '5.3.10');
   +}
   
   +++ b/core/tests/Drupal/Tests/Core/Plugin/CategorizingPluginManagerTraitTest.php
   @@ -151,3 +153,7 @@ public function processDefinition(&$definition, $plugin_id) {
   +
   +if (!defined('DRUPAL_MINIMUM_PHP')) {
   +  define('DRUPAL_MINIMUM_PHP', '5.3.10');
   +}
   

   would be great to add why that added and follow-up to get rid of

Reading through this issue, I understand that we *can* deprecate ModuleHandlerInterface::getName()
But I don't see any discussion of why we should.

The module handler service seems like the perfect place to consult when looking for this information.
Just because it itself has to delegate to another service to find the answer doesn't mean that all module devs should do the same instead.
Additionally, you end up with code like this, where you need TWO separate services injected:

+++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
@@ -65,14 +73,15 @@ class MenuLinkDefaultForm implements MenuLinkFormInterface, ContainerInjectionIn
     $this->moduleHandler = $module_handler;
+    $this->moduleExtensionList = $module_extension_list;

Why do we want to encourage devs to use the extension.list.module service directly?
I think the abstraction of keeping it on the module handler is a good one.

Thanks for all your comments!

40.1

var never set - use `!isset()` condition to get service to static cache

I almost copied getModuleHandler to create getModuleExtensionList and there the moduleHandler is not set either, and I think that is on purpose because this is a trait, so we cannot know if the class that used this trait defined the var or not, so it tries to check if it is defined and if that is the case then it uses it if not then get the service.

41.2

would be great to add why that added and follow-up to get rid of

I had to define the constant because it is used at Drupal\Core\Extension\ModuleExtensionList::defaults not sure if we can just get rid of it, the same happened with Drupal\Core\Extension\ThemeHandler and at ThemeHandlerTest these constants were defined as well at the end of the tests, and there wasn't added a comment explaining why they were defined there, should I anyway add the comment?

I noticed a small error so I attach a new patch.

I hope this address #42, if you want me to change something anyway let me know and I will provided a new patch.

Thanks again.

David.

+++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
@@ -461,3 +474,7 @@ public function formatPlural($count, $singular, $plural, array $args = [], array
+if (!defined('DRUPAL_MINIMUM_PHP')) {
+  define('DRUPAL_MINIMUM_PHP', '5.3.10');

+++ b/core/tests/Drupal/Tests/Core/Menu/MenuLinkDefaultFormTest.php
@@ -51,3 +53,7 @@ public function testExtractFormValues() {
+if (!defined('DRUPAL_MINIMUM_PHP')) {
+  define('DRUPAL_MINIMUM_PHP', '5.3.10');

+++ b/core/tests/Drupal/Tests/Core/Plugin/CategorizingPluginManagerTraitTest.php
@@ -151,3 +153,7 @@ public function processDefinition(&$definition, $plugin_id) {
+if (!defined('DRUPAL_MINIMUM_PHP')) {
+  define('DRUPAL_MINIMUM_PHP', '5.3.10');

Generally, if you're doing this then you're really writing a kernel test and don't know it yet. If we can move the tests to be kernel tests without changing what they're testing, then maybe we should. But if we leave them as unit tests, we should at least mark them as @runTestsInSeparateProcesses so we don't pollute the constant space for other tests. And also add @todo so we can change them in #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions

Hi AlexPott.

45.1

The module handler is not required anymore and should be removed.

Yes it is used, not by MenuLinkDefaultForm but ViewsMenuLinkForm extends this class and there it is being used at buildConfigurationForm

45.2

Ok, new patch attached (I didn't deleted getModuleHandler() even if it is not used anymore because it is a public function and not sure if I will break the BC if I delete it) this is pretty similar to what I did on #3012962 :-)

>>> \Drupal::moduleHandler()->getName('webform_ui');
=> "Webform UI"
>>> \Drupal::moduleHandler()->getModule('webform_ui');
Drupal/Core/Extension/Exception/UnknownExtensionException with message 'The module webform_ui does not exist.'
>>>

The above code is super odd to me.

This is actually a bug. The exception should be UninstalledExtensionException. This particular wtf is one of the reasons for deprecating ModuleHandler::getName(). A thing to do here would be to get them to behave the same.

#46

Generally, if you're doing this then you're really writing a kernel test and don't know it yet. If we can move the tests to be kernel tests without changing what they're testing, then maybe we should

I haven't the expertise to say if we can convert this into a KernelTest, so I will let to someone else decide about that, for now I just added the @todo and the @runTestsInSeparateProcesses in the tests which define the constant.

Also, I fixed the failing test.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -747,6 +747,7 @@ public function getModuleDirectories() {
   +    @trigger_error(__METHOD__ . ' is deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\Extension\ModuleExtensionList::getName($module). See https://www.drupal.org/node/2709919.', E_USER_DEPRECATED);
   

   I guess at this point we would need to reroll this patch against 8.8.x

2. +++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
   @@ -461,3 +475,8 @@ public function formatPlural($count, $singular, $plural, array $args = [], array
   +// @todo Remove as part of https://www.drupal.org/node/2908079
   +if (!defined('DRUPAL_MINIMUM_PHP')) {
   +  define('DRUPAL_MINIMUM_PHP', '5.3.10');
   +}
   

   I'm confused, why are we adding a DRUPAL_MINIMUM_PHP which is lower than the real value right now?

New patch rerolled against 8.8.x

I'm confused, why are we adding a DRUPAL_MINIMUM_PHP which is lower than the real value right now?

No idea, I just updated the patch to use the real value.

Answering @tim.plunkett in #41, there is a hidden circular dependency between the module handler and the extension list.

ExtensionList has module handler injected, and ModuleHandler::getName() refs the extension list.

So this would resolve that. But yeah, there are other ways around that, such as a setModuleExtensionList method on module handler, which we could call from ModuleExtensionList::construct, but that still hides the dependency

Should this be going into 10?

Yes, thank you

Took a shot. Lets see what fails.

Apologize can't get phpstan working on D10 so if this fails sorry!

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -715,6 +715,7 @@ public function getModuleDirectories() {
   +    @trigger_error(__METHOD__ . ' is deprecated in Drupal 10.1.x and will be removed before Drupal 11.0.0. Use \Drupal\Core\Extension\ModuleExtensionList::getName($module). See https://www.drupal.org/node/2709919.', E_USER_DEPRECATED);
   

   We need a legacy test for this deprecation

2. +++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
   @@ -400,3 +418,8 @@ public function formatPlural($count, $singular, $plural, array $args = [], array
   +// @todo Remove as part of https://www.drupal.org/node/2908079
   +if (!defined('DRUPAL_MINIMUM_PHP')) {
   +  define('DRUPAL_MINIMUM_PHP', '5.5.9');
   +}
   

   We shouldn't need to do this anymore. See https://www.drupal.org/node/2909361

3. +++ b/core/tests/Drupal/Tests/Core/Menu/MenuLinkDefaultFormTest.php
   @@ -51,3 +54,8 @@ public function testExtractFormValues() {
   +// @todo Remove as part of https://www.drupal.org/node/2908079
   +if (!defined('DRUPAL_MINIMUM_PHP')) {
   +  define('DRUPAL_MINIMUM_PHP', '5.5.9');
   +}
   

   ditto

4. +++ b/core/tests/Drupal/Tests/Core/Plugin/CategorizingPluginManagerTraitTest.php
   @@ -151,3 +161,8 @@ public function processDefinition(&$definition, $plugin_id) {
   +// @todo Remove as part of https://www.drupal.org/node/2908079
   +if (!defined('DRUPAL_MINIMUM_PHP')) {
   +  define('DRUPAL_MINIMUM_PHP', '5.5.9');
   +}
   

   ditto

Addressed #72

+++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleHandlerTest.php
@@ -26,2 +26,12 @@
+    \Drupal::service('module_handler')->getName('module_test');

We should add an assert here.

Updated based on #74

1. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -60,12 +68,15 @@ class MenuLinkDefaultForm implements MenuLinkFormInterface, ContainerInjectionIn
       * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   

   With the change from this issue is the module handler service no longer used. It can be properly deprecated. The ViewsMenuLinkForm is extending this class and it is using the module handler service. Therefor in that class the module handler service need to be added.

2. +++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
   @@ -46,13 +47,12 @@ protected function processDefinitionCategory(&$definition) {
   +    catch (UnknownExtensionException $unknownExtensionException) {
   

   Can we change this line to: catch (UnknownExtensionException $e) {

3. +++ b/core/modules/breakpoint/src/BreakpointManager.php
   @@ -100,9 +110,10 @@ class BreakpointManager extends DefaultPluginManager implements BreakpointManage
   -  public function __construct(ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler, CacheBackendInterface $cache_backend, TranslationInterface $string_translation) {
   +  public function __construct(ModuleHandlerInterface $module_handler, ModuleExtensionList $module_extension_list, ThemeHandlerInterface $theme_handler, CacheBackendInterface $cache_backend, TranslationInterface $string_translation) {
   

   We add a new service always to the end of the list, not somewhere in the middle. It is easier for classes that are overriding the class we are changing.

4. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -74,13 +74,13 @@ class ReviewForm extends MigrateUpgradeFormBase {
   -  public function __construct(StateInterface $state, MigrationPluginManagerInterface $migration_plugin_manager, PrivateTempStoreFactory $tempstore_private, MigrationState $migrationState, ConfigFactoryInterface $config_factory, ModuleHandlerInterface $module_handler = NULL) {
   +  public function __construct(StateInterface $state, MigrationPluginManagerInterface $migration_plugin_manager, PrivateTempStoreFactory $tempstore_private, MigrationState $migrationState, ConfigFactoryInterface $config_factory, ModuleExtensionList $module_extension_list) {
   

   We cannot just remove an exiting service from a class. The problem is that an overridding class might be using the service and when we remove it the overriding will fail. This is a BC break. So module handler service needs to be properly deprecated in this issue.

5. +++ b/core/modules/user/src/Plugin/views/access/Permission.php
   @@ -55,13 +55,13 @@ class Permission extends AccessPluginBase implements CacheableDependencyInterfac
   -  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, ModuleExtensionList $module_extension_list) {
   

   Again we cannot just remove a service. It need to be properly deprecated.

6. +++ b/core/modules/user/src/Plugin/views/filter/Permissions.php
   @@ -42,14 +42,14 @@ 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, ModuleExtensionList $module_extension_list) {
   

   Again we cannot just remove a service. It need to be properly deprecated.

7. I have created a new change record. Please change the links to the new CR (https://www.drupal.org/node/3310017).

Interdiff is missing some changes. I use patchutils and get an error sometimes when the changes are alot.

This patch needed a reroll also

I addressed the points in #76 to best my ability. Could use some feedback on the deprecated service if that was done correctly.

Here's fixes and clean-up

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.

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

needs summary update for newAPI changes at least

Patch to address review #83

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

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?

random

Needs a rebase on 11.x

re-roll after context change in #3325557: Enable more service autowiring by adding interface aliases to core modules

Reroll looks good

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.

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.