Problem/Motivation
The persist-flag has been removed from many services in #2190665: Remove persist flag from services that do not need it. Citing @berdir from the original issue:
The only reason we made container.namespaces an ArrayObject and a "service" instead of a array/parameter as it was initially was, was to benefit from the persist flag.
Meanwhile there is no service left which needs the persist flag and at the same time depends on @container.namespaces
.
Proposed resolution
Let's remove the persist flag from the @container.namespace
service and also the code from DrupalKernel::initializeContainer
which updates the container namespaces upon rebuilds.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#15 | 2277481-remove-persist-from-container-namespace.diff | 1.19 KB | znerol |
Comments
Comment #1
znerol CreditAttribution: znerol commentedComment #3
znerol CreditAttribution: znerol commentedConvert two additional classes I missed before.
Comment #4
olli CreditAttribution: olli commentedIs it possible to remove persist and keep the service? See #1930020-5: Inject a class into the Core annotated discovery implementation so that namespaces can be updated.
Comment #5
sunComment #6
dawehnerWonderful, it angered me from the beginning that we needed such a workaround.
On top of that the usual developer would have never really understood why it is an object, so this is a developer improvement for everyone, for example, developing a plugin manager.
Comment #7
chx CreditAttribution: chx commentedNot to mention that the Traversable typehint; strictly speakin; was (at least at one point) a lie -- we reset the thing which is not always supported by Traversable things. Arrays are always resetable,of course.
Comment #8
olli CreditAttribution: olli commentedI agree about the array/object. Are we sure the size of the compiled container will not be a problem?
Comment #9
sun@olli: As far as I can see, the %container.namespaces% parameter exists in HEAD already; this patch just removes the additional service that wrapped it.
Further optimizations may be possible, but it would make sense to investigate them in separate follow-up issues.
Comment #10
olli CreditAttribution: olli commentedThe parameter exists only once or twice in the compiled container in HEAD. With this patch it exists multiple times increasing the size in bytes from ~200K to ~500K with standard profile.
Comment #11
dawehnerReally good catch olli!
There have been numbers in the original issue as well, see #1930020-2: Inject a class into the Core annotated discovery implementation so that namespaces can be updated
Given that this scales with the amount of usages of the parameters and the problem of the slowness of reading the container on every request (unless you have PHP)
I guess this is a nogo to fix?
Comment #12
dawehnerI am so out of core development these days, so we certainly need a change record due to the change of the API for every plugin manager developer
Comment #13
znerol CreditAttribution: znerol commentedRe #10: Oh, I did not notice that. It seems this is a bit more complicated than I initially thought. Let us try to answer the following questions before proceeding:
1. Should we try to fix the PHP dumper upstream or should we stick with @container.namespaces service?
The method responsible for generating the parameter code is PhpDumper::dumpParameter(). It might be worthwhile not only for Drupal but also for other bigger projects if there was a way to specify whether a parameter value should be inlined (default) or not when dumping the container. The list of non-inlined parameters could be an option to the
PhpDumper
(least invasive and proper extension point IMHO) or we might try to add that to the ParameterBagInterface (API change, not really the proper extension point because it only would affect thePhpDumper
).I see one problem with adding an option to
PhpDumper
though. It would also be necessary to somehow build up the list of non-inlined parameters.Poll question: In order to prevent the regression pointed out by olli in #10, should we...
a) introduce non-inlined parameters in Symfony and use arrays for the namespaces.
b) stick with
ArrayObject
and optionally try to find a better type-hint.2. Is it save to revert the changes introduced in #1930020 in the first place?
#1930020: Inject a class into the Core annotated discovery implementation so that namespaces can be updated removed the following todo:
Given that the issue at hand would revert much of #1930020, it seems to me that problem described in the comment should reappear. On the other hand it would be possible that other changes committed in the meantime did resolve the original problem. I assume this was caused by instances of
AnnotatedClassDiscovery
magically surviving module enable/disable calls (see #6 in #1930020-6: Inject a class into the Core annotated discovery implementation so that namespaces can be updated).Is it save to revert #1930020: Inject a class into the Core annotated discovery implementation so that namespaces can be updated?
a) Yes, the problem was fixed by issue #...
b) Nope.
Comment #14
znerol CreditAttribution: znerol commentedDigging into question two, trying to get insight into why this patch returned green. I'll outline changes which I think are relevant on the example of
CKEditorPluginManagerTest
(see attachment showing the relevant changesets).CKEditorPluginManager
instance after enabling theckeditor_test
module midways though a test-method. Before the plugin manager always had an up-to-date list of namespaces available fromdrupal_classloader()
. After that change an immutable array of namespaces was injected directly into the constructor of the plugin manager.ArrayObject
and exposing it as a service. Since then the list of available namespaces is updated during kernel rebuilds (initializeKernel
). As a result plugin manager instances do not need to be reinstantiated after enabling a module.It looks like there was the intention to remove the
persist
-flag also from thecontainer.namespaces
service. But @berdir advised against that just before the patch got in #2190665-24: Remove persist flag from services that do not need it. However, the resulting commit still introduced changes in tests, which only make sense if thepersist
flag would have been removed entirely.I think the only reason why #3 passed is because we're lacking test coverage in that area as a result of #2190665.
Comment #15
znerol CreditAttribution: znerol commentedComment #16
znerol CreditAttribution: znerol commentedAs discussed with @berdir on IRC, leaving out the API changes and narrowing the scope to just removing the persist flag from the
container.namespaces
service.Also we probably need to identify a place to document that it is not save to reuse services across container rebuilds. Note that this is not a new thing introduced by this patch. We just remove one workaround implemented at a time when this fact was not yet understood very well.
Comment #17
BerdirMoving forward with that makes sense to me, it solves the actual problem without introducing memory issues or an API change (this will break all contrib plugin managers, I have ~4 affected modules myself, some with 3+ plugin managers, they would all have to be updated for this.
Comment #18
znerol CreditAttribution: znerol commentedFiled #2282371: Document the fact that it is not safe to reuse services across container rebuilds. Given that the API changes are not anymore part of this patch, also removed the tags. From my point of view this is now ready.
Comment #19
BerdirI think this is RTBC.
Comment #20
alexpottCommitted e4802af and pushed to 8.x. Thanks!