Disable HTML formatting by default

[osbre]

https://github.com/MyIntervals/emogrifier/blob/2923bdf5a78f2c8d65f961eb633bb4fd5c0cb46d/src/HtmlProcessor/AbstractHtmlProcessor.php#L243

Currently I'm solving it by using ::fromDomDocument and passing custom document, wonder if there is a possibility of making it a built-in option/parameter?

[JakeQZ]

Thanks @osbre for the feedback, suggestion, and pinpointing the exact line of code which would need changing to support it.

You would be more than welcome to submit a PR to make the change.

My initial thoughts are

• while we are there, we should cover all the DOMDocument properties that could be set, not just this one
• they could be passed via an associative array as an optional parameter to the 'constuctor' (or as is the case the static fromDomDocument method, as well as any other similar 'construction' methods where it the same applies
  • passing additional optional parameters this way is more extensible than a potentially never-ending long list of function/method parameters, and seems to be the chosen approach used by many libraries (including WordPress - if there is a better general approach in PHP to this kind of thing, I am not aware of it)
• the code itself could loop through the array of passed (optional) parameters, test if DOMDocuemnt has a corresponding property (perhaps of the same type as provided), and assign it accordingly

I think this is a great suggestion, and if expanded to the gerenal case, would allow users of the library the freedom to change any DOMDocument construction setting without us having to make any changes to the package, or them to wait for the next release.

Our release cycle of major releases coincides with that of PHP's point releases. The next one is not due for about 7-8 months. But we could make a point release to include such a change, so you don't have to wait.

@oliverklee, WDYT?

@osbre, would you be willing to submit a PR to move this forwads? The code change would need corresponding unit tests, to confirm that a selection of some common parameters of at least a couple of different types (assuming those documented are not all Boolean) do actually get set as desired. Probably just reading them back to confirm a match would suffice - I don't think it would need to confirm actual behaviour of DOMDocument beyond testing that the property is set as desired (we are not testing DOMDocument behaviour). (See the contributing guidelines for the finer points like commit message format and updating the changelog.)

Many thanks, Jake

[JakeQZ]

(perhaps of the same type as provided)

I'm not 100% sure of this - if it's a Boolean and the value that ends up being provided is (say), (int)1, rejecting it may be over-the-top, particularly given that PHP is loosely-typed, and, e.g., values from SQL databases often end up as strings when they are supposed to be numbers.

Actually my current thinking is now that the type should not be checked, and it should be left down to "programmer beware" (PHP functions don't normally fail because you passed 1 instead of true).

[oliverklee]

My first question is: What's the usecase? Why would one want to now have nicely-formatted HTML (assuming that the HTML formatting gets mangled anyway due to the emogrification)?

[osbre]

@oliverklee I have a use-case for that: my app builds email layouts dynamically, and has a CSS rule that does white-space: pre-wrap;. When I use formatting - DOMDocument's built-in formatter adds unnecessary whitespaces which my users then see. It makes a bug in my project, that's why I needed a way to disable formatting.

[oliverklee]

Then maybe we can completely turn off the autoformatting (so we don't need to have an option for this we'd have to maintain). What do you think?

[osbre]

Seems like a breaking change? Will it affect any of the current usages that people have? That's why I thought it needed to be optional

[oliverklee]

I don't consider it to be breaking, as the exact way Emogrifier reformats the HTML is not guaranteed anyway.

[osbre]

Agreed, then it makes sense to remove it. (it doesn't feel intuitive to have formatting by default)