Move module_set_weight() into ModuleHandler::setWeight(), add ModuleHandler::getWeight() to replace missing functionality

Not sure about the 0 default. A module that does not appear among enabled or disabled modules simply does not exist: FALSE or an exception would make more sense to me.

Also, the ifs are not very readable right now.

Can we just wait and add this as a method to #1331486: Move module_invoke_*() and friends to an Extensions class? We really need to stop adding utility functions to the system and start adding well-thought-out methods and APIs.

So is the time for continuing "throw a function at the wall and see if it sticks" development... At minimum, such a function should be marked as @deprecated with a note to replace it before release. (And the same for set_weight()).

What? I don't agree with that at all. If/when #1331486: Move module_invoke_*() and friends to an Extensions class actually happens, it's free to deprecate whatever it likes. Until then, this is the API we actually have, not the one we wish we had.

1) We could also return NULL or FALSE. In that case, we should ensure that module_set_weight() converts both NULL and FALSE into 0 though.

That's exactly why I think a FALSE/NULL/exception (probably the latter) would make more sense: module_set_weight() cannot assume that what's returned by module_get_weight() is always valid. An empty catch statement would be probably a good way to handle the OP scenario when the "depended" module is not installed.

2) I'm generally not a fan of reversed "yoda" conditions either, but there is a single case in which they make sense: the one in this patch; i.e., a variable assignment combined with a value comparison, leveraging the right-to-left processing of the PHP interpreter.

I'm not talking about yoda conditions, I'm talking about yoda conditions + an assignment in the same if test. Why?

What about this?

Well, the typical use case for this is when your module depends on another one, in this case module_get_weight() will always return a valid weight. Now that we are starting to really use exceptions IMHO we should try and get rid of those icky multiple-typed results. If module_get_weight() cannot return a weight, making it return NULL or FALSE is just poorman's exception handling.

Comparing:

try {
  $weight = module_get_weight('other') + 1;
}
catch ($e) {
  $weight = 10;
}
module_set_weight('mine', $weight);

vs.

$weight = module_get_weight('other');
module_set_weight('mine', $weight !== FALSE ? $weight + 1 : 10);

The former is more verbose but also more readable to my eyes, as its code is cleaner and not "infested" with ifs and ternaries everywhere (actualy I'm thinking about slightly more complex cases, where more than one exception needs to be handled).

I would agree with plach.
In my opinion, if a function cannot return a valid value which can be used directly after the function call, it should throw an exception. I think the practice of returning 'special' values to determine whether there is a valid result was one of the reasons to introduce exceptions.

Just my two cents

9: drupal-module_get_weight-1808132-9.patch queued for re-testing.

could use some help with this.

In the 2 years since this issue was last active, module functionality has almost entirely moved to a service object. There's no need for another function, but it may make sense to put on the appropriate service object.

I was wondering why the set method is still around. is there another issue to move it to a service object?

That code has changed very drastically. system.module is now core.extension, there is no disabled modules, etc.

Updated to use core.extension.

So ... the {system} table was removed more than 5 years ago. Change record: https://www.drupal.org/node/1813642

With it disappeared the ability to set module weights relative to other modules, needed in order to ensure a relative loading order so that dependencies may be met.

This is not a "feature request", it is a bug - functionality has been removed. The change record promised a module_get_weight() function, but that never materialized and the patch in #21 doesn't work because ModuleHandler::moduleExists() no longer returns an integer weight.

As far as I can tell, the concept of module weight is still used internally, we just don't have access to it any more. And a module_set_weight() without a module_get_weight() is pretty useless - the only reason to use module_set_weight() is to set a weight RELATIVE to another module, so you have to be able to find out the weight of that other module.

Is there some alternative solution I don't know about? Perhaps the relative weights are guaranteed by the dependency order listed in the .info.yml files? What about for use cases where there is no dependency involved but if a specific other module exists we still need to load after that module?

If weight is a concept that's no longer relevant to contrib development, then module_set_weight() should be removed and this issue should be changed to "won't fix". Then the above change record should be modified and there should be some documentation added which explains how module relative weights are handled in D8.

I've changed this to "Major" because this is missing functionality which is needed in order to port some modules to D8.

Idk about the second hunk of that patch. But the first hunk is fine. If the module is installed, it's weight will be in that config

Not going to play status wars, because just fixing this is a better use of time

If all the module weights are now stored in the core.extension config, then how about something like the following:
1) Add setWeight($module, $weight) and getWeight($module) methods to ModuleHandler
2) Deprecate module_set_weight()

This puts the weight functionality in the logical and expected place, removes the arbitrary procedural function, and importantly restores the D7 functionality of being able to set and get weights.

Let's see if this works ...

Corrected the missing public on the two new methods.

That's an unhelpful message ...

I spun up a new site to test this and the real error is:

Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "config.factory", path: "config.factory -> config.typed -> module_handler -> config.factory". in Symfony\Component\DependencyInjection\Compiler\CheckCircularReferencesPass->checkOutEdges() (line 69 of /home/dujtj/www/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php).

