emogrifier icon indicating copy to clipboard operation
emogrifier copied to clipboard

Pass CssInliner as a dependency

Open SpazzMarticus opened this issue 6 years ago • 5 comments

Since CssInliner has to be created by passing html to ::fromHtml it can not be passed as a dependency.

Current version 3.0.0 in a PSR-7/PSR-15 example:

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Server\MiddlewareInterface;
use Pelago\Emogrifier\CssInliner;

class CssInlineMiddleware implements Psr\Http\Server\MiddlewareInterface{
    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface {
        $response = $handler->handle($request);
        $inlineHtml = CssInliner::fromHtml($response->getBody()->getContents())->inlineCss()->render();
        $response->withBody($inlineHtml);
    }
}

One simple solution I can think of would be a factory a la:

namespace Pelago\Emogrifier;

class CssInlinerFactory
{
    public function generate($unprocessedHtml): CssInliner {
        return CssInliner::fromHtml($unprocessedHtml);
    }
}

Any chance you could make passing CssInliner as a dependency as simple as it was with the now deprecated Emogrifier.

Cheers!

SpazzMarticus avatar Dec 12 '19 11:12 SpazzMarticus

@SpazzMarticus Thanks for the feature proposal!

Would you be willing to create a PR for this?

oliverklee avatar Dec 12 '19 11:12 oliverklee

Of course! ~~PR coming soon.~~ #826

SpazzMarticus avatar Dec 12 '19 11:12 SpazzMarticus

Continuing the discussion from #826...

(An interface would help if there were multiple packages with different APIs that provide the same functionality as this repo does.)

Or if one wanted to allow functionality to be provided via an optional package (e.g. "suggested" in composer.json)? I have no idea how prevelant a use case that might be.

If, instead of calling CssInliner::fromHtml , one wants to avoid the static method call and/or the thight coupling, they might want to implement their own service, helper-class, or what-ever-class, with an API that fit's their use case.

Simple use case: A global stylesheet should be applied to user-generated html code.

class StylesheetService
{
    protected string $stylesheet;

    public function inlineStylesheet(string $html): string
    {
        return CssInliner::fromHtml($html)->inlineCss($this->stylesheet)->render();
    }
}

I was coming round to the idea that this would be a good general solution to the problem, and could be included in the package.

The documuntation gives a basic example and a more fully-functional example of usage (which could in theory become concrete implementations provided by the Emorgrifier package).

In my use case, I have (where $this is an instance of PHPMailer)

// Emogrify
$cssInliner = CssInliner::fromHtml($this->Body)->inlineCss();
$domDocument = $cssInliner->getDomDocument();
HtmlPruner::fromDomDocument($domDocument)
  ->removeElementsWithDisplayNone()
  ->removeRedundantClassesAfterCssInlined($cssInliner);
$this->Body = CssToAttributeConverter::fromDomDocument($domDocument)
  ->convertCssToVisualAttributes()
  ->render();
// Minify
$minifier = new Minify(false); // private/external class
$this->Body = $minifier->processBuffer($this->Body);

So a concrete implementation that did precisely the above (up to the Minify comment) would be quite useful, and the suggestion of such might also persuade better code factoring. You will also note that the (external) Minify class also does an HTML-to-HTML transform, so in an ideal world these could be flexibly chained as 'strategies' somehow.

Also, @thomasfw made a good point:

Dependency injection is so commonplace that this should be possible without requiring the implementor to create their own.

...

become topic of discussion

Discussion is always good. Coding is surely 90% discussion, 10% implementation? -- to make sure the implementation (or lack of) is well thought out; and avoid having to revisit the same old issues time after time (maintenance burden*).

Call me fickle, but I would not be so hasty to close this now, particularly with at least two people looking for a solution. @oliverklee, @zoliszabo, @jjriv, @thomasfw, @SpazzMarticus, what do you think?

(*Maintenance burden can come from both writing code - that needs to be maintained - and not writing code - that should have been written to address the potential haemorrhage of issues. It has been quoted: "the best code is the code not written.")

JakeQZ avatar Apr 24 '20 06:04 JakeQZ

Yes, I'd like to have this (in some way or other). I'm thinking of some thin facade class that can be instantiated (and that might implement some interface or not) and that knows the different HtmlProcessors, but does not relay any other methods than those needed for creating the HtmlProcessors and maybe passing some data (so that we do not need to duplicate all the option setting methods). We might also look at other libraries how they do that so that we could have a common interface, e.g.:

  • https://github.com/northys/CSS-Inliner
  • https://github.com/christiaan/InlineStyle
  • https://github.com/tijsverkoyen/CssToInlineStyles (this is the biggest player as far as Packagist downloads are concerned as it's required by Laravel: https://packagist.org/packages/tijsverkoyen/css-to-inline-styles/dependents?order_by=downloads)

Does this make sense?

oliverklee avatar Apr 24 '20 13:04 oliverklee

Continued here: #994

oliverklee avatar Apr 17 '21 14:04 oliverklee