yiisoft / yii2-swiftmailer

Yii 2 swiftmailer extension.
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
116 stars 74 forks source link

Unable to extend `compose()`, unable to extend `render()` #14

Closed creocoder closed 9 years ago

creocoder commented 9 years ago

@klimov-paul Because of $this->_message private variable. Also all this $params['message'] workaround looks like real voodoo magic. Maybe its good idea to replace it with something smart?

samdark commented 9 years ago

What's your proposal?

klimov-paul commented 9 years ago

$this->_message private variable

Which class scope are you refering exaclty?

Also all this $params['message'] workaround looks like real voodoo magic

It is much magic as $params['widget'] in ListView::renderItem().

What exactly is the problem?

creocoder commented 9 years ago

@klimov-paul

What exactly is the problem?

The problem is class can not be extended due to bad design. While it obvious, ok, example:

class MyMailer extends \yii\mailer\BaseMailer
{
    // ...
    public function compose($view = null, array $params = [])
    {
        // my own logic, but i want use $this->render() in my implementation too
        $this->render($view['html'], $params, $this->htmlLayout);
    }
}

In child classes $message in layouts will always be null and you can't do anything with that. Check your code and you'll understand why. Look at original class compose() method:

    public function compose($view = null, array $params = [])
    {
        if (!array_key_exists('message', $params)) {
            $params['message'] = $message;
        }

        // ...
        $this->_message = $message;
        // render call
        $this->_message = null;
        // ...
    }

    public function render($view, $params = [], $layout = false)
    {
        // $this->_message used there and you can't set its value from child classes
    }

Its what i call voodoo magic which make class complettely unextensible. While i understand that you try to avoid override message key if user put it to $params this can be done with pure OOP way without such hacks.

But in general i think its complettely overcoded + ugly hacked. You can just mark message key as reserved in method documentation.

creocoder commented 9 years ago

You can just mark message key as reserved in method documentation.

Or there is 10 other methods how to solve trouble without hacks. For example extend render() signature and make $message argument, etc.

klimov-paul commented 9 years ago

class MyMailer extends \yii\mailer\BaseMailer

Well, in this case this issue does not belong to the yii2-swifmailer repository and should be reported to https://github.com/yiisoft/yii2

Check your code and you'll understand why

First of all it not my code, see: https://github.com/yiisoft/yii2/commit/a2a6028253c7669acd813caa85d26ccb59dde723 So stop blaming, put aside your emotions and concentrate on the issue, please.

The change, which you complain on, has been introduced at https://github.com/yiisoft/yii2/issues/4147 If you have a nice solution for this issue - feel free to sumit a PR.

You can just mark message key as reserved in method documentation.

It was so at the beginning.

creocoder commented 9 years ago

@klimov-paul Ok, thanks. Created issue in corresponding repo: https://github.com/yiisoft/yii2/issues/9260

So stop blaming, put aside your emotions and concentrate on the issue, please.

Sometimes its hard to put aside emotions, especially when you see hacks like:

$this->_message = $message;
// call method where $this->_message will be used
$this->_message = null;

Sorry :)