fuelphp-storage / email

FuelPHP Framework - Email library
1 stars 1 forks source link

Email generation process #7

Open sagikazarmark opened 10 years ago

sagikazarmark commented 10 years ago

Since Message has been decoupled from Transport it is possible to reuse both components. To make it possible after sending a message both Message and Transport should be untouched or returned in their original state. Therefore all the building and generation process should be achieved by any of the following way:

sagikazarmark commented 10 years ago

Two things could be separated: any modification which does not include any transport specific modification (like generated alt body or auto_attached attachments) are permitted, so the Message can be reused and these generation processes run only once.

sagikazarmark commented 10 years ago

Is it the ServiceProvider's or the Transport's job to populate message with default values? (eg. default sender, default recipients, etc)

WanWizard commented 10 years ago

I'd say that is configuration, and will come with the config object that the ServiceProvider will inject.

i.e. use a merge of the defaults in the class, the config in the config objects, and any config passed directly (if applicable for your class). This way you always have a config you can work with, your app can set defaults outside the class, and you can override those defaults when you forge an instance.

sagikazarmark commented 10 years ago

I actually meant other kind of defaults. We recently talked about a header config item where default headers can be set. The only problem with this is the recipient and sender headers can be formatted differently, so default recipient headers are not a good idea.

So I would like to define default sender and recipients differently. Other problem is that sender and recipients are defined as objects now, so it makes it harder. So what I have in my mind is an array of Address and Recipient objects (which could be stored in a php config file). The question is: should they be set ($message->setFrom($address)) in the transport or the ServiceProvider?

WanWizard commented 10 years ago

I thought formatting was done by the Transport layer? I would just use a separate method to set this specific header, like you can set from, to, cc, bcc, and reply-to.

sagikazarmark commented 10 years ago

Yes, formatting is done by Transport layer, but there is a header config, where you can enter headers in string format, like Bcc: "Sender" <sender@domain.com>. I would like to make it possible to be able to set default recipients, but not in header config, but some recipient config. Which means does must be stored somewhere (config?) and set somewhere (transport, service provider?). The specific methods for these specific headers are already in place in Message, but the Message class gets no config, so it cannot get the default values from config.

WanWizard commented 10 years ago

Perhaps it's time for a Manager, like it's used in Display, Auth and Session, a central class that is the main entry point of the packages functionality, and deals with everything inside the package. It could have the config, and other components to access the manager to request information that has to be "package global" or shared between classes?

sagikazarmark commented 10 years ago

I will look into it, but I am not sure I fully understand the point.

WanWizard commented 10 years ago

I'm responding to your remark about not having access to the config. There are only two options, you either inject it (at instantiation or using a setter) or you allow the class to get it (without creating dependencies to classes outside the package).

sagikazarmark commented 10 years ago

IMO it is not bad that the Message has no access to the config. It is just a data container which stores data used for sending an email. There should be no logic in it at all. The default data should be set using one of the setters(setFrom, etc) instead of getting it from the config by the Message.

So I understand what a manager is supposed to be, but I am not sure if it is the solution that should be used here. At least not the data container point of view. Injecting a manager instance into a data container is almost the same as injecting config. But I might be wrong. Anyway, it would be better if this could be solved outside the message class.

IMO there are two points in the application where these defaults can be set: in the ServiceProvider and when the Message gets injected to the Transport. Both have pros and cons as well.

WanWizard commented 10 years ago

Given the fact that it is transport level data, the obvious answer is it should be injected when the Transport is instantiated. And it needs to be determined what exactly is injected. I mean: if the config defines a to address, is that a default (i.e. to be used if setTo() is never called), or is it something to add to the To() list (i.e. always send the email to this account, no matter which other addresses are defined).

I agree it is not related to the message as such, so it should not be in there.

sagikazarmark commented 10 years ago

I think the defined addresses should always be added to the message. The only exception is the From address.

WanWizard commented 10 years ago

What makes that different from all other addresses attached to a message?

sagikazarmark commented 10 years ago

There can only be one from address, so if there is one, that should not been overwritten. Any other address type can be added multiple times.

Any objections?

sagikazarmark commented 10 years ago

It seems to me a more common use case to define always used addresses than default addresses. This is not the case with from address since it is usually unique.

WanWizard commented 10 years ago

I agree that if there is a default from address, but one is specified at runtime, the default is overwritten. For the others I'm not so sure, because I can see both use-cases. Perhaps an option to reset a property, so you can do ->resetTo()->setTo(...) if you want to get rid of any configured defaults?

That doesn't answer the question though about why all addresses are Message properties, except the from address?

sagikazarmark commented 10 years ago

From is a property as well. :-)

The difference is that from is set others are added.

Set means overwrite.

There is a clearRecipients method.

But the if the defaults are set when the message gets injected in the transport (when the message gets sent), then it is after any user modification.

This means the defaults should be set before. Or the defaults should be added if there is no other address of the same type is added.

sagikazarmark commented 10 years ago

It is still undecided then if recipient addresses should be default or always added. IMO the second is more common.

WanWizard commented 10 years ago

The defaults should be added as soon as the Message object is created. Because they are defaults, values you start with.

As long as there is not only a clearRecipients but also a method to clear individual ones (for example only clear BCC) I have no problem with starting out with the default, and add to it, as I can clear after creating the Message object (which removes the default).

sagikazarmark commented 10 years ago

Ok, that could work.

Where exactly should the defaults be set then? Because if you do new Message then next time the message gets near the config is when it is being sent.

WanWizard commented 10 years ago

As I said before, there are only two options:

Where do you see new Message being used? Ideally the user should not have to do this (if only to avoid having to manually inject all kinds of things). A "Manager" type of class could do that (you could have something like $manager->newMessage() that abstracts setting up the Message instance.

That would also solve the problem of Config. In v2, Config is a Request level class, which means that at any given time, there are at least 3 versions of config floating around in your application. And Message needs access to the correct one, so that needs to be injected somewhere. The ServiceProvider can do that for you (it knows the current Request context), but that requires a manager-like class to inject it into...

sagikazarmark commented 10 years ago

You might be right. How would this manager be related to the Transports?

IMO it would be better if the Manager itself could handle setting default values. I would like to keep Message "config-free".