Problem/Motivation
Over in #3074585-62: [Symfony 5] Replace app.root and site.path string services with container parameters we discovered that during compiling a container we're falling back to DRUPAL_ROOT when getting extension objects in \Drupal\Core\Extension\ExtensionDiscovery::scanDirectory() because that happens very very early when compiling a container. We shouldn't be falling back to a global and also the ExtensionDiscovery class needs the app root to work so it can take care of creating extension objects with the correct app root. Furthermore extension object serialisation has caused bugs in the past and this is extremely low level so anything we can do to decouple the cache entry from the code is welcome.
Proposed resolution
We use file cache here because we have to read the .info.yml file to check the type and that takes time. Instead of caching the full extension object let's cache the arguments - apart from the app root - that we use to create the object.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#16 | 3116858-16.patch | 6.18 KB | alexpott |
#16 | 10-16-interdiff.txt | 1.29 KB | alexpott |
#10 | interdiff-3116858-9-10.txt | 1.14 KB | daffie |
#10 | 3116858-10.patch | 6.33 KB | daffie |
Comments
Comment #2
alexpottI guess eventually we should deprecate serialisation of Extension objects since storing the app root on them is a bit fraught.
Comment #3
andypostIt's interesting how core will bootstrap if some module will be moved inside of modules directory...
If extension is cached with path then bootstrap could fail, or I'm wrong?
Comment #4
alexpott@andypost the file path is part of the cache key so you'll get a miss and rebuild the Extension object. So it bootstrap works just fine.
Comment #5
Mile23I was like, didn't we already do this? And then I was like, maybe not, but maybe...
And then I was like, oh yeah... #2881981: Mitigate Extension dependency on DRUPAL_ROOT
Comment #6
daffie CreditAttribution: daffie commentedI have simplefied the patch from @alexpott in comment #2. Hopefully I did not miss something. What I do not understand is why the result of
$this->fileCache->get($fileinfo->getPathName())
needs to be an array. We still need testing for this issue.Comment #7
alexpott@daffie #6 just results in never using the cache - because the cache object is an extension object and you're checking if it is an array.
Re-uploading #2.
And this change doesn't really require tests. We use this file cache millions of times during a test run. We're changing what we store in the cache but we're not changing the output of \Drupal\Core\Extension\ExtensionDiscovery::scanDirectory() - if that breaks every thing breaks.
Comment #8
alexpott@daffie the answer as to why we shouldn't cache extension objects in the file cache is in the issue summary. We are scanning extension directories very very early in container compilation - that's why we have/need the fallback to DRUPAL_ROOT in \Drupal\Core\Extension\Extension::__wakeup(). We shouldn't fallback to DRUPAL_ROOT - therefore we need to construct new extension objects because they are value objects and we can't and shouldn't add a setter for root on the object. Therefore we need to cache the arguments we use to construct the extension object.
Comment #9
alexpottThere is one bit of this that is worth testing because it affects real sites. We need to test the ability of ExtensionDiscovery to cope with pre-existing cache entries that have Extension objects and not arrays in them.
Comment #10
daffie CreditAttribution: daffie commentedWith the added test and the explanation of @alexpott, the makes the issue understandable for me. Thank you for that @alexpott. I have reviewed the patch and changed the comments in the patch. I have made no code changes, therefor I can still review the patch. All code changes look good to me. There is testing for this issue. For me it is RTBC.
Comment #13
alexpottThere was a random fail
Comment #14
catchThe comment should avoid 'previously stored' since this implies some knowledge of the history - we can just explain why we don't store full Extension objects.
Comment #15
alexpott@catch I agree. But for me also #10 misses the point of commenting. The comment in previous patches was in a different place and more relevant. It was:
For me this is important because it details that on sites just after this patch is applied the file cache will contain Extension objects and we need to deal with that in a safe way by checking the type. Most cache gets we do don't have a type check because it doesn't often change - though when it does it is often the source of interesting problems.
Comment #16
alexpottBacking out #10 as per #15 for the reasons stated.
Comment #17
catchAgreed with going back to the comment from #9.
Comment #18
neclimdulI think the location is right but it still seems like its referring to history rather than just being a comment about being defensive about values coming out of the cache.
This is admittedly a pretty late review but why are we using an array? They're convenient for sure but why not a bespoke cache object for filecache that does serialize well and doesn't rely on these services? Haven't looked closely enough to know the benefits or downsides but seemed worth asking since that could maybe own some of the logic from the filecache service and maybe simplify some things. It might pay off with an interface for BC down the road which gets ugly with arrays.
Comment #19
catchBack to needs review for #18.
Comment #20
alexpottI disagree with creating another object to represent a partially parsed extension object. Storing the arguments as an array seems fine to me. There's nothing to be gained here for reuse.
What might be possible and interesting to explore whether or not we should use the info parser here and store the parsed yaml instead of reading the file and looking for
type:
- that way we'd stop reading all the .info.yml twice and if we injected file cache into the info parser then we're speed up cache rebuilds because they'd not have to reparse the extension files.And I also think it is worth trying to work out out how to remove anything other than extension info from the Extension object.
But this a small step in the right direction that reduces dependency on the DRUPAL_ROOT global.
Comment #21
catch#20 seems worth a follow-up. I don't have anything else here that would block commit.
Comment #22
catchOpened the follow-up #3159779: Considering caching parsed YAML for extension discovery.
Committed a7a34f1 and pushed to 9.1.x. Thanks!