Skip to content

refactor(util): clean up script-related methods in OCP\Util#61018

Draft
joshtrichards wants to merge 9 commits into
masterfrom
jtr/addScript-cleanup
Draft

refactor(util): clean up script-related methods in OCP\Util#61018
joshtrichards wants to merge 9 commits into
masterfrom
jtr/addScript-cleanup

Conversation

@joshtrichards
Copy link
Copy Markdown
Member

@joshtrichards joshtrichards commented Jun 5, 2026

  • Resolves: #

Summary

This PR refactors the script registration methods in OCP\Util for clarity and safety. It improves docblocks and naming, adds missing type declarations, fixes a couple of array-initialization issues, and makes translation asset registration idempotent earlier in the pipeline.

Changes

  • expand/clarify docblocks for script-related properties and methods
  • rename scriptOrderValue() to scriptPriority()
  • add missing parameter/return types to several Util methods
  • tighten addScript()’s $file parameter from ?string to string
  • fix potential undefined-index / missing-initialization cases in addScript() and addTranslations()
  • deduplicate repeated translation asset registrations at insertion time via new private helpers
  • small readability cleanups (isset() for $scriptDeps, local variable renames, docblock cleanup)

Behavior notes

  • addScript() no longer accepts null for $file (technically a BC, but it would have never worked anyhow)
  • translation asset registration is now idempotent at insertion time

Scope note

This branch also currently contains a few small non-script-adjacent Util.php cleanups (getServerHostName(), numericToNumber(), and related docblocks). I can split those out if preferred.

Follow-up ideas

  • Introduce a dedicated script-registry service and start migrating to it

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Also rename scriptOrderValue -> scriptPriority and misc formatting

Signed-off-by: Josh <josh.t.richards@gmail.com>
Admittedly some typing/refactoring not directly related to addScript, but this class needed a little love.

Signed-off-by: Josh <josh.t.richards@gmail.com>
… key is always initialized

Signed-off-by: Josh <josh.t.richards@gmail.com>
addScript() states it accepts null for $file, but the implementation immediately builds a script path from it, which would become "$application/js/" or "js/". I don’t see a valid use case for that, and addInitScript() already requires a non-null filename. Tightening addScript() to string $file is technically a BC, but I believe it's really just correcting the contract to reflect reality.

Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
@joshtrichards joshtrichards added the 2. developing Work in progress label Jun 5, 2026
@joshtrichards joshtrichards added this to the Nextcloud 35 milestone Jun 5, 2026
@joshtrichards joshtrichards added ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) technical debt 🧱 🤔🚀 labels Jun 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2. developing Work in progress ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) technical debt 🧱 🤔🚀

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant