-
Notifications
You must be signed in to change notification settings - Fork 823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX Class manifest is not caching enums #11532
base: 5.3
Are you sure you want to change the base?
Conversation
Actually now i see why enums are not there silverstripe-framework/src/Core/Manifest/ClassManifest.php Lines 694 to 701 in f40b1a4
|
For reference, it's due to this PR which seems to have forgotten to store the enums in the cache @kinglozzer did I miss something here? |
I can’t remember (or think of) any reason why they shouldn’t be included in the cache, so it’s probably just an oversight in the original PR |
an additionnal improvement would be to not store empty arrays and simply consider missing keys as empty arrays. |
since i made benchmarks for my other PR, i took the opportunity to benchmark this as well without the fix manifest (no cache folder) manifest (regenerate cache) with the fix manifest (no cache folder) manifest (regenerate cache) The missing cache key makes the cache regeneration take 7s instead of 640ms... (with a regular hard drive (no ssd) and a file based cache... this could be less worse on a better hard drive) |
Description
Fix #11531
Enums were not being added to cache, but validated and required
Manual testing steps
See related issue
Basically, this will ensure that validateItemCache will actually return true
Currently it will always return false unless i've missed something somewhere...
Issues
Fixes #11531
Pull request checklist