RFC for refactoring Cake\Mailer in CakePHP 4.x

burzum commented 6 years ago

By spending some time with DDD, trying to build a hexagonal app with Cake and looking at how others frameworks do it I came across these things I'm not happy with in CakePHPs mailer code when I was working on my infrastructure layer that is used to send emails.

Architectural Issues

\Cake\Mailer\Email is a god object
- It's aware of the transport
- It's aware of rendering
It has ~3000 lines of code 😮

Proposed Change

Add \Cake\Mailer\Transport\TransportInterface
Add \Cake\Mailer\MailerInterface
Add \Cake\Mailer\ implements MailerInterface
Add \Cake\Mailer\EmailInterface
\Cake\Mailer\Email implements EmailInterface
\Cake\Mailer\Email should be a pure data transfer object
Move the rendering of an email to a dedicated class
- Add an interface for that class
Add SendingFailedException

Benefits

This will make testing of each single component a lot more easier
Less coupling
Might be opinionated but I think it's a lot easier to understand than having this god object

Example

$mail = new Email();
$mail->setSubject('testing')
    ->setReceiver('burzum@cakephp.org')
    ->setFrom('noreply@cakephp.org')
    ->setFormat(EmailFormat::BOTH);

// MailRenderer will call Email::setHtmlContent() and/or setPlainContent()
$mailRenderer = new MailRenderer();
// Takes only instances of EmailInterface
$mailRenderer->set(['foo' => 'bar'])->render($mail);

// Alternatives?
$email->setHtmlContent($myContent);
$email->setHtmlContent($mailRenderer->render());

$mailer = new Mailer(new Transport());
$mailer->send($mail);

We can use another class and using __call() as a proxy to all of the three classes above to combine them in a fluid interface as we have it right now but with proper abstraction.

$mailer = new SomeMailer();
$mail
     // Not required, just if default needs to be changed
    ->setRender($renderer)
     // Not required, just if default needs to be changed
    ->setTransport($transport)
    ->setSubject('testing')
    ->setReceiver('burzum@cakephp.org')
    ->setFrom('noreply@cakephp.org')
    ->setViewVars(['foo' => 'bar'])
    ->send();

markstory commented 6 years ago

We can use another class and using __call() as a proxy to all of the three classes above to combine them in a fluid interface as we have it right now but with proper abstraction.

Won't a proxy using __call thwart IDE autocompletion and type safety?

Is there an approach that lets us refactor the internal implementation without directly impacting userland code? One of the goals for 4.0 was to not require significant application changes from 3.x unless there were existing deprecation warnings and forwards compatible APIs in 3.x. I like the direction of these changes but it seems at odds with the existing goals for 4.0.0. Perhaps these new classes could be introduced adjacent to the existing classes so there is an opt-in experience for folks during 4.x?

burzum commented 6 years ago

Won't a proxy using __call thwart IDE autocompletion and type safety?

Yes but probably stay BC through it.

Why do we do a major if we want to maintain BC? Just to get a new major out because of the number? Let's do another 3.x then and deprecate the mailer stuff?

I don't think we can get the proposed changes done while keeping the existing implementation except we use a new namespace like \Cake\Email.

ADmad commented 6 years ago

Why do we do a major if we want to maintain BC? Just to get a new major out because of the number? Let's do another 3.x then and deprecate the mailer stuff?

We have already decided goals for 4.x, primary one being it should mainly be a cleanup version with minimal BC breaks for smooth upgrading. The delivery time for 4.x will be delayed if we keep add new stuff and deprecating old in 3.x.

Instead let's focus on getting 4.x out quickly and non trivial enhancements like this proposal can go into 5.x.

burzum commented 6 years ago

@ADmad 5.x should make a huge break then and we should clearly cleanup all the architecture issues we keep dragging along and I guess sometimes still introducing them. After spending some more time with DDD and architecture it becomes hard to deny that Laravel, in terms of architecture, is the better framework. 😢

dereuromark commented 6 years ago

it becomes hard to deny that Laravel, in terms of architecture, is the better framework. cry

They sure used their quick and hard BC breaks in the past to clean up the architecture quite a bit. CakePHP was always a bit more conservative and running behind those ideas here - which of course benefited the projects in terms of stability. We must do something similar at some point, though. It does otherwise also lose skilled developers that just don't see themselves represented here anymore. Too long major versions can be a killer of architecture adaptability. The PHP (framework) world has always been moving a bit faster here in general. See also the other tickets about clean code and architecture discussions.

burzum commented 6 years ago

See also the other tickets about clean code and architecture discussions.

Can we create a ticket that collects them in a list? Which others are you aware of?

ADmad commented 5 years ago

Closing as #12468, #12810 and #13017 together achieve splitting of Email's responsibilities into separate classes. The Email class can still be used as before for backwards compatibility.

cakephp / cakephp