So basically I can't inject the config.factory service into the module_handler because module_handler is already being injected into config.factory.

So what's the Drupal way out of this? Static call to config()? Injection of container then a lazy get of the config.factory?

I'll work on this some more if there is some consensus for the approach I outlined in #26.

The ModuleInstaller has to use dedicated calls to \Drupal::configFactory() for the same exact file:

  public function install(array $module_list, $enable_dependencies = TRUE) {
    $extension_config = \Drupal::configFactory()->getEditable('core.extension');
    ....

I think #26 sounds great

1. +++ b/core/includes/module.inc
   @@ -170,12 +170,15 @@ function drupal_required_modules() {
    function module_set_weight($module, $weight) {
   +  @trigger_error('module_set_weight() is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0.', E_USER_DEPRECATED);
      $extension_config = \Drupal::configFactory()->getEditable('core.extension');
      if ($extension_config->get("module.$module") !== NULL) {
        // Pre-cast the $weight to an integer so that we can save this without using
   

   Should't the old code now call to \Drupal::moduleHandler()->setWeight?

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -503,6 +503,59 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
   +    }
   +  }
   

   In case we can't find the module, should we throw an exception?

Shouldn't the old code now call to \Drupal::moduleHandler()->setWeight?

Good point, yes, here's a new patch with the old code removed and replaced by a call to \Drupal::moduleHandler()->setWeight()

In case we can't find the module, should we throw an exception?

I'm not sure exactly what you mean here, because the code you quoted isn't part of the patch ... But if you're referring to the 5-year old discussion above, then I think that is outdated.

No public methods on ModuleHandler throw exceptions currently. ModuleHandler::loadInclude(), for instance, does not throw an exception if the include file is not found ... If we throw an exception in setWeight()/getWeight() they would be the only ones. IMO that makes adding exceptions here a little outside the scope of the patch, which I see as restoring the missing getWeight() functionality.

So I vote no on throwing an exception in either setWeight() or getWeight(). If added, they would be the ONLY public functions on ModuleHandler that throw exceptions. Maybe there should be a separate issue opened to re-architect ModuleHandler to use exception handling. But if the consensus is that we should just do it for these two new methods, I'll add that to the patch. I have no desire to stall the issue on this point.

...

All the use cases for setting a weight that I can think of involve relative weights, with the code knowing exactly which other module(s) are involved. (Which is why getWeight() is critical to have!) Why else would you arbitrarily set your own module weight? I don't know. And why would you ever want to set another module's weight?

So say there's another module you conflict with because you both hook_form_alter() the same form so you have to ensure order of execution between the two modules. Then:
1) If you depend on that module, the dependency will ensure the other module exists, so the pattern would be:

// Make sure we get loaded after 'othermodule'.
$module_handler->setWeight('mymodule', $module_handler->getWeight('othermodule') + 1);

2) If you don't depend on that module, you at least know the name of the module so the pattern would be:

// Make sure we get loaded after 'othermodule'.
if ($module_handler->moduleExists('othermodule')) {
  $module_handler->setWeight('mymodule', $module_handler->getWeight('othermodule') + 1);
}

Note we already have a reference to $module_handler if we need to set the weight, so checking the existence of the other module is trivial.

+1 for adding getWeight() and the use case of relative weights.
#34 worked for me

Minor nits since 8.5.0 is released now, needs updated docs and re-roll against 8.6:

+  @trigger_error('module_set_weight() is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0.', E_USER_DEPRECATED);
+  \Drupal::moduleHandler()->setWeight($module, $weight);

+ * @deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0.
+ *

Changed 8.5.0 to 8.6.0 in two places, no other changes.

@aaronbauman: Maybe you can RTBC this if there's nothing else?

I didn't see in the thread: do we not need additional tests for this?
+1 RTBC from me, but I'm jumping in late so I'll let someone else switch the status.

The "Needs tests" tag was added in #18 because none of the proposed patches before then had test cases.

