-
Notifications
You must be signed in to change notification settings - Fork 109
Hotfix zendframework/zf2#7555 - Header\Sender::fromString() #9
Hotfix zendframework/zf2#7555 - Header\Sender::fromString() #9
Conversation
*/ | ||
public function testFromString($headerString, $expectedName, $expectedEmail, $expectedException = null) | ||
{ | ||
if ($expectedException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't add test logic here in the same test case. Instead split the test.
@stefanotorresi Do you will complete this? |
@Maks3w sorry I couldn't attend to this for a while, but I'll try and get at it by tomorrow, otherwise I'll let you know. |
8764b0c
to
b7002c7
Compare
@Maks3w should be good enough now. |
|
||
public function invalidHeaderLinesProvider() | ||
{ | ||
$mailInvalidArgumentException = 'Zend\Mail\Exception\InvalidArgumentException'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can replace these variables with php 5.5 class constant
Could you reuse existing data providers? |
@Maks3w not without making the tests much less clear, so tbh I don't think it's worth it. |
@Maks3w to be clear, this is good to merge AFAIC, do you need me to do something? |
@weierophinney is there anything I can do to get this merged? I saw two releases recently but this has slipped under the radar. |
Hotfix zendframework/zendframework#7555 - Header\Sender::fromString()
Header\Sender::fromString()
didn't correctly set the address for some formats (most notablySender: email@domain
).Close zendframework/zendframework#7555
Supersede and close #11