Implement null connection #31
Conversation
- Add Genkgo\Mail\Protocol\Smtp\NullConnection class - Be more informative when throwing exception on unsupported scheme - Add test coverage
|
@GawainLynch Thanks! Will look at this next week. Although I have one remark already, the |
I'm going to blame that on "Friday night" … I'll dig back into it in the morning. 👍 |
|
And tests for the class are missing. |
|
Moved NS & added tests. Just to note, I had left out tests on the original PR as I had (originally) copied |
|
Thanks. I’d love to have tests though. There is defined behaviour, which is pretty well testable. This is not the case for the PlainTcpConnection, because you will have to open a tcp connection in another thread (other php process). Therefore I marked it as ignored for code coverage. |
|
Not a problem in the world for me. I'm just in the process of "getting to know how you work" … every project does things (slightly or more) different. |
|
Oh, I understand completely, and I am very happy with the fact you are contributing! In terms of how I work regarding to code: as strict as possible ;) |
|
Actually, on that … could you ping me via email (address on my GH profile), I have a couple of things to quickly run by you. |
| public function it_does_nothing_when_upgrade_called(): void | ||
| { | ||
| $connection = new NullConnection(); | ||
| $connection->upgrade(42); |
There was a problem hiding this comment.
Maybe use a constant here.
src/Protocol/Smtp/NullConnection.php
Outdated
|
|
||
| public function send(string $request): int | ||
| { | ||
| $this->setReceive($request); |
There was a problem hiding this comment.
My aversion to set is pretty big... :) I'd prefer something like pushResponseToBuffer.
src/Protocol/Smtp/ClientFactory.php
Outdated
| @@ -224,10 +224,14 @@ public static function fromString(string $dataSourceName):ClientFactory | |||
| $components['port'] ?? 25 | |||
| ); | |||
| break; | |||
| case 'null': | |||
There was a problem hiding this comment.
I don't know if we should add null:// to the client factory. A new NullConnection is not something that typically requires a factory. new Client(new NullConnection()) is not harder than ClientFactory::fromString()->newClient(). Moreover, NullConnection is typically required during testing. So there is no argument in adding it for the developer's easiness. Unless, you have sound reasons to have it in the factory, I'd say we do not add it to the factory.
There was a problem hiding this comment.
[…] is not harder than […]
That is both fair and correct …
Unless, you have sound reasons to have it in the factory, I'd say we do not add it to the factory.
My counter — that I am only adding for historical reference — is that my aversion to inconsistency is approximate to your aversion to setters 😉
However, personal ideology shouldn't stand in the way of a practical solution … so consider it "removed" 👍
There was a problem hiding this comment.
Oh, and linking #10 as when that gets moving, I'll add your suggestion to the docs.
|
Much obliged! |
|
A pleasure 😺 |
Use:
Use cases:
Please feel very free to throw rejection, requests for changes, or merge my way. 😉