fuelphp-storage / email

FuelPHP Framework - Email library
1 stars 1 forks source link

SMTP connection library #5

Open sagikazarmark opened 10 years ago

sagikazarmark commented 10 years ago

I've started to write an SMTP connection library inside email package, but I think this is wrong. I mean, it is not really part of the email implementation and I am thinking about releasing it as a different package. @WanWizard what do you think?

Email is quite a big package now with all those stuff.

emlynwest commented 10 years ago

For now I'd say it should stay as part of email. for 99% of cases you are not going to want an SMTP connection library without the email side of things.

sagikazarmark commented 10 years ago

I don't insist on a different package, but IMO it should/could be somehow separated from the rest.

WanWizard commented 10 years ago

There should be separate classes for the transport layer anyway, is there a (significant) difference between an SMTP class and an "SMTP class that sends an email"? I would argue that the sole purpose of SMTP transport is to send email?

sagikazarmark commented 10 years ago

Since I am a non-native english speaker I might not be able to tell you what in my mind is.

I can live with having it in the same package. But the "library" I just wrote is not about sending emails, it is about handling a low-level connection to an smtp server. (You can check it in src/Transport/Smtp) This is the reason why I think it might not be part of Smtp transport. But again, I don't have a problem with that.

WanWizard commented 10 years ago

Neither am I, so it's a good thing to discuss it to make it clear what is meant. ;-)

You mean with "library" a separate class? If so, then we say the same thing, I thought you meant separate composer library (= separate repository). I'd say it looks good sofar!

sagikazarmark commented 10 years ago

I mean a bunch of classes. ATM there is no Smtp Transport at all which actually composes the message and sends it. There is only a connection "library", which handles connection, can authenticate and send commands through a stream socket. This is why eg. the namespace is weird to me. (Fuel\Email\Transport\Smtp, but it has nothing to do with transport IMO since it is a lower level than that)

I originally meant a completely different repository, but @stevewest convinced me that it is not necessary. So currently I still have that weird feeling with no solution. ;)

WanWizard commented 10 years ago

Ok, then were all on the same line again. Cool. ;-)

You could split it into Connection and Transport, so transport uses a connection of type 'tcp' which contains the open/close code, and a read and write method, and gives a connection object back to transport to read and write on. Then you'll have Fuel\Email\Smtp that uses Fuel\Email\Transport\Tcp.

sagikazarmark commented 10 years ago

Öhm...

I am not sure we are. ;)

Fuel\Email\Transport contains the v1 drivers like Smtp, Mail, Mandrill, etc. That is another thing, that I put the connection library (please, tell me how I should call it) under that namespace. But I don't get what Fuel\Email\Transport\Tcp is, since it is an smtp connection and not an email transport.

What I could imagine: put the connection library under the namespace Fuel\Email\Smtp, so Fuel\Email\Transport\Smtp CLASS (the transport class) can use the library and there is no need to mix the Transport with Smtp connection classes.

WanWizard commented 10 years ago

From an OSI stack point of view, you start with your connection layer, which is in charge of making the physical connection, and handles open, close, read and write (any may errors). You can have different types of physical connections, most will use tcp (a socket connection), or ssl (socket over ssl), but I would implement the current "Noop" driver at this level too).

On top of that you'll have a transport layer, which is this case is SMTP. It sets up a connection with the mail server using the connection layer, and implements the SMTP protocol. For me, "tls" is an smtp level implementation on a tcp connection, so that should not belong in connection, it belongs in SMTP. If you would have an X.400 transport layer, it would use the same tcp connection layer to setup the connection to the mailserver.

So, basically what you now have in Fuel\Email\Smtp\Connection.php and Fuel\Email\Smtp\Connection* (except tls) should be in Fuel\Email\Connection as it's not linked to SMTP. Other email protocols could also use a socket connection.

sagikazarmark commented 10 years ago

Hm.. so you say it could be implemented as a general connection layer?

I can see the value in it, but still, there are much things that is needed to be built on top of this layer to make it work with SMTP Transport.

So as I can see, it is needed to be split up: a general connection layer implementation, and the extended version for SMTP.

Just one note: this way we are building a socket connection layer inside an email package which makes me feel the same weird feeling.

WanWizard commented 10 years ago

Yes. Which is already sort of is (the Fuel\Email\Smtp\Connection class and the classes in Fuel\Email\Smtp\Connection* (except tls)). All the rest is transport.

And obviously stuff like Command::invoke(new Ehlo($this)); does not belong in Connection, it's part of the SMTP protocol, and should be in the transport layer.

I would not worry about the weird feeling at the moment, we can always see/decide whether or not it should be a separate repo later. I don't want to create repo's for everything that you could possibly re-use, you'll end up with uncountable repo's... ;-)

sagikazarmark commented 10 years ago

Basically the following things are 'protocol dependent' in a connection: read, write, authentication (and connection state based on that), socket opening/closing events.

So there must be a way to let the protocol implementation do this. Either by an event based implemetation (which is IMO good) or by injecting a connection instance into the specific connection type(ssl, tcp, tls), so extending the connection itself is easy.

WanWizard commented 10 years ago

Connection should have:

This needs a connection config, containing the required type (tcp, ssl) and optional parameters for that type, like hostname or IP, port number, timeout info, etc. So you can do:

$this->connection = new Fuel\Email\Connection\Tcp(['host' => '1.2.3.4', 'port' => 25, 'timeout' => 60]);

which either throws an exception of the setup fails, or an instance of the requested connection type. This setup can call open() implictly in the constructor, so the returned connection is ready to work with.

Everything else is transport. Authentication for example is also SMTP protocol specific, and should be in the transport layer.

So the SMTP transport layer should use the above to setup the connection, and if it has one, it has to do it's initial EHLO/HELO handshake, and check if TLS is needed, and if so do a STARTTLS and repeat the EHLO/HELO handshake.

To give an example of why you would want to split is this way: say I have an SMTP mail server, which is only accesible via a modem. So you would have a Connection class Modem, which would setup the modem connection, dials the phone number of the remote modem, sets up a PPP connection, and sets up a tcp connection over the ppp connection. After this, SMTP would work, whichout having any knowledge of how it's commands reach the SMTP server...

sagikazarmark commented 10 years ago

May I use events for the following: connection open, connection close, connection state change, error? It would make easier to implement protocol specific things when these events occur.

WanWizard commented 10 years ago

Why do you need events? It seems to me it's a very complicated way of implementing signalling. And open and close don't need signalling at all, they either work, or they don't?

sagikazarmark commented 10 years ago

Actually I do not, so I left it. ;)

There is a Socket namespace now, which contains a simple socket client implementation. SMTP protocol specific things moved to SMTP Transport.