opened
tremby
username_0
The link resolver is now passed to the HTML serializer, as the third parameter. This allows HTML serializers to be a little more flexible and reusable.

The type of the HTML serializer is now explicitly described as "callable", since this shows that it can be not only a lambda but also any other kind of callable like a string function name, an array of class name and method or object and method or an object which implements the __invoke method.

The HTML serializer is now passed to the method which turns spans within blocks into HTML as well as the method which turns blocks into HTML. This allows finer control.

However, since internally blocks and spans are currently turned into HTML quite differently (blocks with string manipulation and spans with the DOM) an implementation of HTML serializer needs to handle each type differently. In particular, it must return a string of HTML if the $element passed to it is an instance of BlockInterface and a DOMElement if $element is an instance of SpanInterface. As before, it can also return null to skip this element and defer to the built-in serialization routines.

Some documentation has been added to this effect.
Allow HTML serializer to also work on spans within blocks
issue
created
tremby
username_0
Obviously it's not ideal that the HTML serializer needs to work in two different ways. One solution would be to change over to using DOM across the board to build HTML. If that sounds good I may find some time to implement it.
comment
created
tremby
username_0
Any word?
comment
created
erwan
username_1
Hi Bart,

Yes, we need to have the HTMLSerializer work with spans too. That's how it works in other kits. I've recently made a few bug fixes to the way serialization works in the PHP kit, so I need to adapt the fix to have spans support in the HTML Serializer.
comment
created
erwan
username_1
That's fixed in 1.5.8 - thank you for your help!
comment
created
tremby
username_0
I guess I didn't stress it before, but one of the important things in this PR was that the link resolver was passed to the HTML serializer. I just made a new PR for that, #108.

I'm surprised you moved away from using the DOM, since you're potentially introducing non-escaping errors. For instance if a special character eg < or " is in a label, it won't be escaped and could cause problems. (Or an ampersand, though most browsers will handle these ok.)
comment
created
erwan
username_1
Hi,

The DOM code wasn't really rock-solid in the sense that generating the HTML for StructuredText (in particular the spans from their position) is tricky if you consider they can be nested. While the DOM gives the guarantee that the generated HTML is valid, it doesn't guarantee that it is functionally correct, e.g. doesn't skip a span.

Overall as you can see in the code the algorithm is a bit complex, and it's easier to maintain if we have roughly the same algorithm in all languages.
comment
created
tremby
username_0
Can you show me an example of where it didn't work? Is there a new test
case which the old code failed, for example? I'd like to see where I went
wrong. I thought I covered the case of nested spans.
comment
created
erwan
username_1
We had cases where the spans positions got out of sync, depending on how your spans were organized..
comment
created
erwan
username_1
It wasn't your pull request, but the code we had in our repository that was using DOM.
comment
created
tremby
username_0
Yes, I wrote that code, or at least the original version of it. Can you
give an example case?
comment
created
erwan
username_1
I believe it was on this content: http://pastebin.com/vyg2ixhs
comment
created
tremby
username_0
Thanks; I'll post-mortem my code tomorrow.
comment
created
tremby
username_0
Actually, you broke it in https://github.com/prismicio/php-kit/commit/1e52533d880f705a3ba06c0a26e8c03b1629b41d -- spans lined up perfectly before that commit, in the JSON you linked, and not after.

In that commit you reused some of my DOM code but made one subtle error: you changed a call to `htmlspecialchars` (old line https://github.com/prismicio/php-kit/commit/1e52533d880f705a3ba06c0a26e8c03b1629b41d#diff-afb4af17c86cb37f05bd052b76265f67L345) to `htmlentities` (new line https://github.com/prismicio/php-kit/commit/1e52533d880f705a3ba06c0a26e8c03b1629b41d#diff-afb4af17c86cb37f05bd052b76265f67R317).

When you escape more than the bare minimum (`htmlspecialchars` escapes much less by default than `htmlentities`) and pass into `createElement`, it automatically makes not just one textNode inside as the rest of my code was expecting, but also an entity reference node for each entity. My code didn't support looking through these. I should have clarified that in the original code or, better, explicitly made and inserted the text nodes (since doing it that way the escaping is automatic).
comment
created
erwan
username_1
Thank you for finding where the bug came from!
comment