Technical Debt Analysis: Talk (spreed) PHP Codebase

[nickvergessen]

Prompt

Do a technical analysis of the PHP code base. Focus on technical debt topics and outline where similar
things use different concepts and should be aligned, use of deprecated and old functionality, code
and writing style that steams from originally supporting PHP 7.0 while the code base is PHP 8.2 or
later nowerdays, and similar things.

Fable5

Technical Debt Analysis: Talk (spreed) PHP Codebase

TL;DR: The codebase is in remarkably good shape for its age regarding PHP language modernization —
constructor promotion, readonly, attributes, match, and typed events are widely adopted, and almost no
PHP 7-era syntax survives. The real debt is architectural: a legacy data-access layer (Manager.php
hand-rolling what QBMapper does elsewhere), service-locator calls inside the Room domain model, god
classes/controllers, naming-convention drift (*Manager vs *Service), and 8-year-old "temporary" code.
The biggest language-level opportunity is converting ~20 de-facto enum constant groups to native PHP
enums (only 2 enums exist today).

---

PHP 7.0 leftovers — mostly cleaned up, few stragglers

The mechanical modernization is essentially done. What remains:

• Native enums barely used. Only lib/RoomAttributes.php and lib/RoomPresets/Parameter.php are real
  enums. Meanwhile the core domain is full of PHP 7-style constant groups that are de-facto enums:
  • Room::TYPE_* (7 values), Room::OBJECT_TYPE_* (9), Room::RECORDING_, Room::LISTABLE_,
    Room::START_CALL_*
  • Participant::OWNER/MODERATOR/USER/GUEST/..., Participant::NOTIFY_, Participant::PRIVACY_
  • Attendee::ACTOR_* (string-backed candidate), Webinary::LOBBY_/SIP_, BreakoutRoom::MODE_/STATUS_
    This is the highest-ROI language modernization: int-backed enums would give exhaustive match checking
    and kill a whole class of "magic int" bugs. (Caveat: values cross the OCS API and DB, so backed enums
    with ->value at the boundaries; bitflag groups like Attendee::PERMISSIONS_* and Participant::FLAG_*
    should stay as int constants.)
• ~84 strpos/substr call sites where str_contains/str_starts_with would read better — clusters in
  lib/Config.php:413-433 (manual URL slicing that should arguably be parse_url()),
  lib/MatterbridgeManager.php:573-583, lib/Chat/MessageParser.php:188.
• Trivia: 6 is_null() calls (mostly MatterbridgeManager), 2 loose comparisons
  (lib/Command/Signaling/VerifyKeys.php:50, lib/BackgroundJob/CheckCertificates.php:90 — the latter is ==
  null, a real smell), 13 remaining switch statements vs 62 match (the 16-branch elseif chain in
  MatterbridgeManager::generateConfig() around line 337 is the worst).
• Otherwise clean: zero array(), zero list(), zero docblock route annotations, 82% of constructors use
  property promotion, ~1,242 readonly properties, no PHP 8.2 deprecation triggers (utf8_encode, dynamic
  properties, ${var}).

Deprecated / legacy API usage

• \OC_Util::tearDownFS()/setupFS() at lib/Chat/Parser/SystemMessage.php:990-992 — the only
  private-server-API usage left, already FIXME-marked. This is the one item that can genuinely break on a
  future server release; should move to OCP\IRootFolder/userFolder-based access or a public FS-setup
  API.
• OBJECT_TYPE_PHONE_LEGACY (lib/Room.php:43, marked @deprecated) is still consumed in 5 places
  (Notification/Notifier.php:1095, Service/AvatarService.php:265, Controller/RoomController.php:767,
  Service/RoomService.php:192, Listener/RestrictStartingCalls.php:57). Either the deprecation is wishful
  thinking or the call sites need a migration path.
• ~164 of 193 json_encode/decode calls lack JSON_THROW_ON_ERROR — mostly trusted data (DB, config), but
  the 1:1 room name trick (Room.php:248 json_decode($this->getName())) silently degrades on malformed
  data.
