- Issue created by @alexpott
- Status changed to Needs work
6 months ago 8:05am 16 January 2024 - π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
This is not yet passing tests.
Also: one question on the MR π€
- π¬π§United Kingdom longwave UK
Given 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? - Status changed to Needs review
6 months ago 11:37am 16 January 2024 - π¬π§United Kingdom alexpott πͺπΊπ
I 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.
- π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
alexpott β credited kim.pepper β .
- π¬π§United Kingdom alexpott πͺπΊπ
Thanks for the review @kim.pepper
- π¬π§United Kingdom alexpott πͺπΊπ
I 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_prefix
setting set to FALSE. - Status changed to RTBC
6 months ago 9:46am 19 January 2024 - π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
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_prefix
setting set to FALSE.Holy π© π€©π€―
You're right!
π€―
- Status changed to Needs work
6 months ago 10:46am 19 January 2024 - π¬π§United Kingdom longwave UK
Agree #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.
- π¬π§United Kingdom alexpott πͺπΊπ
Did #13 - it looks great - https://git.drupalcode.org/project/drupal/-/merge_requests/6181/diffs?co...
- Status changed to Needs review
6 months ago 10:54am 19 January 2024 - Status changed to RTBC
6 months ago 11:29am 19 January 2024 - Status changed to Fixed
6 months ago 11:43am 19 January 2024 - π¬π§United Kingdom catch
Yes this looks really good. Committed/pushed to 11.x, thanks!
- π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
Wow, elegant solution for a test-only scenario! π
Automatically closed - issue fixed for 2 weeks with no activity.