Problem/Motivation
Currently, the system clears CSS cache every time a theme is installed/enabled(refer: #2109287: Replace list_themes() with a service.).
Since the CSS cache is not theme-specific, so clearing it on every theme installation provides no benefit and may impact performance.
Steps to reproduce
- Install or enable a theme
- Observe that CSS cache is cleared via $this->cssCollectionOptimizer->deleteAll()
Proposed resolution
Remove the unnecessary CSS cache clearing from the theme installer by deleting the line:
$this->cssCollectionOptimizer->deleteAll();
Remaining tasks
Remove the code from installer and raise a MR.- Review the MR and merge.
User interface changes
N/A.
Introduced terminology
N/A.
API changes
N/A.
Data model changes
N/A.
Release notes snippet
Removed unnecessary CSS cache clearing during theme installation.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | 2149631-1.patch | 588 bytes | star-szr |
Issue fork drupal-2149631
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 2149631-dont-clear-the
changes, plain diff MR !13067
Comments
Comment #1
star-szrTiny patch to see if anything blows up. Manual testing seems to check out.
Comment #2
brianperrySpent some time looking at this - the one concern that I had is that while clearing the cache would be unnecessary for install in most cases, it seems like it would have some value when a theme is being installed and set default at the same time. That said, it might make the most sense for that to be happening in setDefaulTheme() but I couldn't really prove that it was happening there either.
It also looks like this patch would need to be re-rolled as the cache clear that we're trying to remove has moved into ThemeInstaller.php
Comment #5
lauriiiIt would be nice to see some test coverage for this.
Comment #17
smustgrave commentedThank you for creating this issue to improve Drupal.
We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #18
catchThis is still in the theme installer.
Needs the patch converted to an MR, or just a fresh MR created removing that line. Not sure we need tests for this, we'd just be testing that we removed the code really - e.g. we'd need to get some CSS aggregates created on disk, install a theme, make sure they're still there. But also with #3505776: Retain asset aggregates for 45 days by default to reduce generation of identical files new aggregates wouldn't get deleted anyway, making this impossible to test. Given that, removing the needs tests tag.
Comment #20
neerajsinghI will convert the patch to MR.
Comment #22
smustgrave commentedJust an FYI newly tagged novice tickets are typically meant for newer users to try, that have less then 1 page of posts.
After a few weeks if no new user doesn't pick it up it's fair game.
Comment #23
neerajsinghThank you, for highlighting this @smustgrave.
Comment #24
smustgrave commentedNo worries!
For good practice lets update the summary to use the standard template
Comment #25
neerajsinghUpdated issue summary.
Comment #28
slawrence10 commentedI have rebased the MR. Needs review.
Comment #29
slawrence10 commentedTest failing from removing the line.
May indicate cache clear is needed after all?
Comment #30
catchPretty sure that's a random test failure. This looks good to me, there's absolutely no reason to clear this cache. Apart from the CSS aggregates being unnecessarily cleared, it also adds overhead to the process of installing a theme.
Comment #31
slawrence10 commentedOK thank you @catch. New to contributing, so I'm still learning the process.
Comment #32
smustgrave commentedThere’s a major bug in gitlab not allowing test reruns but when that’s fixed JavaScript ones have the most random and and can be reran
Comment #33
alexpottCommitted and pushed d759d555ab4 to main and 53c4b27ed67 to 11.x. Thanks!