• Already fully modernized (no action): Psr\Log\LoggerInterface everywhere, 100% attribute-based
  controller annotations (#[NoAdminRequired], #[BruteForceProtection], #[ApiRoute]), 100% dispatchTyped()
  events with zero legacy hooks, query builder everywhere with one acceptable raw-SQL migration.

Same thing, different concepts — alignment targets

This is where the "generational drift" shows most clearly. Newer modules (Federation, Recording, Bot,
RoomPresets) follow one clean pattern; the 2016-era core follows another.

1. Two data-access worlds. ~15 newer entities (Attendee, Session, Poll, Thread, Bot*, Ban, Invitation,
   …) use QBMapper + Entity in lib/Model/. The two most central objects — Room and Participant — are
   hydrated by hand in Manager::createRoomObject() (lib/Manager.php:127) with ~26 positional constructor
   arguments mapped from SelectHelper-aliased columns. Every new Room column touches SelectHelper,
   Manager, and the Room constructor in lockstep. Recommendation: a RoomMapper (even without making Room
   an Entity) that owns hydration, so column mapping lives in one place.

2. Service-locator calls inside the domain model. lib/Room.php:231-332 has four Server::get(...) blocks
   each marked // TODO use DI — getName() lazily writes to the database via RoomService::setName(), and
   getDisplayName()/getLastMessage() pull Manager out of the container. This creates the Room →
   RoomService → Room cycle, makes Room untestable in isolation, and means a getter has side effects. The
   cleanest fix is moving name-resolution/last-message loading into Manager/formatter code that already
   has DI, leaving Room a dumb data object.

3. *Manager vs *Service, lib-root vs lib/Service/. GuestManager, MatterbridgeManager, Manager itself
   sit at lib root; RoomService, ParticipantService, RecordingService, BotService in lib/Service/;
   ChatManager in lib/Chat/. Reads (Manager) vs writes (RoomService) is a real split for rooms, but
   nothing enforces or documents it, and GuestManager/ChatManager are services in all but name. Worth
   writing the convention down even if renames aren't worth the churn.

4. Error-signaling styles coexist. Lookups throw (Manager::getRoomById() → RoomNotFoundException), some
   loaders return null (Manager::loadLastCommentInfo()), mutations return bool-for-noop
   (RoomService::setPermissions()). Each is defensible alone; together a caller can't predict the
   contract. A one-paragraph policy in the contributing docs would prevent further drift.

5. Config access in three layers. The Config.php facade (975 lines) dominates; some classes inject
   IConfig/IAppConfig directly; ConfigLexicon is brand-new and covers exactly 2 user-preference entries
   with Strictness::IGNORE ("only start" per its own comment). Direction is clearly toward the Lexicon —
   the debt is the long tail of keys not yet registered there.

6. Controller hierarchy split. 18 controllers extend AEnvironmentAwareOCSController (with the
   InjectionMiddleware room/participant setter pattern); ~12 extend OCSController directly. Mostly
   justified (no room context), but undocumented. Notably, controllers contain ~42 \OCP\Server::get()
   calls (13 in ChatController alone) to instantiate federation proxy controllers per-request — consistent
   as a pattern, but it's a second DI mechanism living inside the first.

7. Copy-paste siblings. Signaling/BackendNotifier (541 lines), Recording/BackendNotifier (169), and
   Federation/BackendNotifier share the same doRequest() + retry + PHPUNIT_RUN guard boilerplate — extract
   an abstract base.

8. Cache prefixes. lib/CachePrefix.php exists, but one of its own constants (hpb_servers) skips the
   talk/ prefix, and Capabilities.php uses a raw 'talk::' string instead of the constant class.

Structural hotspots & process debt

• God files: RoomController.php (3,305 lines, 63 endpoints, ~40 constructor deps), ChatController.php
  (2,604), ParticipantService.php (2,423 — participants + sessions + permissions + federation + caching),
  RoomService.php (1,601), Manager.php (1,587). ParticipantService is the best split candidate (a
  SessionService would fall out naturally).
• 15× // FIXME Temporary solution for the Talk6 release in lib/Manager.php — Talk 6 shipped in 2019;
  this "temporary" empty-token filtering is now load-bearing. Decide: make permanent (delete comments) or
  clean the data with a repair step.
• Psalm: level 4, findUnusedCode="false", ~199-line baseline. Tightening to level 3 and enabling
  unused-code detection would surface debt the baseline currently hides. Only ~8 inline suppressions in
  own code — disciplined.
• Test pyramid is inverted: tests/php/ has ~61 unit test files against 567 lib files; the bulk of
  coverage is Behat integration tests. Combined with the Server::get() calls in Room, the core domain
  objects are effectively only tested end-to-end. Unit tests still use @dataProvider annotations
  (deprecated in PHPUnit 10/11) rather than #[DataProvider] attributes — a mechanical migration worth
  doing before the next PHPUnit bump.
• 112 migrations spanning 2019–2024 (Version5099* onward) — normal for the app's age, but the oldest
  could be collapsed if the project ever does a min-version baseline reset.

Suggested priority order

• \OC_Util removal (SystemMessage.php:990) — only true forward-compat risk. chore(chat): Drop OC_Util usage and use setupManager instead #18306
• Room.php service-locator removal — unlocks unit-testing the core domain and removes a
  getter-with-DB-write.
• Enum migration for the constant groups — big readability/safety win, mechanical, can go
  group-by-group.
• RoomMapper-style hydration consolidation — kills the three-place column-mapping ritual.
• Resolve the Talk6 FIXMEs + OBJECT_TYPE_PHONE_LEGACY call sites — decide and delete.
• PHPUnit attribute migration + Psalm tightening — cheap, prevents future breakage.
• Document the Manager-reads/Service-writes and throw-vs-bool conventions — zero-cost drift
  prevention.

Items 3–5 are good "one PR per group" refactors; nothing here requires a big-bang rewrite.