Closed (fixed)
Project:
Drupal core
Version:
11.x-dev
Component:
base system
Priority:
Major
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
15 Jan 2024 at 16:44 UTC
Updated:
7 Oct 2024 at 13:43 UTC
Jump to comment: Most recent
Show commands
changes
Comments
Comment #3
wim leersThis is not yet passing tests.
Also: one question on the MR 🤓
Comment #4
longwaveGiven that the primary use of FileCache appears to be YAML parsing, wonder if we should add a
Yaml::parseCacheableFile()or similar method that wraps the parsing in FileCache and then callers don't need to care about implementing it themselves?Comment #5
alexpottComment #6
alexpottI think we should add a CR just in case anyone is doing something like ThemeUiTest where they are re-writing .info.yml in under a second.
Comment #7
alexpottAdded the change record.
Comment #9
alexpottThanks for the review @kim.pepper
Comment #10
wim leersI think this is ready. @longwave's #4 has not gotten an answer yet, so letting him re-check/RTBC this, because it sounds like he thinks this should be implemented differently?
Comment #11
alexpottI think #4 is follow-up material and I think collections are useful - as shown in the test in this issue where we're able to clear the info parser static cache and make it not use a cache backend.
Also, I think this issue is responsible for a considerable speed up in tests. The webservers are only going to have read every extension info.yml once per job. And as we have APCu enabled in cli same for each CLI process. This is likely to the millions of function calls in each job. See how
\Drupal\Core\DrupalKernel::boot()sets the prefix without a site path and we have theapcu_ensure_unique_prefixsetting set to FALSE.Comment #12
wim leersHoly 💩 🤩🤯
You're right!
— https://git.drupalcode.org/project/drupal/-/pipelines/79608
🤯
Comment #13
longwaveAgree #4 can be explored elsewhere.
Discussed in Slack with @alexpott and we think that instead of forcing a cache invalidation in the trait we can just force the mtime of the file to be one second newer than the previous mtime, which should trigger the cache to invalidate by itself.
Comment #14
alexpottDid #13 - it looks great - https://git.drupalcode.org/project/drupal/-/merge_requests/6181/diffs?co...
Comment #15
alexpottSaving issue credit.
Comment #16
longwaveLooks great.
Comment #18
catchYes this looks really good. Committed/pushed to 11.x, thanks!
Comment #19
wim leersWow, elegant solution for a test-only scenario! 👏
Comment #20
andypostPlease close MR
Comment #23
bkosborneI know there was some discussion if this needed a change record or not, and ultimately one was added.
I just want to ask that any changes that affect APCu cache always do receive a change record. I help maintain a very large (> 500 sites) multisite install. Any increase to APCu cache utilization, however small, can have cause a big increase in my overall APCu utilization and cause major issues if all allocated memory is exhausted. I don't say this to indicate Drupal shouldn't utilize APCu cache for new things, just that the core committers please ensure that the changes are advertised as change records, just like this one was. It allows me time to test how much more memory I'll need to allocate. Thank you