Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up to #2324055: Split up the module manager into runtime information and extension information.
Problem/Motivation
The ModuleInstaller
is littered with calls to \Drupal::service
.
Proposed resolution
Properly inject services and ensure that instances are reinjected after every container rebuild.
Remaining tasks
User interface changes
API changes
Beta phase evaluation
Issue priority | Major because dependency management in low level system components allows us to discover system fragilities that are otherwise superhard to find. |
---|---|
Prioritized changes | This is prioritised because it is a followup from #2324055: Split up the module manager into runtime information and extension information. |
Disruption | Not disruptive since the module_installer is a new service and injecting its dependencies should have no impact on calling code. |
Comment | File | Size | Author |
---|---|---|---|
#129 | 2380293-REVERT2.patch | 17.85 KB | mpdonadio |
#125 | 2380293-REVERT.patch | 17.69 KB | mpdonadio |
#113 | interdiff.txt | 1.61 KB | dawehner |
#113 | 2380293-112.patch | 17.74 KB | dawehner |
#108 | interdiff.txt | 1.87 KB | dawehner |
Comments
Comment #1
znerol CreditAttribution: znerol commentedComment #2
alexpottComment #3
dawehnerLet's see whether this works ...
Comment #5
dawehnerThere we go.
Comment #6
BerdirLooks great.
Having a logger named 'system' in core.services.yml is a bit weird...
Not sure if we should change it here, but there's nothing that mandates that channel must be a module name. Something like modules or so would make more sense to me.
Long list.. wondering if we want to use a pattern similar to DependencySerializationTrait, loop over properties and look for _serviceId and then update?
Comment #7
dawehnerTried that in #2324055-53: Split up the module manager into runtime information and extension information. already, let's see whether this works better this time.
Comment #10
dawehnerMaybe this works?
Comment #12
BerdirI think the kernel doesn't actually change?
But the issue about improving Container::get() would fix that, one way or another, so we should get back to that :)
Comment #13
dawehnerRerolled #7 and read the failure.
Comment #15
dawehnerdoh!
Comment #16
BerdirThis looks great to me.
We discussed synthetic services in #2307869: Remove Drupal's Container::get(), we can consider to implement setting the _serviceId there in set(), as we've discussed already. Or keep it like this.
Comment #17
jibranI find some minor issues. Uploaded the patch with the fixes.
space missing.
Re-injection is a proper word so I think the trait name should be
DependencyReInjectionTrait
.re-fetch
Also can we do this?
Comment #18
jibranWith patch.
Comment #20
BerdirYou didn't rename the file. Also, reinjection is a word: http://www.merriam-webster.com/medical/reinjection
Comment #21
jibranThanks for pointing that out renamed the file.
medicinal word not English word :D
Comment #22
dawehnerWell ... http://en.wikipedia.org/wiki/Gas_reinjection for example exists
Comment #23
jibranLet's not bikeshed here. Reverted back to Reinjection. RTBC as per #16.
Comment #24
alexpottHow about UpdateDependenciesTrait? Since that is the name of the method it provides. It is in the dependency injection namespace so that is probably enough about injection.
it => them
This should have more docs about when to use this and when not to.
Comment #25
dawehnerAgreed, your name is better. Addressed the feedback
Comment #26
jibranThanks for fixing the feedback. The interdiff doesn't include renaming :P but anyways RTBC
minor issue can be fixed on commit.
Comment #27
alexpottComment #28
dawehnerDoesn't the kernel and class loader both don't have it either? Would be worth to quickly research.
newline :P
Comment #29
alexpottre #28.1
The kernel comes through
get()
so it will get a _serviceId if your service depends on it. Same if your service depends on the class_loader.I think more problematic are container parameters. Add some documentation about what to do then. So the question now is - it is feasible for
app.root
to change during a module install. I don't think so because I can't see anyway a module could successfully swap out the kernel.fixed #28.2 - is this actually a standard?
Comment #30
dawehnerChecked it, we also have the property set on the class loader ...
Take that: https://www.drupal.org/node/608152 :P
Comment #31
dawehnerJust a reroll.
Comment #33
dawehnerInteresting ... so yeah we don't support lazy classes yet actually.
Comment #38
dawehnerAnyone wants to reroll this one?
Comment #39
AjitSMe ;-)
Comment #40
mpdonadioApplied patch to 8e0bcfef0870e916eb9b8b07ad35f26e98bad486 and rebased against a fresh pull of 8.0.x. Six merge conflicts in ModuleInstaller, but I think this is good.
Comment #41
AjitSSaw the tweet a little late ;-)
Comment #43
dawehnerYeah, we need the router.builder service, or somehow named like that, now.
Comment #44
mpdonadioLet's try this.
Comment #47
mpdonadioGot a
Retesting.
Comment #49
willzyx CreditAttribution: willzyx commentedComment #51
mpdonadio#2090115: Don't install a module when its default configuration has unmet dependencies changed \Drupal\Core\Config\ConfigInstaller::findPreExistingConfiguration() from a public method to a protected one, and removed it from the interface. According to that, we need to refactor to use ::checkConfigurationToInstall() instead, but we are already calling that a few lines up?
Comment #52
mpdonadioI am waiting on a TestBot run to finish, but I took a look at #2090115: Don't install a module when its default configuration has unmet dependencies, and think the reroll has some extra hunks in it that can come out. If that passes, I will post it here.
Comment #53
mpdonadioLooking at #2090115: Don't install a module when its default configuration has unmet dependencies, it looks like these two hunks snuck back in. Still have failures. When I have time, I'll look at the patch in #3 side by side with this one to see if anything else pops out.
Comment #55
willzyx CreditAttribution: willzyx commentedthe code in the patch is the exact equivalent (with injected services) of the code of HEAD but the tests still fail. Code looks good the problem is not in it.. Seems we have some problems with lazy-loaded services (router.builder and config.installer) and that the services proxy does not work properly
Comment #56
jibranComment #58
willzyx CreditAttribution: willzyx commentedNow that #2408371: Proxies of module interfaces don't work is in, see if anything changes
Comment #61
willzyx CreditAttribution: willzyx commentedit should pass. After #2408371: Proxies of module interfaces don't work it should work properly
Comment #63
willzyx CreditAttribution: willzyx commented#61 is a good example of why we should avoid faulty dependency on concrete classes and use interfaces :/
Comment #64
dawehnerOH yeah, we just recently introduced that interface ...
Comment #65
willzyx CreditAttribution: willzyx commentedfinally the patch is green.. is this ready to go?
Comment #66
znerol CreditAttribution: znerol commentedIt looks like
ConfigImporter
could useUpdateDependenciesTrait
and then replacereInjectMe()
withupdateDependencies()
.Comment #70
dawehner.
Comment #71
willzyx CreditAttribution: willzyx commentedrerolled after #2392559: Remove all uses of file_stream_wrapper_get_* and file_get_stream_wrappers
Comment #72
willzyx CreditAttribution: willzyx commentedrerolled after #2514044: Do not use SafeMarkup::format in exceptions
Comment #73
dawehnerThis patch provides some progress, let's do it.
Comment #74
plachThe entity manager was not injected on purpose to avoid unnecessary API changes when doing #2350111: Introduce an event to decouple the module installer from the entity manager.
Comment #75
dawehner@plach
I'm sorry but constructors aren't APIs. If you think they are, things are wrong.
Comment #76
dawehnerMh, maybe I just don't get your point.
Comment #77
plachCall it API or not, but when doing #2350111: Introduce an event to decouple the module installer from the entity manager we will have to remove that parameter again from the constructor, which is an unnecessary change.
Comment #78
dawehnerWell, we don't HAVE to right, if we care about the removal at that point?
Comment #79
dawehnerIt would be great if we can make babysteps and actual progress.
Comment #80
plachFeel free to proceed here, but I really don't see the point of injecting the entity manager.
Comment #81
dawehnerWell, I totally agree with you that the design of that class is wrong. It just does too much. But you know, better inject and be honest about it, than to lie.
Comment #83
mpdonadioThis needs a reroll, I think b/c #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state, but I didn't feel comfortable with how to handle the conflict in ModuleInstaller().
Comment #84
willzyx CreditAttribution: willzyx commentedRerolled and injected also the entity.definition_update_manager service. I can't provide interdiff.txt because interdiff crashes when I try to produce an interdiff file for this patch, sorry.
Comment #86
willzyx CreditAttribution: willzyx commentedboth of the failures are related to out of memory (see log). Probably avoid to inject entity.manager and entity.definition_update_manager services (like in HEAD) can limit the memory usage (and also should help with #2350111: Introduce an event to decouple the module installer from the entity manager)
Comment #88
Nikolay ShapovalovComment #89
almaudoh CreditAttribution: almaudoh commentedComment #90
cilefen CreditAttribution: cilefen commentedThis is a simple reroll against 8.1.x. Note that there are still \Drupal::'s in ModuleInstaller that need refactoring. This class has a large list of dependencies in its current form.
Comment #92
dawehnerI really like how this is looking right now.
Comment #94
dawehnerLet's reroll it against
8.2.x
. I think at this time, it should be part of8.2.x
onlyComment #95
mpdonadioFor anyone looking at tackling this, it isn't a straightforward rebase/merge. There are a decent amount of weird conflicts in ModuleInstaller, but I didn't start to dig into which issues mucked this up.
Comment #96
willzyx CreditAttribution: willzyx commentedrerolled
Comment #97
mpdonadioCompared the two patches, and this looks like a good reroll.
Comment #98
dawehnerIt is great to see this patch moving forward.
Let's drop that quickly ...
For some reason we never mock the container, but just create a new instance of it. This makes it easier to read IMHO
IMHO we should provide a concrete class using that trait, which provides a public method we can call. It feels more realistic than a mock.
Comment #99
damiankloip CreditAttribution: damiankloip at Tag1 Consulting commentedLet's give dawehner what he wants.
Comment #100
damiankloip CreditAttribution: damiankloip at Tag1 Consulting commentedAs a side note, the amount of dependencies here is absurd. A true indication that we should pursue the issue to split the module handler.
Comment #101
dawehnerThank you damian!
Well, this is the module installer only now ... but yeah this is why we for example want to have a module installation event to not have to deal with all those things directly in there.
In general though its just about being honest.
Can be dropped on commit
Comment #102
dawehner.
Comment #103
damiankloip CreditAttribution: damiankloip at Tag1 Consulting commentedYeah, you mean https://www.drupal.org/node/2206347
Comment #104
dawehnerExactly!
Comment #105
alexpottI think we need a CR for this change of where stuff is being logged. The log channel is changing.
Also need to fix a couple of minor coding standards things...
Comment #106
dawehnerNice!
We agreed that we should reintroduce the system one and not use a new one. Added a follow up on #2721289: Potentially rename the system logging channel to core to deal with that.
Comment #107
alexpottComment #108
dawehnerComment #111
alexpottNeeds to revert this change.
Comment #112
mpdonadioPasses locally.
Comment #113
dawehnerThank you @mpdonadio!
Comment #114
alexpottCommitted 5efda3c and pushed to 8.2.x. Thanks!
Comment #116
neclimdulWoops... double declaring the arguments. Broke PECL YAML, follow up. #2721741: Fix double argument declaration in core.service.yml
Comment #117
neclimdulSo I slept on it and I can't think of a way to undo the changes in this patch that are now blocking a major performance bug. #2531564: Fix leaky and brittle container serialization solution
So since this is a task and that is a bug one option is reverting this issue. However there is clearly a lot of awesome work here and I'm definitely one for injecting requirements so I'm open for discussions on how we can resolve this.
Comment #118
dawehner#2389193: Provide a way to determine whether a given instance was retrieved from the container could really help with that as well ...
Comment #119
neclimdulPersonally the idea that a class cares _how_ it got its dependencies injected is terrifying.
Comment #120
tstoecklerNot sure if this is related to #117 / #188 but this seems to have majorly regressed the memory usage when installing via Drush.
I ran
drush --verbose --debug si -y
before and after this commit and before the commit I have 52.55 MB and after I have 166.26 MB. I ran both twice with the same results.Hat tip to @lokapujya for finding this!
Comment #121
cilefen CreditAttribution: cilefen commentedWhat about in a UI install?
Comment #122
neclimdulThe route builder, config manager, key value factory and uninstall validators(surprise!) are only used in uninstall so they probably weren't being created before. That's a surprising amount of memory for those but that would be my guess as to the memory issue.
Since Drush just calls install_drupal() the same as the UI installer, if we bisect the memory regression to this commit it almost certainly behaves the same.
Comment #123
cilefen CreditAttribution: cilefen commentedOh, some dependencies are not lazy, so they and all their dependencies and so on, are built.
Comment #124
alexpottCan someone roll a revert of this patch. I think we should revert and continue in this issue.
Comment #125
mpdonadioComment #127
mpdonadioConfused. I did a `git apply --reverse 2380293-112.patch` w/ some hand edits to resolve conflicts, and ended up with the exact same patch to back this out.
Comment #128
Mile23The reversion probably doesn't deal with the change to the services file due to duplicate entries in a later commit.
That said: I think this should stick around a little while, and we can fix things in the dependencies that need fixing. If 8.2.x weren't dev, then we'd need to revert.
Also, there are still 20 occurrences of \Drupal:: in ModuleInstaller.
Also: \Drupal is an antipattern. :-)
Comment #129
mpdonadioThink the followup is the problem.
Comment #130
mpdonadioLet's inject the router builder.
We don't need the router builder!
This is present up through d7b430a.
Comment #131
dawehnerI totally agree that this patch should be reverted.
For me the next two parallel steps are:
Comment #132
alexpottReverted 2c22ebb and pushed to 8.2.x. Thanks!
Comment #134
neclimdulre: #130, ha! I guess it probably isn't the router.builder that's the leak then unless the container is smart enough to not instantiate unused parameters. ;)
re: #131 After 1, or possibly as part of it, we could inject the container and manage the "injection" of the uninstall services internally. That would allow it to us the same logic to "reinject" services during the the uninstalls. It is still an anti-pattern but it can probably leverage most of the work done here and gets everything in the right place for further refactors.
Another possibility, though its really hard to grok with the complexity of the uninstall method, would be to have a specific "uninstaller" object that we could repeatedly instantiate and inject rebuilt services into. Then we make
ModuleInstaller::uninstall()
only do some precalculations and instantiate/run those objects.I think at some level we'll have to accept a level of anti-pattern here because loops preserve state and we are looping over global state changes. We have to do something to work around that and I don't think we can get caught trying to make this perfect.
Comment #135
tstoecklerAdding #2495411: Make simpletest fail a test when it detects pages that need more than 64MB as related, as I'm assuming that would have caught this problem.
Comment #136
Mile23Adding parent: #2729597: [meta] Replace \Drupal with injected services where appropriate in core
Comment #140
Mile23This issue could happen in pieces here: #2729597: [meta] Replace \Drupal with injected services where appropriate in core
ModuleInstaller
has calls to\Drupal::logger()
and\Drupal::keyValue()
and others that could be replaced one at a time along with all other usages.Comment #146
jibran#2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList is in. Can this be fixed now?
Comment #147
dawehnerWe can try. I believe the fundamental challenges might still apply :)
Comment #149
markhalliwellMaybe move/close as a dup of #3023131: [PP-1] Extension System, Part IV: ExtensionHandler and ExtensionHandlerInterface?
Comment #151
mxr576FYI, I added this issue as related to #3056604 because in my patch I had to change when
\Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions();
gets called when a module is uninstalled.Comment #153
alexpottNote that injecting a new service into the module installer caused a memory leak - see #3126003: Increased memory usage during install
Comment #154
alexpottComment #160
larowlanClosed related issue as a duplicate - applying credit