Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Symfony 3.4 introduced some performance improvements to \Symfony\Component\DependencyInjection\Container
and in #2927746: Update Symfony components to 3.4.* we've upgraded to 3.4 but we did the minimum changes necessary to make it work. We should look at the changes and improvements in 3.4 and see if we can apply to Drupal.
Proposed resolution
Remaining tasks
User interface changes
None
API changes
to be decided - should be none.
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#22 | 2937010-22.patch | 3.11 KB | andypost |
| |||
#13 | 2937010-10.patch | 3.09 KB | longwave |
Comments
Comment #6
neclimdulI found this rummaging in ContainerBuilder. I'm a little confused about why some methods where left in the previous issue. Though it was probably just that a bunch of patches where being worked on so a final review wasn't done after they all landed.
The methods
ContainerBuilder::callMethod
andContainerBuilder::shareService
are both private in Symfony. #2927746: Update Symfony components to 3.4.* removed the last calls to them inside Drupal which means the only calls to them that can be made are in Symfony. But since symfony's versions of those methods are private so the code on the Drupal container is just dead and not doing anything. Best I can tell we're already using those optimizations... maybe by accident?Anyways, this patch cleans up the dead code in ContainerBuilder. I'm also removing
getProxyInstantiator
because its private and the last call to it was remove in #2726645: Do not @deprecate EntityManager until all usages are removed (helps the Symfony 3 update).Just skimming the file, to my eyes the
// @codingStandardsIgnoreFile
can probably be removed as well because we're not copying any Symfony code anymore.Maybe there's additional optimizations I'm missing but we can get this rolling I think.
Comment #8
longwaveAlso just saw this in ContainerBuilder, I think this counts as part of #3136002: [meta] Remove compatibility shims for Symfony 4.3 and earlier
Comment #9
longwaveSo I think this is probably OK to remove. Even if someone extends ContainerBuilder (and they don't in contrib; http://grep.xnddx.ru/search?text=extends+ContainerBuilder&filename=) would they call these methods?
I also agree that @codingStandardsIgnoreFile can be removed now, as I think can "@todo Submit upstream patches to Symfony to not require these overrides" as there is seemingly nothing useful left for upstream now - I've removed both those in this patch.
Comment #10
longwaveFixed coding standards issue.
Comment #11
alexpottOur says \Drupal\Core\DependencyInjection\ContainerBuilder::set()
It appears this restriction has been fixed in the 4.4 ContainerBuilder so maybe we can call the parent here rather than the grandparent...
Comment #12
longwaveAh yeah it does indeed look like the @todo is fixed upstream.
Comment #13
longwaveReuploading #10, as that failed spectacularly.
The issue is this code in
Symfony\Component\DependencyInjection\ContainerBuilder::get()
:I don't understand enough about the container build process but somehow this then results in an exception when we compile the container:
Comment #14
alexpott#12 is due to
Doing
$container->set('database', NULL);
would now unset it. In our container it'll cause the service to be rebuilt. So this is another difference that at some point we're going to address. Even if we do fix this we'll then hit the problem that we have persistent, non-synthetic services which will cause:during a site install and probably during a module install too.
So I guess punting this further down the track is okay.
Comment #15
alexpottHere's the issue to fix our odd persist thing... #2368263: Remove the persist service tag
Comment #17
longwaveTriggered a retest against 9.3.x.
Comment #20
andypostI bet #11 and #14 should be addressed in #3300306: Drupal ContainerBuilder allows mutation of non-synthetic services
Comment #21
andypostRelated to #3228629: [meta] Bring dependency injection more in line with upstream Symfony component
Comment #22
andypostre-roll #13
Comment #23
Ghost of Drupal Past#14
neclimdul tracked down what happens there with very little help from me. #2531564: Fix leaky and brittle container serialization solution will fix it.
What an incredible, incredible mess.
_serviceId
is set on a service object using the property injection facility.DependencySerializationTraitPass
sets this up (this was the easy part I contributed). If a compiler pass runs before that and creates a service then that service instance will not have_serviceId
and sleeping it might break (this is the hard part neclimdul figured out).The two issues are hopelessly intertwined though because allowing
set
on the compiled container breaks that solution as I noted in #216.So shall we merge these two? The two issues form a circular dependency: you can't kill
set
without killing_serviceId
as shown in #14 here and you can't kill_serviceId
without killingset
as shown in #216 over there.It's possible though that I wanted to post this on #3300306: Drupal ContainerBuilder allows mutation of non-synthetic services.
How many heads does this hydra have?
Comment #24
longwavePersonal opinion is that we should commit #22 here as that is easy cleanup, and then we can solve #23 in a separate issue where the patch will be more focused and slightly smaller.
Comment #25
Ghost of Drupal PastI agree, I commented here because the code snippet we wrestled with today was posted in #14 here but I think the decision was to punt it so this might not be the place for this.
Comment #26
catchLet's get #22 in and continue in the other issues.
Comment #27
alexpottCommitted and pushed 9cd6d55883 to 10.1.x and 2c5031a5c0 to 10.0.x and 00b239dc20 to 9.5.x. Thanks!
Comment #31
andypostRe-rolled follow-up #3300306-12: Drupal ContainerBuilder allows mutation of non-synthetic services