[WIP][FEATURE] Allow dependency injection in case of helper classes #668

zoliszabo · 2019-06-18T20:01:27Z

Recently had to briefly customize the functionality CssConcatenator and there was no way to do it without modifying the "core" Emogrifier.php file in the vendor folder.

Hence my proposal to make the used "helper" classes inject-able:

Made the modification in a single place only, just to showcase it;
It allows users of the library to inject a custom subclass of CssConcatenator;
It falls back to the default implementation, when there is no explicit injection of a custom implementation;
Also modified the visibility of $mediaRules property (from private to protected) in CssConcatenator, so sub-classes have access to it.

How to use it:

class CustomCssConcatenator extends CssConcatenator
{
    // Override / implement additional logic
}

...

$emogrifier = new \Pelago\Emogrifier($html);
$emogrifier->setCssConcatenator(new CustomCssConcatenator());
...

Should we use interfaces (like CssConcatenatorInterface would declare two methods - append() and getCss()), which will be used as the argument type of the injector method and all implementations (starting with the default ones ofc) will implement those interfaces. Otherwise all additional implementations must extend the default ones provided by Emogrifier.

What do you think? Let's discuss.

Thanks!

…tor`

JakeQZ · 2019-06-22T00:59:53Z

I have also found the need to make a hacked version of that class for debugging/testing (to add whitespace and newlines to the CSS so it is readable).

I found that for the extended class I would probably like the methods getCss, getMediaRuleCss, and getRuleBlockCss, as well as the property $mediaRules to be protected rather than private. (Though actually it was possible with the closure hack.)

And a method to 'inject' the extended class. I added a protected method createCssConcatenator to CssInliner that could be overridden, but a method to set it may be more convenient. However, if it is used more than once, reusing the same object may be problematic as it would need resetting; it may be better to be able to set the class name rather than class object.

The drawback of doing this and making methods protected is that it makes them public in the real sense, so it would not be possible to change the interfaces in future (unless backward-compatible) without a new major version and a release note.

zoliszabo · 2019-06-23T18:49:54Z

I found that for the extended class I would probably like the methods getCss, getMediaRuleCss, and getRuleBlockCss, as well as the property $mediaRules to be protected rather than private. (Though actually it was possible with the closure hack.)
...
The drawback of doing this and making methods protected is that it makes them public in the real sense, so it would not be possible to change the interfaces in future (unless backward-compatible) without a new major version and a release note.

You're right about protected methods technically being part of the "interface" of a library, but as far as I understand from what is written out there about SemVer, the public API (of a library) is not necessarily defined by the public (& protected) methods of the code itself, but it can be a well-documented subset of it. Thus, as long as we document it well that protected methods of class X can still change, I think it should be OK even if we "open" the default implementation using protected instead of private.

Nevertheless, I think an even better approach would be to define interfaces - i.e. in this case there should be a CssConcatenatorInterface, which would define the two public methods any implementer class must have: append() & getCss(). Of course, extending the default implementation is still an option, if you want to reuse some of its internal" functionality, with the "risk" that you will have to pay attention for eventual changes of the parent class.

And a method to 'inject' the extended class. I added a protected method createCssConcatenator to CssInliner that could be overridden, but a method to set it may be more convenient. However, if it is used more than once, reusing the same object may be problematic as it would need resetting; it may be better to be able to set the class name rather than class object.

I may misunderstood you, but I am not sure I get the difference between injecting an object versus injecting a class name. In any case, I prefer injecting objects as it is more versatile (e.g. think of implementer classes that have constructor arguments - Emogrifier/CssInliner couldn't handle those).
But why not extract the "resolve" logic of the CSS concatenator object into its own overridable protected method anyway; it wouldn't hurt in any case (actually did it here: 6e26405).

JakeQZ · 2019-06-23T21:48:44Z

I may misunderstood you, but I am not sure I get the difference between injecting an object versus injecting a class name. In any case, I prefer injecting objects as it is more versatile...

Injecting a class name is like injecting a class factory (PHP allows class names to be passed around without the need for a specific factory class). Though a class factory would have more versatility.

The point I'm trying to make is that CssConcatenator is not originally intended to be used like a singleton. After it has been used, it will contain the data from that usage. So if it were to be re-used, it would need some kind of reset method. It is only happenchance if it is actually used only once (which I assume to be the case if the tests all pass).

The current design is that a new object instance is created each time it is required, and discarded after use (there is no need for it to persist).

So I think if consumers were to be allowed to set some property (as well the alternative of overriding Emogrifier/CssInliner with a subclassed get/create/resolve method), that property would have to be a class name or class factory. Or CssConcatenator would have to be changed to be resettable, which I am not particularly keen on as it would mean keeping a redundant object in memory, changing the design pattern, and implementing extra methods and tests.

oliverklee · 2019-06-25T11:44:05Z

A few thoughts:

I like the approach.
We should define an interface that the inject method expects.
I agree that CssConcatenator must not be a singleton. However, I think it's okay if we shift the responsibility to injecting a fresh instance to the environment that provides an alternative implementation. (We should also document this in the inject method.)
I'd prefer to have this in the new CssInliner class only for two reasons: 1. I'd like the Emogrifier to be extended as little as possible as it's complex and unwieldy. 2. Unlike Emogrifier, CssInliner is intended to be used with a fresh instance for reach HTML document, which might help with the Singleton question.

JakeQZ · 2019-06-26T01:05:14Z

We should define an interface that the inject method expects.

So, are we leaning towards a method to be overridden in an extended CssInliner, rather than a property to be set (via a get/setter)? I think the 'method' approach would be simpler to implement.

I think it's okay if we shift the responsibility to injecting a fresh instance to the environment that provides an alternative implementation.

The default implementation would also need to take that responsibility, though probably as simply as return new CssConcatenator();.

CssInliner is intended to be used with a fresh instance for [r]each HTML document, which might help with the Singleton question.

I'm not sure that should be allowed to help. Although CssConcatenator is only used in copyUninlineableCssToStyleNode which (normally) is only called once, no-one (in future maintainance) would expect a side-effect that copyUninlineableCssToStyleNode produces different results if called a second time because it's reusing a dirty object.

oliverklee · 2019-06-26T10:59:45Z

So, are we leaning towards a method to be overridden in an extended CssInliner, rather than a property to be set (via a get/setter)? I think the 'method' approach would be simpler to implement.

I'm confused …

I was thinking about something like this:

class CssInliner {
public function injectCssConcatenator(CssConcatenatorInterface $concatenator) {…}
}

class CssConcatenatorInterface implements CssConcatenatorInterface {}

oliverklee · 2019-06-26T11:00:11Z

The default implementation would also need to take that responsibility, though probably as simply as return new CssConcatenator();.

Agreed.

oliverklee · 2019-06-26T11:02:07Z

And I agree that CssConcatenator::copyUninlineableCssToStyleNode should not have side effects. I propose we fix this separately.

JakeQZ · 2019-06-26T19:16:40Z

class CssInliner {
  public function injectCssConcatenator(CssConcatenatorInterface $concatenator) {…}
}

Ah, ok, so a method to set an object (presumably as a property), rather than a overridable method which would return said object.

class CssConcatenatorInterface implements CssConcatenatorInterface {}

Presumably this should read class CssConcatenator implements CssConcatenatorInterface?

And I agree that CssConcatenator::copyUninlineableCssToStyleNode should not have side effects. I propose we fix this separately.

It doesn't at present, but would if it were changed to use (and potentially reuse) a CssConcatenator instance stored as a property instead of creating a new one each time.

oliverklee · 2019-06-26T21:15:55Z

Presumably this should read class CssConcatenator implements CssConcatenatorInterface?

Yes, you're right. (That's what happens when I reply from a train when it's too hot … ;-) )

Added a test to confirm `copyUninlineableCssToStyleNode` does not have unexpected side-effects, i.e. it would produce the same result if called a second time on the same `CssInliner` instance. This will help ensure that the solution for #668 does not create future potential maintenance issues.

JakeQZ · 2019-07-03T18:56:16Z

And I agree that CssConcatenator::copyUninlineableCssToStyleNode should not have side effects. I propose we fix this separately.

It doesn't at present, but would if it were changed to use (and potentially reuse) a CssConcatenator instance stored as a property instead of creating a new one each time.

I've added #671 to test this. It currently passes but the current proposed implementation in the dependency-injection branch would cause it to fail, due to reusing a CssConcatenator.

JakeQZ · 2019-07-06T00:54:09Z

... so I think this calls for the factory design pattern. In most OO languages this would require a factory class. PHP would (as an alternative, still following the design pattern) allow a class name (of the objects to be created) to be set as a pseudo-factory without having to define/implement a factory interface - whether that would be benefitial is open to debate.

zoliszabo · 2019-07-10T15:54:59Z

Thinking it through, I would still revisit the idea of simple (object) dependency injection, where the to-be-injected object must implement a well-defined interface, and the idea of having a reset() method for CssConcatenator.

In the concrete case of CssConcatenator, the interface (CssConcatenatorInterface) would define 3 methods: append(), getCss() and reset(). The class is some kind of a collection/container (of CSS rules), so - imho - there is nothing wrong with having a way to empty the container. I see this solution less complicated and clearer than injecting factory classes or class names. Neither of these two really "force" the implementer of a custom CssConcatenator class to really clean up between instances (I am evil and would collect CSS rules in a static property 😈), only demanding a reset() method does.

JakeQZ · 2019-07-11T18:29:00Z

I would still revisit the idea of simple (object) dependency injection ... The class is some kind of a collection/container (of CSS rules) ...

I was also thinking that this approach would make it no longer possible to use more than one such container at the same time (if required at some future time).

However, PHP’s object clone-ing means this would still be possible – a second, separate container could be cloned from the first (in its initial, empty state).

Thus, I wonder, if we do take this approach, should resolveCssConcatenator always return a new clone, while the cssConcatenator property holds a ‘template’ object in the initial, empty state? (It might then not be necessary for a reset method.)

My main objection is Emogrifier/CssInliner being restricted to only being able to use one CssConcatentor container instance throughout its lifecycle.

Neither of these two really "force" the implementer of a custom CssConcatenator class to really clean up between instances (I am evil and would collect CSS rules in a static property 😈), only demanding a reset() method does.

I could be evil and implement reset() as a no-op 😉

zoliszabo · 2019-07-12T19:49:10Z

After more thinking...

If we limit ourselves to current functionality, then simple object dependency injection with a reset method is sufficient.

BUT if we want to make it super future-proof, then I am leaning towards injecting a factory class (of course using a factory interface - CssConcatenatorFactoryInterface). While the above described cloning method is quite a cool trick to cover our current and eventual future use-cases, I feel the factory pattern is a bit cleaner.

If you guys agree, I will submit a new PR implementing the factory pattern way.

JakeQZ · 2019-07-13T18:38:54Z

If you guys agree, I will submit a new PR implementing the factory pattern way.

Although it involves more indirection and more interfaces, I think this would be the most future-proof. We could avoid an extra class by having CssInliner (and Emogrifier) itself implement CssConcatenatorFactoryInterface and default

    $this->cssConcatenatorFactory = $this;

I don't know if that would be preferred to a separate default factory class?

zoliszabo · 2019-07-16T09:13:04Z

We could avoid an extra class by having CssInliner (and Emogrifier) itself implement CssConcatenatorFactoryInterface...
I don't know if that would be preferred to a separate default factory class?

While this is a neat idea, I would still vote for a separate class because of the following:

we do not have to duplicate code for Emogrifier and CssInliner;
(and more importantly) the default implementation would be a perfect example of how to create/use a custom implementation.

oliverklee · 2019-07-17T21:29:24Z

I am reluctant to go the factory approach now. This feels over-engineered to me in order to solve a problem we do not have (yet). Unless we really need this flexibility, please let's keep it simple. (See https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it for details.)

However, if there is a valid usecase coming up in the near future where we'll need multiple CssConcatenators, I might be persuaded otherwise. What is the usecase, and how soon will it come up?

JakeQZ · 2019-07-17T23:18:12Z

What is the usecase, and how soon will it come up?

I don't have a crystal ball. But should the situation arise it would require a new major release as a public interface is changed.

This feels over-engineered to me in order to solve a problem we do not have (yet). Unless we really need this flexibility, please let's keep it simple. (See https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it for details.)

The simplest solution is to just to add to CssInliner/Emogrifier a method that can be overridden:

protected function createCssConcatenator()
{
    return new CssConcatenator();
}

Then if anyone wants to 'inject' a different object, they extend CssInliner/Emogrifier and override that method.

Not only is it simple, it also does not hinder future development (i.e. restricting to a single CssConcatenator instance or introducing unexpected method side-effects).

However, the gist I was getting is that you are both preferring to have some property that can be set, rather than some method that can be overridden…?

zoliszabo · 2019-07-18T16:48:13Z

However, the gist I was getting is that you are both preferring to have some property that can be set, rather than some method that can be overridden…?

It's just that I named the PR "dependency injection" and tried to find a way to make it work that way - without the need for sub-classing 😊

This being said, jokes aside, I do feel that the method overriding way is the simplest solution in our current case. It helps both with "resetting" the collection of rules and (eventually) use more concatenator objects in parallel too.

oliverklee · 2019-07-24T11:10:06Z

This being said, jokes aside, I do feel that the method overriding way is the simplest solution in our current case.

I concur.

Added a test to confirm `copyUninlineableCssToStyleNode` does not have unexpected side-effects, i.e. it would produce the same result if called a second time on the same `CssInliner` instance. This will help ensure that the solution for #668 does not create future potential maintenance issues.

oliverklee · 2019-09-28T16:34:14Z

@zoliszabo Do you consider this PR ready to review, is it still a WIP, or was it only to show some prototype code to discuss?

JakeQZ

As I understand, we have subsequently agreed on a simpler solution, which is just to add a createCssConcatenator method that can be overridden to return a different or extended implementation.

JakeQZ · 2019-09-28T20:57:03Z

src/Emogrifier.php

+     *
+     * @var Emogrifier\CssConcatenator
+     */
+    private $cssConcatenator = null;


As I understand, we have subsequently agreed on a simpler solution, which is just to add a createCssConcatenator method that can be overridden to return a different or extended implementation.

JakeQZ · 2019-09-28T20:57:27Z

src/Emogrifier/CssConcatenator.php

@@ -49,7 +49,7 @@ class CssConcatenator
     *
     * @var \stdClass[]
     */
-    private $mediaRules = [];
+    protected $mediaRules = [];


This change is fine.

zoliszabo · 2019-09-30T17:56:17Z

@zoliszabo Do you consider this PR ready to review, is it still a WIP, or was it only to show some prototype code to discuss?

Hi @oliverklee, as @JakeQZ wrote above:

As I understand, we have subsequently agreed on a simpler solution, which is just to add a createCssConcatenator method that can be overridden to return a different or extended implementation.

I will implement the necessary changes in 1-2 days, then ask for your review. (Unfortunately, I had no time in the past few weeks to participate in the sustained work you both did here; I will try to gear up my pace the next days.)

oliverklee · 2020-11-23T18:46:08Z

I will switch the default git branch from master to main in a minute. GitHub then will autoclose all PRs targeted at the removed/renamed branch. Please create a new PR (from the existing PR branch) targeting main.

zoliszabo added 3 commits June 18, 2019 17:10

Support for injected CSS concatenator

136401a

Allow access to the $mediaRules for classes extending `CssConcatena…

