[10.x] Support Inline Attachments within Markdown Mailable

[cosmastech]

to close #47592

If using a view for building a message, the actual rendering of the view into HTML doesn't happen until later in the process, where it has the Message object (which has the embed() method) for use within the View. When using markdown instead, the process was happening before calling Mailer@send().

By converting these two to closures, we allow for the markdown component to be built within Mailer@renderView() instead (which passes the message data as an array).

Note that there were some unrelated tests which needed updating. This was only to accommodate how those tests' expectations were set up.

[nunomaduro]

@cosmastech Can you explain me why #47140 didn't not solve this problem?

[cosmastech]

@cosmastech Can you explain me why #47140 didn't not solve this problem?

Illuminate\Mail\Mailer@send handles a MailableContract distinctly (which is what you had implemented in the initial PR).

Mailable@buildMarkdownHtml() was added and it returns a Closure. The closure allows for the $data array to be carried forward when rendering the HTML. Within Mailer@send(), a Illuminate\Mail\Message object is added to the $data array, which the closure relies on to call $message->embed() within the markdown.

For parity, in this MR, we changed MailChannel@buildView() to return a Closure, rather than trying to build the response before it's passed to Illuminate\Contracts\Mail\Mailer@send().

Let me know if that makes sense @nunomaduro

[nunomaduro]

Also, I am little bit rusty on this. Can you tell me how can I locally reproduce the issue this pull request is solving?

[nunomaduro]

In my pull request, we've moved the "markdown->theme($message->theme)" to be executed within the given closure. Shouldn't we do the same here?

[cosmastech]

It looks like theme is respected if I write return (new MailMessage)->markdown('markdown')->theme('abba-zabba'); within the toMail() method of the notification.

That said, the instance level $markdown should be cloned, since we are changing the theme. Though Markdown is not bound as a singleton in the container, subsequent calls would use the same theme, even tho the user would anticipate it to be the result.

[nunomaduro]

But can you adjust the code, so the "set" of theme happens exactly before the rendering? Just like the code I've done for the other PR?

[cosmastech]

@nunomaduro take a look at how I have implemented it with the latest 2 commits. I can move it into the closure if that's better, but I don't see any way that the message's theme property would be overridden. (Also, I just think it would look uglier).

Could we remove the Markdown instance variable and just create it within the closures (or buildView() method)? It would mean a signature change to the constructor, but that instance level variable is more of a hinderance than a help, IMO.

[cosmastech]

Also, I am little bit rusty on this. Can you tell me how can I locally reproduce the issue this pull request is solving?

#47592 describes how to set this up.

[nunomaduro]

I would rather to have an implementation exactly like my pull request, and simply access the instance of markdown ($this->markdown) within the closure, to set the theme based on the message, right before call the renders.

[cosmastech]

I have added what I think you requested. If the code changes look good, I can go and fix the tests that are failing due to Mockery expectations.

That said, I respectfully disagree with this approach. Why even bother passing a default theme to the Markdown class if we are always going to manually fetch the markdown theme from config() every time it's used?

[cosmastech]

Maybe this would be better if there was a PendingRender class or something. Maybe an idea for 11.x

[nunomaduro]

Merging this against a branch of mine, so I can make some adjustments and test it myself. Thanks!