refactor(php): apply constructor property promotion (PHP 8.0+)

[ChristophWurst]

Mechanically applied ClassPropertyAssignToConstructorPromotionRector across lib/ and tests/. Hand-corrected cases where Rector renamed the $userId DI parameter to match the internal property name, which would have broken Nextcloud's container injection.

Assisted-by: ClaudeCode:claude-sonnet-4-6

Ref #13077

🤖 AI (if applicable)

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

[ChristophWurst]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[ChristophWurst]

@coderabbitai review with care

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR info and changed files
gh pr view 13124 --repo nextcloud/mail --json title,body,files | jq '{title, files: [.files[].path]}'

Length of output: 4506

---

🏁 Script executed:

#!/bin/bash
# Get the full diff of the PR
gh pr diff 13124 --repo nextcloud/mail 2>/dev/null | head -3000

Length of output: 121386

---

🏁 Script executed:

#!/bin/bash
# Get the full diff - second chunk
gh pr diff 13124 --repo nextcloud/mail 2>/dev/null | tail -n +3001 | head -3000

Length of output: 94145

---

🏁 Script executed:

#!/bin/bash
# Get the full diff - third chunk
gh pr diff 13124 --repo nextcloud/mail 2>/dev/null | tail -n +6001 | head -3000

Length of output: 56132

---

🏁 Script executed:

#!/bin/bash
# Get the full diff - fourth chunk
gh pr diff 13124 --repo nextcloud/mail 2>/dev/null | tail -n +9001 | head -3000

Length of output: 369

---

🏁 Script executed:

#!/bin/bash
# Get the full diff - fifth chunk
gh pr diff 13124 --repo nextcloud/mail 2>/dev/null | tail -n +12001 | head -3000

Length of output: 369

---

🏁 Script executed:

#!/bin/bash
# Check how many total lines the diff has
gh pr diff 13124 --repo nextcloud/mail 2>/dev/null | wc -l

Length of output: 162

---

---

@ChristophWurst: Here is the careful review of the PR.

---

Review of refactor(php): apply constructor property promotion

Overall the refactor is clean and mechanically correct. The stated hand-corrections to the $userId DI parameter name are all present and correct — the parameter name visible to Nextcloud's container remains $userId in every controller. A few issues below deserve attention.

---

🔴 Bug: Redundant assignment left in InternalAddressController

After $internalAddressService was promoted, the old manual assignment was not removed:

// lib/Controller/InternalAddressController.php
public function __construct(
    IRequest $request,
    private ?string $userId,
    private InternalAddressService $internalAddressService,  // ← promoted
) {
    parent::__construct(Application::APP_ID, $request);

    $this->internalAddressService = $internalAddressService; // ← REDUNDANT, should be removed

This is a harmless double-write but it's the only one of its kind left in the diff — every other class had the redundant body lines removed correctly.

---

🟡 Named-argument break: BeforeMessageDeletedEvent constructor parameter renamed

The old constructor parameter $mailbox was silently renamed to $folderId to match the internal property:

- public function __construct(Account $account, string $mailbox, int $messageId) {
+ public function __construct(
+     private Account $account,
+     private string $folderId,   // ← was: string $mailbox
+     private int $messageId,
+ ) {

This is a public-event constructor. Any caller using PHP 8 named arguments (mailbox: '...') would silently break. Positional callers are unaffected. Since this is an event (not DI-injected), the container impact is zero, but it's worth being aware of.

---

🟡 Unrelated change bundled in: Response::getStats() return type fix

- public function getStats(): MailboxStats {
+ public function getStats(): ?MailboxStats {

This is a correct bug-fix (the property $stats defaults to null and always could have been null), but it's unrelated to constructor property promotion and not called out in the PR description.

---

✅ Things verified as correct

Concern	Status
$userId DI parameter name preserved in all controllers	✅
public properties in IMAPMessage remain public after promotion	✅
Untyped $userId → string $userId tightenings in DraftsController, OutboxController	✅ safe
$userFolder untyped → ?Folder in MessagesController	✅ safe
Parent-constructor-first pattern in background jobs (mixed promotion)	✅ intentional
Parameter renames for class-typed DI parameters ($ci→$contactsIntegration etc.)	✅ DI injects by type

---

Minor observations (non-blocking)

• Partial promotion in PreviewEnhancementProcessingJob, QuotaJob, SyncJob, MailboxSync, CleanupService etc. — some properties remain old-style while others are promoted. This is correct where parent::__construct() must run first, but the inconsistency within a single constructor is mildly jarring.
• AccountsController still has two non-promoted properties ($l10n, $config, $hostValidator) mixed with promoted ones. All correct, just style-inconsistent.

The only mechanical fix needed is the leftover assignment in InternalAddressController.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[ChristophWurst]

@coderabbitai review another time

[coderabbitai]

@ChristophWurst Sure, triggering another review pass now!

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.