POP3 XOAUTH2 support for Microsoft#199
POP3 XOAUTH2 support for Microsoft#199griesi007 wants to merge 10 commits intolaminas:2.17.xfrom griesi007:2.17.x
Conversation
Signed-off-by: griesi007 <github@12live.de>
Signed-off-by: griesi007 <github@12live.de>
Signed-off-by: griesi007 <github@12live.de>
Signed-off-by: griesi007 <github@12live.de>
Signed-off-by: griesi007 <github@12live.de>
| use Laminas\Mail\Protocol\Exception\RuntimeException; | ||
| use Laminas\Mail\Storage\ParamsNormalizer; | ||
|
|
||
| class Microsoft extends \Laminas\Mail\Protocol\Pop3 |
There was a problem hiding this comment.
Docblock is needed here, documentation inside the repo also needs to mention the existence of this protocol
There was a problem hiding this comment.
This class requires unit/integration testing.
Not sure if integration testing is viable though, because it would require a mock Microsoft POP3 server 🤔
There was a problem hiding this comment.
@Ocramius UnitTests for static function fromParams? Rest of the class are private methods or methods heavily relying on the base class POP3 which itself is completely untested as far as I can see. I am not that experienced with unit testing so not really an idea where to start and where to end. Probably I am not able to introduce Unit Test for the base class
There was a problem hiding this comment.
Testing this specific class would suffice: look at existing tests as a start.
For running them:
composer install./vendor/bin/phpunit
If you have no experience, this is the perfect occasion to get started: no code lands without appropriate test coverage here, so it's an unavoidable roadblock. Once you learned it, you will likely start adopting it yourself too 👍
There was a problem hiding this comment.
@Ocramius Sorry but I do not have an idea how to test this class beeing so heavily bound to sockets and more stuff by the underlying base class which cannot easily replaced by mocks or stubs because of missing DI principles. Anyhow without any help you can just sadly delete this pull reqeust
There was a problem hiding this comment.
I gave you an initial starting point: can't really tutor step by step.
Push what you have, see if anyone else can help? You can also ask on our slack for help 👍
There was a problem hiding this comment.
Can.´t do more here. Pushed with all the other issues fixed. If this is not acceptable just reject the pull request
There was a problem hiding this comment.
If this is not acceptable just reject the pull request
Will reject then: yes, testing is required for any patches sent, even if hard to do.
laminas/laminas-mail has the architecture it has: can't change that without breaking a lot of downstream projects, but automated testing should be performed nonetheless, since we are otherwise blind to regressions, incompatibilities during upgrades, and it just ends up being more work on the maintainer's plate.
There was a problem hiding this comment.
@Ocramius I agree that testing is vital. In my Opionion the class is untestable as long as the base class is not refactored accordingly. I can live with my imlpementation not beeing integrated but it´s really annoying that I spent a lot of time to do my best to give back to the community with such an outcome.
There was a problem hiding this comment.
I spent a lot of time to do my best to give back to the community with such an outcome.
I did propose that you'd invest more time into it, but that's your decision. I already ranted about time yesterday, from my end: https://twitter.com/Ocramius/status/1551907387262500865
I'm actually more worried that you "stopped trying" after finding the first roadblock, when I made it clear relatively early ( >2h ago) that testing would be mandatory regardless of implementation.
the class is untestable as long as the base class is not refactored accordingly.
an integration test would have sufficed, and yes, complex.
Signed-off-by: griesi007 <github@12live.de>
Signed-off-by: griesi007 <github@12live.de>
|
Closing as |
Add support for Microsoft POP3 via modern authentication
Usage:
$storage = new \Laminas\Mail\Storage\Pop3Pop3(\Laminas\Mail\Protocol\Pop3\Xoauth2\Microsoft::fromParams([ 'host' => 'Outlook.office365.com', 'targetMailbox' => 'test@contoso.onmicrosoft.com', 'accessToken' => '###ACCÈSSTOKEN###', ]));