perf: drop per-request MIME registration from the App constructor (#108) - #125
Merged
Conversation
…#108) Application::__construct ran IMimeTypeDetector::getAllMappings() + registerType() on every app instantiation. registerType() isn't even part of the public OCP\Files\IMimeTypeDetector interface, and getAllMappings() forces loading the full mapping table each time. The .pad MIME type is already registered persistently by the RegisterMimeType repair step (config mimetypemapping.json + mimetypealiases.json, filecache backfill, core filetype icon) — the supported Nextcloud mechanism. The runtime call was redundant with that, so remove it and keep the constructor clean. Verified on NC 33: .pad still detected as application/x-etherpad-nextcloud (Files list, viewer, template picker). PHPUnit 403 + Psalm green (baseline -1: the UndefinedInterfaceMethod entry for the non-public registerType() is gone); full Playwright 23/23. Closes #108.
There was a problem hiding this comment.
Pull request overview
This PR removes per-request MIME type registration from the app Application constructor, relying solely on Nextcloud’s supported persistent MIME registration mechanism (repair steps + config mapping/aliases + filecache updates). This reduces runtime overhead and eliminates use of a non-public IMimeTypeDetector method.
Changes:
- Removed
IMimeTypeDetector::getAllMappings()/registerType()calls fromApplication::__construct(). - Updated the constructor with an explanatory comment documenting why runtime MIME registration is intentionally absent.
- Dropped the corresponding Psalm baseline entry for the non-public
registerType()call.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
lib/AppInfo/Application.php |
Removes runtime MIME registration from the constructor and documents the rationale. |
psalm-baseline.xml |
Removes the baseline suppression that was only needed due to the non-public registerType() usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…MimeType) Review follow-up on #125: the MIME repair steps had no targeted unit tests. Add the high-value, genuinely unit-testable slices; the filesystem / \OC config-dir / icon-copy parts stay covered by e2e. - RegisterMimeType: extract the idempotent config merge into a pure mergeMimeMappings() helper and test it — preserves unrelated entries, is idempotent on re-run, and corrects a stale value for our own key. - BackfillPadMimeType: fake-QueryBuilder test (mirrors BindingServiceTest) for the two skip branches (pad mime / application mimepart missing) and the update path, asserting the idempotency guard (only rows whose mimetype is not already the pad mime) and the *.pad name filter. Behaviour-preserving (the merge helper is the same array_replace_recursive). PHPUnit 409 + Psalm green.
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
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
Application::__constructranIMimeTypeDetector::getAllMappings()+registerType()on every app instantiation. Two problems:registerType()isn't part of the publicOCP\Files\IMimeTypeDetectorinterface (it's a concrete-class method), andgetAllMappings()forces loading the full mapping table each time — avoidable per-request cost.Change
Remove the runtime registration entirely (not just move it to
boot()). The.padMIME type is already registered persistently by theRegisterMimeTyperepair step — configmimetypemapping.json+mimetypealiases.json, filecache backfill, and the core filetype icon — which is the supported Nextcloud mechanism and the actual source of truth. The runtime call merely duplicated that for the current request. Constructor now only callsparent::__construct, with a comment explaining why no MIME work lives here.Acceptance
.padstill detected asapplication/x-etherpad-nextcloudVerification
UndefinedInterfaceMethodentry for the non-publicregisterType()call is gone)..paddetection still works in the Files list, viewer, and template picker.Closes #108.