Investigate ModuleHandler::add

nicxvan → changed the visibility of the branch 3481778-investigate-modulehandleradd to hidden.

Back to NW for the 2 remarks on the MR.

I think I answered your concerns, let me know if you have any further questions.

My questions have been answered.
All code changes look good to me.
The IS and the CR are in order.
For me it is RTBC.

Thanks for working on this!
GitLab thinks this needs a rebase due to conflicts.
Meanwhile, I opened some MR threads, mostly with pedantic nits.

Yeah I saw the conflict but when I went to rebase it was gone, maybe an issue was reverted I'll fix it and address your comments.

Rebase is werd I'll look tomorrow

Rebased and comments addressed.

The functional test fails in https://git.drupalcode.org/issue/drupal-3481778/-/pipelines/367140 are random, but the fail in Unit is legit. Opened MR threads with suggestions.

Almost there, thanks!
-Derek

Whereas:

1. All feedback addressed.
2. Pipeline 367282 is green.
3. This is my first time experimenting with requesting changes. Even though I've got somewhat elevated perms in GitLab MRs for core as a subsystem maintainer, I can't actually resolve that request (even though I made it myself). Good to know, I won't be using that feature anymore. The committer can ignore the "Merge blocked: 1 check failed" error on the MR.
4. Tweaked some formatting in the summary.
5. Initial pass at saving credit.
6. I don't mind RTBC'ing this, even though I pushed a trivial commit to update the formatting of the deprecation message.

Therefore, RTBC!

Thanks again,
-Derek

This looks like a nice cleanup, added a couple of minor questions to the MR.

I replied to the comments that I could not address.

Follow up created and all comments addressed.

Resolved all threads

When looking at the CR though it wasn't super clear, maybe something like

ModuleHandler::addModule and ModuleHandler::addProfile have been deprecated. There is no direct replacement. (can the reason why be mentioned?)

Adding modules or profiles run time is not supported. Use ModuleInstallerInterface::install instead.

before/after implementations or least an after of how one would use ModuleInstallerInterface::install

Updated the CR.

Thanks!

CR reads much better. I believe all feedback for this one is complete. Fingers crossed.

I left some questions in the MR. The change record before section should be for 'addModule' and 'addProfile' not 'add'. Tagging for updates.

@quiteone RE: #30, updated the CR. Is the new version closer?

Dealt with 2 of 3 comments in the MR. Rebased.

I will revert one of your changes you removed the deprecation on module handler add.

I'll address the remaining comment too it just needs to be more imperative.

@nicxvan Ah, I see. Thank you for fixing.

@quietone, I don't think we can change the return type since it's inherit doc and the parent has return type of array|false?

Is that ok?

nicxvan → changed the visibility of the branch 11.x to hidden.

I rebased on head, the last comment was addressed upstream so this should be ready.

CR does appear to be updated so removing that tag.

Checked the MR and all threads do appear to be addressed, even went back and read some older ones.

Feel this could be considered a 11.2 release priority (remove if I'm wrong)

Believe this one is good to go.

This looks good but one small question.

This should be ready now.

Feedback addressed, and I gave it a once over myself, this looks good to go.

I'm not sure it's necessary. The only way I could see it being an issue is in the later stages of installation if a profile changed the weight of the system module in a hook_system_info_alter maybe? Which seems like a really bad idea. Could go with the following I guess, just to be sure.

Yeah I agree it'd be a horrible idea, but it just made me think about it whereas conditionally setting it doesn't.

Looks good. Committed/pushed to 11.x, thanks!

catch → closed merge request !10365

Automatically closed - issue fixed for 2 weeks with no activity.