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.Updated: Comment 0
Problem/Motivation
A short description of synchronized services:
some special services aren't constructed via DI though they come from outside
as existing instances of the services might already have an instance of that special service injected you can automatically call setters on the other services
so everytime you do $container->set('request', $request); all services with a setRequest are called.
Synchronized services work fine once we have the container on disk, though in a simpletest enviroment we construct one via the ContainerBuild class. Drupal
has overridden the set() method on there in order to not cause failues on already compiled containers, though we skip the code to sync services completly:
if (isset($this->obsoleteDefinitions[$id]) && $this->obsoleteDefinitions[$id]->isSynchronized()) {
$this->synchronize($id);
}
Proposed resolution
- copy over the code for synchronized services
- Also copy some of the tests so we are sure that services are synchronized.
Remaining tasks
User interface changes
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | container_builder-2147451.patch | 3.9 KB | dawehner |
Comments
Comment #1
dawehnerHere is a patch as well as a test. It is not trivial to reuse any code of symfony.
Comment #3
dawehner1: container_builder-2147451.patch queued for re-testing.
Comment #4
chx CreditAttribution: chx commentedUntil we give up this fantasy of working with projects outside of Drupal and just edit them for our betterment we do not have a better recourse here than to copy-paste wads of code. (See https://drupal.org/comment/8291069#comment-8291069 for a better workflow.)
Comment #5
webchickIs there an upstream patch for this?
Comment #6
dawehner@webchick
That is a bit difficult as setting a service on a frozen container is more a bug in our code than in the bug of symfony. Sadly fixing this potentially requires changes all over the place (installer/simpletest).
Comment #7
xjm1: container_builder-2147451.patch queued for re-testing.
Comment #8
webchickOk, after spending some time trawling through a diff of core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php and core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php I think I see. So we're already overriding Symfony's set() method on this class, but haven't copied in the code about frozen containers.
FWIW, it seems like Symfony might've fixed this upstream since we forked ContainerBuilder into core/lib/Drupal. Or at least https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Dep... seems to indicate that it's ok to add services to frozen containers these days. I fired up #2163795: Possible alternative to #2147451? out of curiosity, to see what would happen if we just remove our override of set() altogether instead. It will probably fail.
I'm still mystified, however, as to what the major bug is here and how the test proves we've fixed it. :\ We need to improve the docs here to be explicit about why we're doing this, and how we will know we can remove the hack in the future. (i.e. a @todo pointing to a specific issue about cleaning this up)
Comment #9
dawehner@webchick ... this hunk you look at, was changed when sychronized services got added.
This code still assumes that you CANNOT set arbitrary services on frozen container builders ...
I consider everything on the container as potential major too be honest. The other thin is: tests don't work as expected which cause potentially wrong written tests and so wrong functionality (yes we have seen that on some issue).
Not sure whether I have the motivation to update the patch
Comment #10
dawehnerYou know, we have docs for everything already:
Comment #11
catchI think this is OK. Moving back to webchick for feedback.
Comment #12
webchickI already explained in #8 I have absolutely no idea what this is doing. So either we fix the docs so that I do understand, or some other core committer commits this. :)
Comment #13
webchickDaniel helpfully gave this explanation in IRC:
"You might have services which changes during the runtime of a request (the current logged in user is one example). After changing the logged in user, there might be services with an instance of the old one stored. synchronized services are getting synched/updates automatically by calling some method like setCurrentUser() on all these other services"
I'll see if I can get a quiet moment this evening or tomorrow to parse that and adjust whatever docs accordingly, but I honestly don't think it's worth holding this patch up on me.
Comment #14
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thank.
Comment #15
dawehnerGiven that this just fixed an internal bug there is no real need for a change notice (just wanted to be sure noone asks for one).