Wondering why there is no corresponding module_get_weight() for module_set_weight(). The typical use case is to set my module's weight to one higher than another module, but I don't necessarily know what the weight of the other module is:

  $weight = (int) db_query("SELECT weight FROM {system} WHERE name = 'dependency'")->fetchField();
  db_update('system')
    ->fields(array('weight' => $weight + 1)) 
    ->condition('name', 'mymodule')
    ->execute();

I don't see a way to possibly do this with the newly-converted code.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

plach’s picture

Status: Needs review » Reviewed & tested by the community

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.

plach’s picture

Status: Reviewed & tested by the community » Needs review
sun’s picture

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.

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.

Crell’s picture

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.

sun’s picture

Sorry, but the time for "waiting" is over. ;)

Crell’s picture

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()).

webchick’s picture

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.

plach’s picture

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?

plach’s picture

FileSize
1.07 KB
PASSED: [[SimpleTest]]: [MySQL] 59,880 pass(es). View

What about this?

sun’s picture

Throwing an exception looks overboard to me. Returning FALSE should be sufficient.

Compare:

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

vs.

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

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.

plach’s picture

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).

leschekfm’s picture

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

jibran’s picture

alansaviolobo’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
1.05 KB

could use some help with this.

Crell’s picture

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.

alansaviolobo’s picture

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

tim.plunkett’s picture

Issue tags: +Needs tests

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

markdorison’s picture

Version: 8.1.x-dev » 8.3.x-dev
markdorison’s picture

Status: Needs work » Needs review
FileSize
1.04 KB

Updated to use core.extension.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

TR’s picture

Category: Feature request » Bug report
Priority: Normal » Major
Issue tags: +regression

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.

tim.plunkett’s picture

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

TR’s picture

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 ...

Status: Needs review » Needs work

The last submitted patch, 26: 1808132-26.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

TR’s picture

Status: Needs work » Needs review
FileSize
13.85 KB

Corrected the missing public on the two new methods.

Status: Needs review » Needs work

The last submitted patch, 28: 1808132-28.patch, failed testing. View results

TR’s picture

Status: Needs work » Needs review

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.

tim.plunkett’s picture

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

TR’s picture

dawehner’s picture

  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?

TR’s picture

Title: Complement module_set_weight() with module_get_weight() » Move module_set_weight() into ModuleHandler::setWeight(), add ModuleHandler::getWeight() to replace missing functionality
FileSize
13.08 KB

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.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.