Enhancement: More flexible bootstrap

[arm4b]

This will allow apart of 'converter injection' to initialize plugin this way:

new CssInlinerPlugin([
    'cleanup' => true,
    'useInlineStylesBlock' => true,
    'stripOriginalStyleTags' => true,
]);

which is equivalent of this:

$converter = new CssToInlineStyles();
$converter->setCleanup(true);
$converter->setUseInlineStylesBlock(true);
$converter->setStripOriginalStyleTags(true);

new CssInlinerPlugin($converter);

Actually I suggest it for compatibility with Yii2 framework. With this change it's possible to enable CssInlinerPlugin in this easy way in Yii2 config file:

return [
    'components' => [

        [...]

        /**
         * Mailer
         *
         * @link https://github.com/yiisoft/yii2-swiftmailer
         * @link http://swiftmailer.org/docs/messages.html
         */
        'mail' => [
            'class' => 'yii\swiftmailer\Mailer',
            'messageConfig' => [
                'charset' => 'utf-8',
            ],
            'transport' => [
                'class' => 'Swift_SmtpTransport',
                'host' => 'mail.domain.com',
                'username' => 'support@domain.com',
                'password' => '...',
                'plugins' => [
                    [
                        // connect CssInlinerPlugin
                        'class' => 'Openbuildings\Swiftmailer\CssInlinerPlugin',
                        // pass arguments to CssInlinerPlugin's constructor
                        'constructArgs' => [[
                            'cleanup' => true,
                            'useInlineStylesBlock' => true,
                            'stripOriginalStyleTags' => true,
                        ]],
                    ],
                ],
            ],
        ],
    ],
];

Existing implementation when CssToInlineStyles object is injected is already perfect.

I believe this change will be good addition that allow to use plugin in more flexible way (maybe other frameworks).

Thanks!

[hkdobrev]

That's nice! Thanks!

I would actually go with an approach which does not use dynamic parameters. Something like:

new CssInlinerPlugin($converter, [
    'cleanup' => true,
    'useInlineStylesBlock' => true,
    'stripOriginalStyleTags' => true,
]);

// or if you don't want to create a converter instance:

new CssInlinerPlugin(null, [
    'cleanup' => true,
    'useInlineStylesBlock' => true,
    'stripOriginalStyleTags' => true,
]);

---

Another thing is the default setting here: https://github.com/OpenBuildings/swiftmailer-css-inliner/pull/4/files#diff-03ae371a6f0335442e7b7fc7aa1cd3beL32

$this->converter->setUseInlineStylesBlock(TRUE);

Why not having a property in the CssInlinerPlugin class which would hold an array with configuration. The default array would be:

protected $config = array(
    'useInlineStylesBlock' => TRUE,
);

And if there is a second argument passed to the constructor it will be merged with the default and then this config will be iterated and set like you've done it.

[arm4b]

With default configuration looks even better,
I'll handle it.

[arm4b]

Check it

[hkdobrev]

@armab Great! Could you update it to support PHP 5.3 as it was before? The short array literal syntax is nice, but this is the only reason this is not working on PHP 5.3.

[arm4b]

Done.

Sorry,
I was sure Swift Mailer uses [].

[arm4b]

I see Unit Tests failed.

In this example:

new CssInlinerPlugin($converter);

should default $this->config values to be applied to $converter?

And what's about this example ?:

new CssInlinerPlugin($converter, array(
    'cleanup' => true,
    'stripOriginalStyleTags' => true,
));

Or maybe default $this->config parameters should not work (no merge with $options) when $converter is passed?

What do you think would be good behaviour?

---

I feel like we added more unneeded complexity with second parameter, instead of one dynamic one)

For example this looks weird:

new CssInlinerPlugin($converter, array(
    'cleanup' => true,
    'stripOriginalStyleTags' => true,
));

If converter already passed usually it was already constructed and there is no need of additional rules in array.

And this null looks a bit ugly here:

new CssInlinerPlugin(null, array(
    'cleanup' => true,
    'stripOriginalStyleTags' => true,
));

I'm better with default $config in class and one dynamic parameter (not 2).

[arm4b]

Final version with single dynamic parameter.

[ivank]

Hi - 10x very much for all this support, this is an incredibly great discussion. I have several points to contribute:

• the wisdom of changing default values of the inliner class, as we do at the moment is indeed highly questionable. I have to admit that this was only to help us in our specific case, so I'll rather just remove it and have that configuration custom in our own code.
• I actually really dislike setting stuff via an array. It hides the interface, hides the dependancies, cannot be automatically verified (phpanalyzer, hhvm...) and can be a place where bugs easily crop up. Other languages have support for "named arguments" which would have been perfect here, but PHP still doesn't have these.
• the Yii integration thing is a great point, however since this is specific to Yii its better to separate this from the main implementation.

So turn all of these suggestions into actionable advice, what we can do with this pull request is:

• remove the line where we set the config, as you have done
• leave all the php code that does class check so that static code analysers work properly. This means having only the "set default inliner object, if one is not passed" code in the main plugin class.
• have another class called "CssInlinerWithOptionsPlugin.php" that extends the main plugin class, but the second argument is an array with the code you have suggested.
• add a "example Yii implementation" in the docs for the CssInlinerWithOptionsPlugin class.

How does that sound? We could do this ourselves but it would take time as there's a lot of other work at the moment, so you can do it yourself it would probably be faster.

If this is implemented and merged we'll release it as 0.2 as it will be a breaking change, so it will not affect people that depend on the current implementation.

And again thanks very much for contributing, its really great watching OSS at work.

[arm4b]

I don't think additional file that will implement all this stuff is good way. This plugin is just simple wrapper for other class, no need of additional files.

Here is another implementation that gives possibility to call setters of TijsVerkoyen\CssToInlineStyles\CssToInlineStyles directly:

$plugin = new Openbuildings\Swiftmailer\CssInlinerPlugin();

$plugin->setUseInlineStylesBlock(true);
$plugin->setCleanup(true);
$plugin->setStripOriginalStyleTags(true);

That's even more flexible, because it gives possibility to manipulate $converter object during plugin lifetime.

And Yii2 config to bootstrap/use this plugin will look this way:

return [
    'components' => [
        [...]
        'mail' => [
            'class' => 'yii\swiftmailer\Mailer',
            'transport' => [
                'class' => 'Swift_SmtpTransport',
                'plugins' => [
                    [
                        'class' => 'Openbuildings\Swiftmailer\CssInlinerPlugin',
                        'cleanup' => true, // if plugin property with such name nonexistent, Yii2 will try to call setter 'setcleanup(true)' instead
                        'useInlineStylesBlock' => true,
                        'stripOriginalStyleTags' => true,
                    ]
                ],
            ],
        ],
    ],
];

[hkdobrev]

@armab Actually having properties instead of an array config makes sense. The configuration would not be hidden in string keys.

However it would be strange that this property does not anything other than sit there.

Anyways, a big 👍 for multiple setters.

[hkdobrev]

@armab Hello again! Do you think you would be able to rebase this to fix the conflicts and possibly squash the commits into one? A section in the readme for Yii configuration would be really nice as well. Thank you!

[arm4b]

@hkdobrev, nope.
Not working with PHP for 1+ year already and don't have needed environment like Yii2 to test this PR/make sure it's still actual.

[hkdobrev]

@armab Thanks and sorry for the late response! I can prepare a new PR which squashes all this and it's still attributed to you.

@ivank Do you think we still need that?

[hkdobrev]

Superseded by #3 using composition instead of array config.