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.