refactor(theming): move CSS injection from boot() to render events - #164
Merged
Conversation
Style injection now runs on BeforeTemplateRenderedEvent and BeforeLoginTemplateRenderedEvent (via a new ThemeInjectionListener) instead of Application::boot() on every request. This means WebDAV/OCS/API/cron requests no longer pay for config reads, service resolution, and the custom-overrides filesystem check when no template is ever rendered. The injection body itself moves verbatim into a new CssInjectionService, which also gains render-context discrimination (occ-only `themed_contexts` appconfig; absent by default, so every surface is themed exactly as before). The per-app exclusion guard moves into the listener and resolves the app id from TemplateResponse::getApp() first, falling back to the existing path regex. Also removes the verified-dead `OCA\NLDesign\Themes\NLDesignTheme` import from Application.php. Tests: 27 new unit tests (CssInjectionServiceTest, ThemeInjectionListenerTest). Full suite (excluding the pre-existing, unrelated OC\Mail\EMailTemplate harness gap) is green: 338 tests, 2248 assertions. composer check:strict: lint/phpcs/phpmd/psalm/phpstan all pass. Live regression verification (login/user/public-share stylesheet parity, per-app exclusion, context-gating smoke test) is deferred to post-merge — see openspec/changes/render-event-injection/tasks.md section 5.
Contributor
Quality Report — ConductionNL/nldesign @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 7/7 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Coverage: 0% (0/5 statements)
Quality workflow — 2026-07-23 22:29 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 5, 2026
…failing CI (#223) v1.0.1 is `f4d9756` (2026-08-03) and predates three gate fixes, so every Hydra Gates run this repo has ever made executed a script in which 16 gates reported PASS when their helper never ran (ConductionNL/.github#147), gate-33 had no axe report to read and never said so (#148), and gates 6 and 7 reported PASS on an empty scope (#149). The tick was identical either way, which is why nothing in this repo's history shows it. That pin is now also RED, and the mechanism is worth writing down. quality.yml is referenced `@main` while this package is PINNED, so the two can desync. #164 flipped `hydra-gates-require-full-coverage` to default true in the shared workflow, and that flag requires a gate to DECLARE itself not-applicable. v1.0.1 contains ZERO `_skip` calls; v1.3.0 has 36. v1.0.1 has no vocabulary to declare, so every absent prerequisite became "DID NOT RUN" and failed the job — for gates the repo has no subject matter for. Measured on this branch, diff-scoped against origin/development exactly as CI scopes it, in a private mount namespace with a private tmpfs (the runner's ~50 /tmp/hydra-gate-*.log paths are shared state and two concurrent runs corrupt each other's counts, .github#158 item 6): v1.0.1 exit 98 FAIL — "GATES THAT DID NOT RUN: 24 33" v1.3.0 exit 0 PASS — those gates named NOT APPLICABLE, with reasons Independently confirmed end-to-end: doriath#160 changed this one line and nothing else, and its Hydra Gates job went failure -> success. v1.3.0 is `f7eaf2a` = .github@main at the time it was cut. Refs ConductionNL/.github#159
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What / Why
Application::boot()injected all nldesign CSS on every request — including WebDAV, OCS/API, and cron requests that never render a template — viaOCP\Util::addStyle(). This change moves injection ontoBeforeTemplateRenderedEvent/BeforeLoginTemplateRenderedEvent(the same two events Nextcloud core's ownThemeInjectionServiceuses), so only actual template renders pay for it.lib/Service/CssInjectionService.php: the formerinjectThemeCSS()body moved verbatim (same stylesheet cascade, same order, including the custom-font<link>from the merged custom-font-upload change), now parameterized by render context. Adds occ-onlythemed_contextsappconfig gating — absent by default, so every surface (login/user/guest/public/error) is themed exactly as before.lib/Listener/ThemeInjectionListener.php: handles both events, mapsrenderAs→ context (unmapped/future values fail open to themed), applies the per-app exclusion guard (resolved fromTemplateResponse::getApp(), falling back to the existing path-regex), and fails open (never breaks rendering) on any internal error.Application::boot()is now a no-op; the deaduse OCA\NLDesign\Themes\NLDesignTheme;import is removed.appinfo/info.xmlversion bumped (cache-buster) + CHANGELOG entry.See
openspec/changes/render-event-injection/(proposal/design/tasks + spec deltas oncss-architectureandper-app-theming) for the full rationale.Tests
tests/Unit/Service/CssInjectionServiceTest.php(14) +tests/Unit/Listener/ThemeInjectionListenerTest.php(13) — cover stylesheet order/gating, all renderAs→context mappings, per-app guard precedence/fallback/fail-open, double-dispatch, and listener failure containment.Application::injectThemeCSS()/isThemingDisabled()to retire (verified via grep) — nothing to migrate.nextcloud:34.0.0-apache(excluding the one file blocked by the pre-existing, unrelatedOC\Mail\EMailTemplateharness gap): 338 tests, 2248 assertions — all green.composer check:strict: lint/phpcs/phpmd/psalm/phpstan all pass.test:allinsidecheck:strictdies at suite-load on the same pre-existingOC\Mail\EMailTemplategap (reproduces identically ondevelopment, unrelated to this change).Deferred to post-merge (live 8080 verification — see tasks.md §5)
themed_contextsocc smoke test (set/delete)