Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.8] Add renderable functionality to MailMessage #28386

Merged
merged 3 commits into from
May 2, 2019
Merged

[5.8] Add renderable functionality to MailMessage #28386

merged 3 commits into from
May 2, 2019

Conversation

avrahamappel
Copy link
Contributor

This PR adds the Renderable contract to the MailMessage class, allowing easier previewing of mail notifications in the browser.

In your controller:

return (new FooNotification())->toMail('example@example.com');

This is parallel with the current functionality of the Mailable class, and obviates the need to search for code snippets to accomplish the same for Notifications. This addresses Issue #24073.

…es\MailMessage

This PR adds the Renderable contract to the MailMessage class, allowing easier previewing of mail notifications in the browser. 

In your controller:
```php
return (new FooNotification())->toMail('example@example.com');
```

This is parallel with the current functionality of the Mailable class, and obviates the need to search for code snippets to accomplish the same for Notifications. This addresses Issue #24073.
*/
public function render()
{
return app(Markdown::class)->render($this->markdown, $this->data());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The app method is a Foundation helper and the illuminate/notifications component should work on its own without the entire framework/foundation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the Container be available?

Suggested change
return app(Markdown::class)->render($this->markdown, $this->data());
return Container::getInstance()
->make(Markdown::class)
->render($this->markdown, $this->data());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will, because the notifications package already requires illuminate/container, but I don't think this is the way to go. We have a container contract which becomes useless when a concrete implementation like this is used. In general, I don't like the implementation that's used for rendering the mailables (which is the same as this one proposed here for notifications). It couples the implementation (and thus the entire component) to the concrete illuminate container just so that a class can render itself (so that we save the devs from typing a couple of characters more) which I believe is a total anti-pattern (you can read more about it here -> laravel/ideas#1467). The rendering of a mailable/notification should be the responsibility of another service, the mailable/notification should only be a stupid value object.

So instead of for example:

$notification->render()

it should be $renderer->render($notification)

Basically architecture-wise I don't agree with the way the rendering of mailables is currently implemented in the framework and I think that it should be fixed so I'm obviously against copying that way over to notifications.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@X-Coder264 I don't it's realistic anymore to code around people hypothetically using an entirely different container than the one Laravel provides.

Copy link
Contributor

@X-Coder264 X-Coder264 May 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taylorotwell I think that if there is a contract that we should respect it (there either is one or there isn't, there's no middle ground). If you think that the contract became useless then we can remove it and clearly communicate that. This way is like "here you have the contract, but when we feel like it we will just use a concrete implementation". A good part of illuminate components are used outside of a full Laravel app because they can be used that way (because there is no coupling between components and the framework and the contracts are respected) and I'd like for as much of them to stay that way. In this case I don't see why an object that represents a simple notification (or email) should depend on an entire container (be it a contract or concrete implementation).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there are two issues here.

One, you feel that the render method belongs in a different class. I understand where you're coming from, but seeing as a lot of people are already used to rendering Mailables this way, there's nothing wrong with adding the functionality here. If and when Mailable is refactored, MailMessage can be easily refactored along with it.

Your second issue that we shouldn't be coupling to the concrete container. I read through the discussion you referenced, and I hear the idea. Once again, however, when the huge refactor of Container class type-hinting happens, it will not be a problem to change it in this instance as well.

In short, both of these issues are much larger than this little pull request, and there is no need to reject it because of bigger, more abstract ideas.

Laravel is all about convenience. I'm just trying to add a bit more.

Copy link
Contributor

@X-Coder264 X-Coder264 May 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are correct that there are two issues here and the first issue directly leads to the second issue. By trying to add a feature/responsibility of a service to a value object all of a sudden we need a way to get that service from a globally available container and boom we have immediately tightly coupled a class (and the entire component/package it's in) to an entire container.

I understand that Laravel is all about convenience, but there should be a limit/balance as to how far the framework is gonna go with that versus how the framework code is looking under the hood and what that means for its users who use it either in a full-stack setup or just its standalone components and what it means from the perspective of somebody who has to maintain that code. These issues are indeed much larger than this pull request and I understand where you are coming from, but let's be honest, once you open the floodgates... :) I don't like the idea of adding a new thing while thinking "it's gonna be refactored later so who cares", because is the refactor even gonna happen and who will do it and even if it does happen this means that there is gonna be one more breaking change which didn't have to happen. But anyway, this is just my humble opinion on the matter and it's of course up to Taylor to decide in what direction he wants the framework to go.

@taylorotwell taylorotwell merged commit 5c129ba into laravel:5.8 May 2, 2019
@avrahamappel
Copy link
Contributor Author

Thanks @taylorotwell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants