Problem/Motivation

In #3416522: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller we installing modules without rebuilding the container all the time. To do this we need to be able to reset the container so that any instantiated services can be re-instantiated. Unfortunately this runs into 2 problems:

  • \Drupal\Component\DependencyInjection\Container::reset() doesn't recreate the 'service_container' entry that was added in the constructor
  • We need an equivalent of \Drupal\Core\DrupalKernel::initializeContainer() that copies stuff from the old container to the new container - think synthetic services, logged in user and session.

Proposed resolution

  • Fix Container::reset() by not settings the circular reference but doing what Symfony does and adding hard coded 'service_container' in a few places
  • Add DrupalKernel::resetContainer()

Remaining tasks

User interface changes

None

API changes

New \Drupal\Core\DrupalKernel::resetContainer()

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#14 3419987-nr-bot.txt3.93 KBneeds-review-queue-bot

Issue fork drupal-3419987

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

alexpott created an issue. See original summary.

alexpott opened merge request !6499

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Normal » Major

Making this major as it is part of #3416522: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs change record

Literally zero remarks on the MR.

All this needs now is a change record (for the few souls that have a custom DrupalKernelInterface implementation).

I'd also like to see a test-only run, but I can't trigger that — could you please do that, @alexpott? 🙏

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +blocker

Tagging for blocking #3416522: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Created a CR and triggered a test only run.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Test-only job reports:

There was 1 error:
1) Drupal\Tests\Component\DependencyInjection\ContainerTest::testReset
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "service_container".

👍 🚢

dww’s picture

dww
we/he/they
commented

Agreed that this looks great. Almost nothing to complain about in the MR, title, summary, or the CR. Really excited about the issue this is blocking! 🎉

I opened a few MR threads for optional questions to address, but this is ready as-is. +1 RTBC.

Thanks!
-Derek

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Could you trigger the test-only job again now, to ensure that also after addressing @dww's review the test coverage still does what we think it does? 🙏

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@Wim Leers done.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

Left a couple of questions

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

Addressed @larowlan's feedback - thanks for the review!

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3419987-nr-bot.txt3.93 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

Fixed MR. It'd be great to land this so I can back to the issue this blocks.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Tried to do a thorough review here, because would love to see the blocked issue unblocked, but … IDK enough about this part of core to RTBC this. Hopefully @larowlan does another review 🤞

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

Thanks for adding the new test @alexpott - I think this is good to go now.

alexpott opened merge request !6829

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Merged 11.x and resolved use statement conflict in core/lib/Drupal/Core/DrupalKernelInterface.php due to #3424177: Remove ContainerAwareInterface from DrupalKernelInterface
Opened a 10.3.x MR so we can backport this there.

  • catch committed cfd8eeca on 10.3.x 
    Issue #3419987 by alexpott, Wim Leers, larowlan, dww: Fix Container::...

  • catch committed 241cb51c on 11.x 
    Issue #3419987 by alexpott, Wim Leers, larowlan, dww: Fix Container::...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Also can't find anything to complain about and the other issue will prove this is doing what it's supposed to on top of the extra test coverage here.

Committed/pushed to 11.x and 10.3.x respectively, thanks!

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Published the CR

dww’s picture

dww
we/he/they
commented
Version: 10.3.x-dev » 11.x-dev
Status: Fixed » Needs review

Thanks! Saving credits to match commit.

dww’s picture

dww
we/he/they
commented
Version: 11.x-dev » 10.3.x-dev
Status: Needs review » Fixed

Sorry, stale form values.

Status: Fixed » Closed (fixed)

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