Enable CountArrayToEmptyArrayComparisonRector #12701
Conversation
empty($a) => $a === [] Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
There was a problem hiding this comment.
Pull request overview
This PR enables Rector’s CountArrayToEmptyArrayComparisonRector and applies the resulting refactors across the codebase and unit tests to replace count(...) comparisons with direct empty-array comparisons, alongside some PHPUnit modernizations.
Changes:
- Enable
CountArrayToEmptyArrayComparisonRectorinrector.php. - Replace
count($arr) === 0/> 0checks with$arr === []/$arr !== []across services, jobs, providers, migrations, and mappers. - Update unit tests (e.g.,
createMock()→createStub(), mark some testsfinal, and add casts in a few assertions).
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Sieve/SieveLoggerTest.php | Cast file_get_contents() results to string in several assertions; mark test class final. |
| tests/Unit/Service/MailTransmissionTest.php | Use PHPUnit stubs instead of mocks for Horde_Mail_Transport. |
| tests/Unit/Service/DkimValidatorTest.php | Mark test class final. |
| tests/Unit/Events/SynchronizationEventTest.php | Use stubs instead of mocks for constructor-only collaborators. |
| tests/Unit/Events/SaveDraftEventTest.php | Use stubs instead of mocks for constructor-only collaborators. |
| tests/Unit/Events/OutboxMessageCreatedEventTest.php | Use stubs instead of mocks for constructor-only collaborators. |
| tests/Unit/Events/NewMessagesSynchronizedTest.php | Use stubs instead of mocks for account/mailbox in constructor tests. |
| tests/Unit/Events/MessageSentEventTest.php | Use stubs instead of mocks for constructor-only collaborators. |
| tests/Unit/Events/MessageFlaggedEventTest.php | Use stubs instead of mocks for constructor-only collaborators. |
| tests/Unit/Events/MessageDeletedEventTest.php | Use stubs instead of mocks for constructor-only collaborators. |
| tests/Unit/Events/MailboxesSynchronizedEventTest.php | Use stub instead of mock for constructor-only collaborator. |
| tests/Unit/Events/DraftSavedEventTest.php | Use stubs instead of mocks for constructor-only collaborators. |
| tests/Unit/Events/DraftMessageCreatedEventTest.php | Use stubs instead of mocks for constructor-only collaborators. |
| tests/Unit/Events/BeforeMessageDeletedEventTest.php | Use stub instead of mock for constructor-only collaborator. |
| tests/Unit/Events/BeforeImapClientCreatedTest.php | Use stub instead of mock for constructor-only collaborator. |
| tests/Unit/Db/TextBlockTest.php | Mark test class final; cast json-serialized fields to string in assertions. |
| tests/Unit/Db/MessageSnoozeTest.php | Mark test class final. |
| tests/Unit/Db/ActionsTest.php | Mark test class final. |
| rector.php | Add/enable CountArrayToEmptyArrayComparisonRector. |
| lib/Service/SnoozeService.php | Replace count($messages) === 0 with $messages === []. |
| lib/Service/Provisioning/Manager.php | Replace count($configs) === 0 with $configs === []. |
| lib/Service/PhishingDetection/LinkCheck.php | Replace count($results) > 0 with $results !== []. |
| lib/Service/MimeMessage.php | Replace attachment/embedded-part count(...) > 0 checks with !== []. |
| lib/Service/MailFilter/FilterBuilder.php | Replace count(...) empty/non-empty checks with === [] / !== []. |
| lib/Service/DataUri/DataUriParser.php | Replace count($items) > 0 with $items !== []. |
| lib/Search/FilteringProvider.php | Replace count($filters) === 0 with $filters === []. |
| lib/Provider/MailProvider.php | Replace service existence checks using count(...) with !== []. |
| lib/Migration/Version1105Date20210922104324.php | Replace count($accountIds) === 0 with $accountIds === []. |
| lib/Migration/Version0110Date20180825201241.php | Replace count($chunk) > 0 loop condition with $chunk !== []. |
| lib/IMAP/MessageMapper.php | Replace count($result) === 0 with $result === []. |
| lib/Db/MessageMapper.php | Replace count($excludeMailboxIds) > 0 with $excludeMailboxIds !== []. |
| lib/ContextChat/ContextChatProvider.php | Replace count($messageIds) === 0 with $messageIds === []. |
| lib/BackgroundJob/TrashRetentionJob.php | Replace count($messages) === 0 with $messages === []. |
| lib/BackgroundJob/FollowUpClassifierJob.php | Replace empty/non-empty count(...) checks with === [] / !== []. |
| lib/BackgroundJob/ContextChat/SubmitContentJob.php | Replace count($items) > 0 with $items !== []. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| $content = file_get_contents($logFile); | ||
| $this->assertStringContainsString('Test message', $content); | ||
| $this->assertStringContainsString('Test message', (string)$content); |
There was a problem hiding this comment.
Casting the result of file_get_contents() to string can hide an I/O failure (false becomes ''), making the test failure less actionable. Prefer asserting that file_get_contents() did not return false (or assertIsString) before running assertStringContainsString().
|
|
||
| $content = file_get_contents($logFile); | ||
| $this->assertStringContainsString('Message 1', $content); | ||
| $this->assertStringContainsString('Message 2', $content); | ||
| $this->assertStringContainsString('Message 3', $content); | ||
| $this->assertStringContainsString('Message 1', (string)$content); | ||
| $this->assertStringContainsString('Message 2', (string)$content); | ||
| $this->assertStringContainsString('Message 3', (string)$content); |
There was a problem hiding this comment.
Casting the result of file_get_contents() to string can hide an I/O failure (false becomes ''), making the test failure less actionable. Prefer asserting that file_get_contents() did not return false (or assertIsString) before running assertStringContainsString().
|
|
||
| $content = file_get_contents($logFile); | ||
| $this->assertStringContainsString('äöü', $content); | ||
| $this->assertStringContainsString('🎉', $content); | ||
| $this->assertStringContainsString('äöü', (string)$content); | ||
| $this->assertStringContainsString('🎉', (string)$content); |
There was a problem hiding this comment.
Casting the result of file_get_contents() to string can hide an I/O failure (false becomes ''), making the test failure less actionable. Prefer asserting that file_get_contents() did not return false (or assertIsString) before running assertStringContainsString().
|
|
||
| $content = file_get_contents($logFile); | ||
| $this->assertStringContainsString('Line 1', $content); | ||
| $this->assertStringContainsString('Line 2', $content); | ||
| $this->assertStringContainsString('Line 3', $content); | ||
| $this->assertStringContainsString('Line 1', (string)$content); | ||
| $this->assertStringContainsString('Line 2', (string)$content); | ||
| $this->assertStringContainsString('Line 3', (string)$content); |
There was a problem hiding this comment.
Casting the result of file_get_contents() to string can hide an I/O failure (false becomes ''), making the test failure less actionable. Prefer asserting that file_get_contents() did not return false (or assertIsString) before running assertStringContainsString().
|
|
||
| $content = file_get_contents($logFile); | ||
| $this->assertStringContainsString('First message', $content); | ||
| $this->assertStringContainsString('Second message', $content); | ||
| $this->assertStringContainsString('First message', (string)$content); | ||
| $this->assertStringContainsString('Second message', (string)$content); |
There was a problem hiding this comment.
Casting the result of file_get_contents() to string can hide an I/O failure (false becomes ''), making the test failure less actionable. Prefer asserting that file_get_contents() did not return false (or assertIsString) before running assertStringContainsString().
|
|
||
| $content = file_get_contents($logFile); | ||
| $this->assertStringContainsString(pack('H*', 'deadbeef'), $content); | ||
| $this->assertStringContainsString(pack('H*', 'deadbeef'), (string)$content); |
There was a problem hiding this comment.
Casting the result of file_get_contents() to string can hide an I/O failure (false becomes ''), making the test failure less actionable. Prefer asserting that file_get_contents() did not return false (or assertIsString) before running assertStringContainsString().
| $messages = array_filter( | ||
| $messages, | ||
| static fn (Message $message) => $message->getMailboxId() === $mailboxId, | ||
| ); | ||
| if (count($messages) === 0) { | ||
| if ($messages === []) { |
There was a problem hiding this comment.
After array_filter(), array keys are preserved. This code later reads $messages[0], which can be undefined if the first element was filtered out (leading to notices/TypeErrors at runtime). Consider reindexing with array_values() (or using reset()) before accessing the first element / counting.
| $this->assertStringContainsString('<html>', (string)$json['content']); | ||
| $this->assertStringContainsString('🎉', (string)$json['preview']); |
There was a problem hiding this comment.
Casting $json['content'] and $json['preview'] to string weakens the type check this test is implicitly making (it will coerce non-strings rather than failing with a clear type mismatch). Prefer asserting these entries are strings (or using assertSame with expected strings) instead of casting.
| $this->assertStringContainsString('<html>', (string)$json['content']); | |
| $this->assertStringContainsString('🎉', (string)$json['preview']); | |
| $this->assertIsString($json['content']); | |
| $this->assertStringContainsString('<html>', $json['content']); | |
| $this->assertIsString($json['preview']); | |
| $this->assertStringContainsString('🎉', $json['preview']); |
No description provided.