framework
framework copied to clipboard
feat(compilation): force asset compilation on flush instead of deletion
Changes proposed in this pull request:
Instead of deleting compiler generated assets we will now rebuild them on cache clear.
The reason we're doing this is this scenario mainly:
- cronjob or queue worker is running
- admin clears cache and doesn't visit the site thereafter
- no assets are generated
- cronjob or queue worker is processing a task that needs one of the assets, eg sending notification email with lang keys or style
Reviewers should focus on:
- Are we sure we want to commit thereafter
- Are other changes required that I've overlooked
Necessity
- [ ] Has the problem that is being solved here been clearly explained?
- [ ] If applicable, have various options for solving this problem been considered?
- [ ] For core PRs, does this need to be in core, or could it be in an extension?
- [ ] Are we willing to maintain this for years / potentially forever?
Confirmed
- [ ] Frontend changes: tested on a local Flarum installation.
- [ ] Backend changes: tests are green (run
composer test). - [ ] Core developer confirmed locally this works as intended.
- [ ] Tests have been added, or are not appropriate here.
Required changes:
- [ ] Related documentation PR: (Remove if irrelevant)
- [ ] Related core extension PRs: (Remove if irrelevant)
Technically, isn't the only problem with locale being unavailable from non-web processes? if so I think we should only commit that rather than everything else and add a comment mentionning why.
It is, but I think changing this would make the class do what it's expected to do compared to what it does now.
That's just a naming problem though.
The class is used a lot throughout the code-base for many post-operations (ext enable, disable, settings save, cache clearing ...etc) in addition to the fact that certain extensions call this as well (package manager). There was probably a good reason behind why it flushes assets to delay recompilation to a later time/process.
imo, the main problem here is that the console needs assets to be compiled but doesn't trigger compilation itself. Then the safer solution is to adapt the console to trigger asset compilation.
The fear to change something that works that it might break something unforeseen.
I think it's broken as is and I think we need to modify this logic to do asset compilation as soon as possible. Or we need to move compilation away from the http stack to the application bootstrapping or we need to make compilation smarter about it's contextual use (compile languages when it's needed versus compile js/css only for web).
Created an issue for it (https://github.com/flarum/framework/issues/3680), I still believe we should fix the specific issue at hand rather than make radical changes to the behavior. We can revisit refactoring the behavior itself when we have a clear plan of why and how. But the bug IMO should be tackled with minimal & changes consistent with the current codebase.
I accept your wisdom in this @SychO9.