fix: Avoid undefined array key for address list#12557
Conversation
kesselb
force-pushed
the
bug/noid/undefined-array-key-addr-list
branch
from
91edd9c to
84e3fcb
Compare
| $fromHeader = $headers->getHeader('From'); | ||
| if ($fromHeader instanceof Horde_Mime_Headers_Element_Address) { | ||
| $firstAddr = AddressList::fromHorde($fromHeader->getAddressList(true))?->first(); | ||
| $firstAddr = AddressList::fromHorde($fromHeader->getAddressList(true))->first(); |
There was a problem hiding this comment.
Declared self as return type for fromHorder caused a psalm warning about ? being unless here.
| => $obj instanceof Horde_Mail_Rfc822_Address)); | ||
| public static function fromHorde(Horde_Mail_Rfc822_List $hordeList): self { | ||
| $hordeObjects = iterator_to_array($hordeList, false); | ||
| $hordeAddresses = array_values(array_filter($hordeObjects, static fn (mixed $obj) => $obj instanceof Horde_Mail_Rfc822_Address)); |
There was a problem hiding this comment.
Why changing Horde_Mail_Rfc822_Object to mixed:
Introducing intermediate variables did not only improve the readability, but also made it easier for psalm to unterstand the code. It's currently not documented, that Horde_Mail_Rfc822_List always resolved to Horde_Mail_Rfc822_Objects (it should be, but isn't) and therefore psalm will complain about the typehint.
The instanceof ensures, as before, that only objects of type Horde_Mail_Rfc822_Address are passed through.
| if ($this->addresses === []) { | ||
| return null; | ||
| public function first(): ?Address { | ||
| foreach ($this->addresses as $address) { |
There was a problem hiding this comment.
While 33 has a polyfill for array_first, 32 has not.
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
kesselb
force-pushed
the
bug/noid/undefined-array-key-addr-list
branch
from
1a1e1dc to
f3497f8
Compare
| @@ -198,7 +196,6 @@ public function testGeneratedMessage(): void { | |||
| $this->assertEquals($replies, []); | |||
| $imapMessage->method('isOneClickUnsubscribe')->willReturn(false); | |||
| $imapMessage->method('getUnsubscribeUrl')->willReturn(null); | |||
| $addessList->method('first')->willreturn('noreply@test.com'); | |||
There was a problem hiding this comment.
1) OCA\Mail\Tests\Unit\Service\AiIntegrationsServiceTest::testGeneratedMessage
PHPUnit\Framework\MockObject\IncompatibleReturnValueException: Method first may not return value of type string, its declared return type is "?OCA\Mail\Address"
The mock is not used -> remove
|
/backport to stable5.7 |
There was a problem hiding this comment.
Pull request overview
This PR fixes an "undefined array key" bug in AddressList that occurred when the internal $addresses array had non-sequential keys (e.g., after array_filter removed elements), causing $this->addresses[0] in first() to fail.
Changes:
- Rewrote
AddressList::first()to useforeachinstead of direct index access, and ensuredfromHorde()produces sequential arrays viaarray_values() - Added
array_values()inTransmissionService::getAddressList()to normalize keys afterarray_filterand group expansion - Added test coverage for
AddressList::first()andfromHorde()with non-standard input (groups producing gaps)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/AddressList.php | Fixed first() to be key-agnostic, normalized keys in fromHorde(), updated type annotations to list<Address> |
| lib/Service/TransmissionService.php | Added array_values() after filtering/expanding recipients to ensure sequential array keys |
| lib/Service/PhishingDetection/PhishingDetectionService.php | Removed unnecessary null-safe operator since fromHorde() always returns self |
| tests/AddressListTest.php | Added tests for first() and fromHorde() with group-expanded (gapped) input |
| tests/Unit/Service/AiIntegrationsServiceTest.php | Removed unused AddressList mock that was never wired into the test subject |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.