Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

fix: compatible way to call the method on mailable #219

Merged
merged 3 commits into from
Nov 21, 2022

Conversation

ReeceM
Copy link
Collaborator

@ReeceM ReeceM commented Nov 3, 2022

Description

Adds a method exists check to the function of the buildMailable data method to check which method exists on the Mailable.

Related Issue

#218

Motivation and Context

This is to make the preview functionality work with Laravel 9

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@ReeceM ReeceM requested a review from Qoraiche November 3, 2022 15:10
@ReeceM ReeceM linked an issue Nov 3, 2022 that may be closed by this pull request
@ReeceM
Copy link
Collaborator Author

ReeceM commented Nov 3, 2022

The render method seems to be the same functionally to the old build one.

I tested the fix locally, not sure if we can do a require on composer with the branch number

@ReeceM ReeceM added WIP Work in progress V3 Version 3 of MailEclipse (Laravel 8) Review Needed v4 Laravel 9 support version labels Nov 3, 2022
@Qoraiche
Copy link
Owner

Qoraiche commented Nov 3, 2022

I got this error while testing it locally:
https://flareapp.io/share/xPQo2Ar5

Mailable:

<?php

namespace App\Mail;

use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Mail\Mailable;
use Illuminate\Mail\Mailables\Content;
use Illuminate\Mail\Mailables\Envelope;
use Illuminate\Queue\SerializesModels;

class ContactAdmin extends Mailable
{
    use Queueable, SerializesModels;

    public $name;

    /**
     * Create a new message instance.
     *
     * @return void
     */
    public function __construct($name)
    {
        $this->name = $name;
    }

    /**
     * Get the message envelope.
     *
     * @return \Illuminate\Mail\Mailables\Envelope
     */
    public function envelope()
    {
        return new Envelope(
            subject: 'Contact Admin',
        );
    }

    /**
     * Get the message content definition.
     *
     * @return \Illuminate\Mail\Mailables\Content
     */
    public function content()
    {
            return new Content(
                markdown: 'default-mail',
                with: [
                'name' => $this->name,
            ],
        );
    }

    /**
     * Get the attachments for the message.
     *
     * @return array
     */
    public function attachments()
    {
        return [];
    }
}

@ReeceM
Copy link
Collaborator Author

ReeceM commented Nov 3, 2022

Render works in some contexts then. I didn't get that error but does make sense as the returned value gets used later in to get the view data etc.

I'll have to have another look in a few hours though at the moment 👍

@Qoraiche
Copy link
Owner

Qoraiche commented Nov 4, 2022

The render method seems to be the same functionally to the old build one.

I tested the fix locally, not sure if we can do a require on composer with the branch number

we can use the render method to return the mailable preview

@ReeceM
Copy link
Collaborator Author

ReeceM commented Nov 17, 2022

Personal and reference Note:

Lines 553 - 557 needs to change in the MailEclipse class as the mailables won't display after the fix works for Laravel 9

            $collection = collect($fqcns)->map(function ($mailable) {
                return $mailable;
+            });
-            })->reject(function ($object) {
-               return ! method_exists($object['namespace'], 'build');
-            });

@ReeceM
Copy link
Collaborator Author

ReeceM commented Nov 19, 2022

Currently working when there aren't things in the Mailables constructor. As when it is instanciated using the content method / render method it actually returns a model that doesn't have viewData props.

@Qoraiche
Copy link
Owner

Thanks for the changes @ReeceM, I'll give it a try tonight and get back to you

@ReeceM
Copy link
Collaborator Author

ReeceM commented Nov 19, 2022

Thank you 👍

I suspect it will need some more work to get it working nicely.

@Qoraiche
Copy link
Owner

Qoraiche commented Nov 20, 2022

Thank you 👍

I suspect it will need some more work to get it working nicely.

Indeed, it looks good to me, is it safe to push these changes as a quick fix for laravel 9?

@ReeceM
Copy link
Collaborator Author

ReeceM commented Nov 21, 2022

Thank you 👍

I suspect it will need some more work to get it working nicely.

Indeed, it looks good to me, is it safe to push these changes as a quick fix for laravel 9?

The changes are alright for Laravel 9 it shouldn't be breaking it further than it is at the moment because of the change in the Mailable methods.

What might happen is the edge cases will be found for some who have a different setup.

I did try and run it using an old mailable made with the build method and one made using the new style in a Laravel 9 app and both worked.

Copy link
Owner

@Qoraiche Qoraiche left a comment

Choose a reason for hiding this comment

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

All good, it can be merged @ReeceM (y)

@Qoraiche
Copy link
Owner

Qoraiche commented Nov 21, 2022

I'm going to open a new PR to make MailEclipse work with SymfonyMail after SwiftMailer was removed

@ReeceM
Copy link
Collaborator Author

ReeceM commented Nov 21, 2022

I'm going to open a new PR to make MailEclipse work with SymfonyMail after SwiftMailer was removed

Okay cool 💯 I'll merge this and tag it

@ReeceM ReeceM merged commit 51a3efd into master Nov 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Review Needed V3 Version 3 of MailEclipse (Laravel 8) v4 Laravel 9 support version WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Method App\Mail\RepoTestMail::build() does not exist
2 participants