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.

Files: 
CommentFileSizeAuthor
#21 complement-1808132-20.patch1.04 KBmarkdorison
#15 complement-1808132-15.patch1.05 KBalansaviolobo
#9 drupal-module_get_weight-1808132-9.patch1.07 KBplach
PASSED: [[SimpleTest]]: [MySQL] 59,880 pass(es). View
drupal8.module-get-weight.0.patch865 bytessun
PASSED: [[SimpleTest]]: [MySQL] 42,094 pass(es). View

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.