framework icon indicating copy to clipboard operation
framework copied to clipboard

feat(compilation): force asset compilation on flush instead of deletion

Open luceos opened this issue 3 years ago • 4 comments

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)

luceos avatar Aug 05 '22 13:08 luceos

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.

SychO9 avatar Aug 17 '22 12:08 SychO9

It is, but I think changing this would make the class do what it's expected to do compared to what it does now.

davwheat avatar Aug 17 '22 12:08 davwheat

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.

SychO9 avatar Aug 17 '22 15:08 SychO9

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).

luceos avatar Aug 18 '22 12:08 luceos

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.

SychO9 avatar Nov 15 '22 13:11 SychO9

I accept your wisdom in this @SychO9.

luceos avatar Nov 23 '22 14:11 luceos