The patch in #37 (and previous patches back to and including #32) DO have test cases which test both setWeight() and getWeight(),so the tag is no longer relevant.

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -563,6 +563,59 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
+  public function setWeight($module, $weight) {
+    $extension_config = \Drupal::configFactory()->getEditable('core.extension');
+    if ($extension_config->get("module.$module") !== NULL) {

It feels like moving this to the Module Installer seems to better bet.

I think it should be in the ModuleHandler for several reasons:

The ModuleInstaller has a very specific functionality - it *only* installs and uninstalls modules. Period. ModuleInstallerInterface:: defines these methods, and nothing else:

• addUninstallValidator()
• install()
• uninstall()
• validateUninstall()

ModuleInstaller does NOT deal with any metadata about the modules being used - to find out what modules are installed or anything else about modules, we currently use the ModuleHandler.

Some of the many methods of ModuleHandlerInterface::

• getModuleList() - Returns the list of currently active modules.
• getName() - Gets the human readable name of a given module.
• moduleExists() - Determines whether a given module is enabled.
• setModuleList() - Sets an explicit list of currently active modules.

Additionally, the use case I presented in #34, which I believe to be the primary way getWeight()/setWeight() will be used, is this:

// Make sure we get loaded after 'othermodule'.
if ($module_handler->moduleExists('othermodule')) {
  $module_handler->setWeight('mymodule', $module_handler->getWeight('othermodule') + 1);
}

This use case needs to call ModuleHandler::moduleExists(). If setWeight()/getWeight() are in the same class as moduleExists(), we only need the reference to ModuleHandler to perform this task. If they are in ModuleInstaller instead, then you need a reference to both ModuleHandler and ModuleInstaller to perform this task. This tells me that setWeight()/getWeight() are more related to the existing ModuleHandler functionality.

If there is some consensus for putting this in ModuleInstaller, I will rewrite the patch. But for now I think the correct place to put this is in ModuleHandler.

The reason why I suggested to move it to the module installer are the following:

1. It needs the module installer ...
2. It is an operation you do on install time. The module handler is used on runtime instead.

setModuleList() - Sets an explicit list of currently active modules.

I hope we are able to drop this feature in the module. It is a weird edge case feature.

Note: The module installer also didn't use to exist before.

@TR

I tested the \Drupal::moduleHandler()->setWeight($module_name, $weight); in one of my modules and it works well.
Great work.

I'm in favour of working on deprecating the entire concept of module weight. I think module weight should be an internal detail between itself and the module system. Ideally with autoloading the concept of module weight can eventually be deprecated. If any code is actually relying on this then it really is a code smell. We should declare dependencies properly and order the module list by that instead. And if you still have something that is not solved by that then you have hook_module_implements_alter()

Yes the above might only be possible in D9 but I don't think that means we should "improve" the API now. We should try to put everything in place to remove it.

If we must do this then...

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -563,6 +563,59 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
+  /**
+   * Gets weight of a particular module.
+   *
+   * @param string $module
+   *   The name of the module (without the .module extension).
+   *
+   * @return int|null
+   *   An integer representing the weight of the module, or NULL if the named
+   *   module is not installed.
+   */
+  public function getWeight($module) {
+    $extension_config = \Drupal::configFactory()->get('core.extension');
+    return $extension_config->get("module.$module");
+  }

Let's not add API which is only used in test of itself. The fact we've got away without a getter for five years shows us something.

I've opened #2968232: Deprecate module_set_weight() to see how far we can get with Drupal 8 for #44.

If we're going to refactor the module weighting system I think we should change setting and getting of weight to be a config activity. So for example to get a weight you do:

$extension_config = \Drupal::configFactory()->get('core.extension');
$weight = $extension_config->get("module.$module");

and to set a weight we do

$extension_config = \Drupal::configFactory()->getEditable('core.extension');
$extension_config->set("module.$module", $weight);

The actually sorting of the module list according to weight can go into wherever it needs to. ModuleInstaller / ModuleHandler / Kernel. Ideally they would be a config listener that would do the module sorting for the module handler etc... if module weights changed and nothing was installed or uninstalled (as those cases need to be handled by the ModuleInstaller). This would mean that editing the core.extension config via Drush or the Config single import / export would have a chance to actually work as expected.

+++ b/core/includes/module.inc
@@ -170,35 +170,16 @@ function drupal_required_modules() {
+ * @deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0.
...
+  @trigger_error('module_set_weight() is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0.', E_USER_DEPRECATED);

needs work for 9.4.x and proper message

I realize this thread is old - somewhat - but in the real world for two upgraded modules - uc_addresses and uc_extra_fields_pane on Drupal 8.9.20 the function at the head of this post exists in the Drupal 7 install files. So I used

$weight = \Drupal::moduleHandler()->getWeight('uc_addresses', '=');
\Drupal::moduleHandler()->setWeight($weight, + 10);

for uc_extra_fields_pane and at first it worked then on uninstall threw an unsupported method error and I had to change this into

$weight = \Drupal::moduleHandler()->schema()->getWeight('uc_addresses', '=');
\Drupal::moduleHandler()->schema()->setWeight($weight, + 10);

to uninstall and install it again the schema()-> caused an error and had to be removed. It does the same thing in uc_addresses install which is instead

$weight = \Drupal::moduleHandler()->getWeight('uc_quotes', '=');
\Drupal::moduleHandler()->setWeight($weight, + 1);

Am I doing something wrong, or are there thoughts on how this is now to be used ??

(I am still on 8.9.20 to use the Upgrade Status module with Upgrade Rector to solve all module issues of the entire site before upgrading so it will work on Drupal 9 as I have a Drupal 9 site I can switch to by changing the directory name with the same database and it was found the Drupal 9 site would park on an error if certain modules were not fixed because an API call was depreciated AND removed in Drupal 9 so effectively hosing the site)

ATM there's modules extension list as primary source of truth so I think it should be closed in favour of #2968232: Deprecate module_set_weight()
The reason is that most of time modules need to order specific hooks but not the modules' list

Makes sense to transfer credits to new issue and close this one

Moreover module list should be moved out of handler
- #2941155: ModuleHandler should not maintain list of installed modules now that ModuleExtensionList exists
- #3179546: Tag ExtensionList services with extension.list