Problem/Motivation

module.inc contains two functions related to module weight and configuration.

Let's deprecate these and move them to a small utility service.

I think eventually we would want to deprecate weights, but it's tricky, how do you remove a weight. Also it is one of the only ways to order all hooks globally for a module.

Steps to reproduce

N/A

Proposed resolution

Create small utility class ModuleWeight
Add set and sort methods.

Remaining tasks

Review

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#17 3595652-nr-bot_05igrb4l.txt91 bytesneeds-review-queue-bot
#8 3595652-nr-bot_sd1dq_jr.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3595652

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

nicxvan opened merge request !16032

nicxvan’s picture

nicxvan commented
Status: Active » Needs review

This is ready for review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Did a search for:
module_set_weight: all references replaced
module_config_sort: all references replaced

Since this was 2 wonder if it would of been disruptive to remove in 12? But will defer to your judgement.

LGTM

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue tags: +11.5.0 release priority

Adding 11.5.0 release priority since it would be great to get this in there too.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3595652-nr-bot_sd1dq_jr.txt91 bytes

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.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Wouldn't be more appropriate to add the methods to module handler instead of a new service?

nicxvan’s picture

nicxvan commented

No, moduleHandler already does a lot, and most of the places using miduleweight and sort don't already have moduleHandler injected, so we might as well keep it lightweight.

Still needs a rebase.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Rebased

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Parent issue: » #3097045: [META] Provide modern replacements for and deprecate the legacy include files
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

A lot is already happening in

There is also #1808132: Move module_set_weight() into ModuleHandler::setWeight(), add ModuleHandler::getWeight() to replace missing functionality which has been closed in favor of the above.

I think we need to align. Maybe close this or the others. No idea for the moment...

nicxvan’s picture

nicxvan commented

I think it's worth doing these together, if this gets in we can close the others and provide credit there.

I took them into account, I think we need to keep weight for now even if we want to get rid of it someday since it's the only way to globally order hooks for a module.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Restoring status from bot kicking back for rebase.

Should any of the other issues be closed and credit moved over? Happy to do that.

nicxvan’s picture

nicxvan commented

All closed issues can grant credit so I'll take care of the other ones when this lands.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3595652-nr-bot_05igrb4l.txt91 bytes

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.

nicxvan’s picture

nicxvan commented
Status: Needs work » Reviewed & tested by the community

Rebased, the conflict was the auto uninstall profile issue moved the profile weight into an if.

  • catch committed 0c3247ed on main 
    task: #3595652 Deprecate module.inc contents

By: nicxvan
By: claudiu....
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I had a look at #3262807: Deprecate module_config_sort(), it adds a generic sorting method which is then used by the module handler. I'm not sure we should add that, because we actually want to deprecate module weights, and then there'd be no usages in core. I think we should potentially repurpose that one for 'add a generic method that sorts by keys and values' though. But for me this means the approach of just moving the logic as in this issue makes sense.

#2968232: Deprecate module_set_weight() is about deprecating module weights, not the method, and should stay open.

Committed/pushed to main, thanks!

Will need a backport MR for 11.x

catch closed merge request !16032

nicxvan opened merge request !16322

nicxvan’s picture

nicxvan commented
Status: Patch (to be ported) » Reviewed & tested by the community

There were three conflicts, none of them complex:

ModuleInstaller, main has removed the uninstall validator so we had to manually add the ModuleWeight argument back and make it nullable explicitly.
Content translation lines changed so the replacement of the set weight call had to be redone, and the use statement had to be readded to the installer config directory test base.

  • longwave committed daf0e8e3 on 11.x 
    task: #3595652 Deprecate module.inc contents

By: nicxvan
By: claudiu....
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Backport looks correct to me.

Committed and pushed daf0e8e3a1e to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

longwave closed merge request !16322