sendportal-core icon indicating copy to clipboard operation
sendportal-core copied to clipboard

Laravel 10 and updates

Open sabas opened this issue 3 years ago • 9 comments

  • Upgrade dependencies to Laravel 10
  • Migrate SMTP Adapter to use Symfony Mail directly
  • [BREAKING] Use Blade templates in content: the {{content}} placeholder now needs to be written as {!! $content !!}

sabas avatar Mar 24 '23 16:03 sabas

Thanks for your PR. An error (undefined variable) occurs when using templates (on the campaign preview page) - specifically if I want to use more placeholders besides the basic "content" (for example "email" or "first_name").

That's because you're passing only the "content" variable when using Blade::render() method - see https://github.com/mettle/sendportal-core/pull/187/files#diff-6c7b787be1493ddfe8a8f8063122e80e5a251e3087949348725e89d43f6f77a2R262

This should not occur later when sending the messages, as all tags are generated then. https://github.com/mettle/sendportal-core/pull/187/files#diff-1b913a0ad17255f035ef3f36d75495934814ffe2eabec21ff21c90b683696813R47

Thanks

ppolacek avatar Jun 14 '23 07:06 ppolacek

@ppolacek if you want please add a commit to my PR :) (I am not actively using it right now)

sabas avatar Jun 14 '23 07:06 sabas

Is there anything blocking this PR that I can contribute to?

damms005 avatar Sep 06 '23 12:09 damms005

Why this repo is inactive?

shankhadevpadam avatar Oct 27 '23 05:10 shankhadevpadam

@sabas

I got this error.

Undefined constant "email"

shankhadevpadam avatar Oct 31 '23 11:10 shankhadevpadam

@shankhadevpadam what line? I don't actively use it so feel free to improve the PR.

sabas avatar Oct 31 '23 12:10 sabas

@shankhadevpadam what line? I don't actively use it so feel free to improve the PR.

Models\Campaign.php

/**
 * Get the merged content for this email, including the template content.
 */
public function getMergedContentAttribute(): ?string
{
    if ($this->template_id) {
        return Blade::render(
            $this->template->content,
            [
                'content' => $this->content,
                // placeholders
                'email' => '{!! $email !!}',
                'first_name' => '{!! $first_name !!}',
                'last_name' => '{!! $last_name !!}',
                'unsubscribe_url' => '{!! $unsubscribe_url !!}',
                'webview_url' => '{!! $webview_url !!}',
            ]
        );
    }

    return $this->content;
}

shankhadevpadam avatar Nov 03 '23 05:11 shankhadevpadam

Hi @sabas, I know you don't actively use this fork, this is mainly for informing others.

After a few months of working with this fork (unfortunately, I've modified/added a lot of things already, so I can't edit this PR directly), I've discovered several bugs - using the Blade::render method in MergeContentService (and elsewhere) is not ideal for 2 reasons:

  • it breaks compatibility, the original placeholders are used without the $ sign, if using Blade they must be used as $variables
  • if using templates, if $content contains HTML code (which it 99% does), the template needs to display $content as unescaped {!! $content !!} - you mentioned this in your first post - except that after this step, all other variables in $content will be displayed in raw format ($email will be displayed as $email, not [email protected]). That's why I would recommend going back to the original solution using str_ireplace() fn - then everything will work as it should, and compatibility will be preserved.

There are a few more details, e.g. here the $templateContent parameter should also be nullable ?string.

But otherwise this fork helped me quite a lot, so thank you 🍺

ppolacek avatar Dec 06 '23 11:12 ppolacek

@ppolacek congratulations! do you plan to release a proper fork? 🙂

sabas avatar Dec 06 '23 12:12 sabas