11a83e1

…tor`

[Fix] Respect the coding standard

3ed7359

zoliszabo requested review from oliverklee and JakeQZ June 19, 2019 05:26

zoliszabo added discussion enhancement labels Jun 19, 2019

Extract CSS concatenator resolver logic into its own method

6e26405

JakeQZ mentioned this pull request Jul 3, 2019

[TASK] Test copyUninlineableCssToStyleNode side-effects #671

Merged

JakeQZ mentioned this pull request Sep 15, 2019

Mark the utitlity classes as @internal? #713

Closed

JakeQZ mentioned this pull request Sep 28, 2019

[CLEANUP] Replace most static:: with self:: #731

Merged

JakeQZ requested changes Sep 28, 2019

View reviewed changes

oliverklee assigned zoliszabo Oct 2, 2019

oliverklee closed this Nov 23, 2020

oliverklee deleted the branch master November 23, 2020 18:49

JakeQZ mentioned this pull request Nov 20, 2021

@internal instead of private? #1118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP][FEATURE] Allow dependency injection in case of helper classes #668

[WIP][FEATURE] Allow dependency injection in case of helper classes #668

zoliszabo commented Jun 18, 2019 •

edited

Loading

JakeQZ commented Jun 22, 2019 •

edited

Loading

zoliszabo commented Jun 23, 2019

JakeQZ commented Jun 23, 2019

oliverklee commented Jun 25, 2019

JakeQZ commented Jun 26, 2019

oliverklee commented Jun 26, 2019

oliverklee commented Jun 26, 2019

oliverklee commented Jun 26, 2019

JakeQZ commented Jun 26, 2019 •

edited

Loading

oliverklee commented Jun 26, 2019

JakeQZ commented Jul 3, 2019 •

edited

Loading

JakeQZ commented Jul 6, 2019

zoliszabo commented Jul 10, 2019

JakeQZ commented Jul 11, 2019

zoliszabo commented Jul 12, 2019

JakeQZ commented Jul 13, 2019

zoliszabo commented Jul 16, 2019

oliverklee commented Jul 17, 2019 •

edited

Loading

JakeQZ commented Jul 17, 2019 •

edited

Loading

zoliszabo commented Jul 18, 2019

oliverklee commented Jul 24, 2019

oliverklee commented Sep 28, 2019

JakeQZ left a comment

JakeQZ Sep 28, 2019

JakeQZ Sep 28, 2019

zoliszabo commented Sep 30, 2019

oliverklee commented Nov 23, 2020

[WIP][FEATURE] Allow dependency injection in case of helper classes #668

[WIP][FEATURE] Allow dependency injection in case of helper classes #668

Conversation

zoliszabo commented Jun 18, 2019 • edited Loading

JakeQZ commented Jun 22, 2019 • edited Loading

zoliszabo commented Jun 23, 2019

JakeQZ commented Jun 23, 2019

oliverklee commented Jun 25, 2019

JakeQZ commented Jun 26, 2019

oliverklee commented Jun 26, 2019

oliverklee commented Jun 26, 2019

oliverklee commented Jun 26, 2019

JakeQZ commented Jun 26, 2019 • edited Loading

oliverklee commented Jun 26, 2019

JakeQZ commented Jul 3, 2019 • edited Loading

JakeQZ commented Jul 6, 2019

zoliszabo commented Jul 10, 2019

JakeQZ commented Jul 11, 2019

zoliszabo commented Jul 12, 2019

JakeQZ commented Jul 13, 2019

zoliszabo commented Jul 16, 2019

oliverklee commented Jul 17, 2019 • edited Loading

JakeQZ commented Jul 17, 2019 • edited Loading

zoliszabo commented Jul 18, 2019

oliverklee commented Jul 24, 2019

oliverklee commented Sep 28, 2019

JakeQZ left a comment

Choose a reason for hiding this comment

JakeQZ Sep 28, 2019

Choose a reason for hiding this comment

JakeQZ Sep 28, 2019

Choose a reason for hiding this comment

zoliszabo commented Sep 30, 2019

oliverklee commented Nov 23, 2020

zoliszabo commented Jun 18, 2019 •

edited

Loading

JakeQZ commented Jun 22, 2019 •

edited

Loading

JakeQZ commented Jun 26, 2019 •

edited

Loading

JakeQZ commented Jul 3, 2019 •

edited

Loading

oliverklee commented Jul 17, 2019 •

edited

Loading

JakeQZ commented Jul 17, 2019 •

edited

Loading