verbb / vizy

A flexible visual editor for Craft CMS
Other
43 stars 8 forks source link

[Craft 4.x]: renderHtml() outputs a string #127

Closed JeanLucEsser closed 1 year ago

JeanLucEsser commented 2 years ago

Description

This may be a very stupid question as I'm new to Vizy and using it in beta form with Craft 4.

Per your doc:

{{ entry.vizyField }}

{# Is the same as... #}
{{ entry.vizyField.renderHtml() }}

Here's my experience:

{{ entry.vizyField }}
Outputs html:
<p>Lorem ipsum</p>

{{ entry.vizyField.renderHtml() }}
Outputs a string:
"<p>Lorem ipsum</p>"

{{ entry.vizyField.renderHtml()|raw }}
Outputs html:
<p>Lorem ipsum</p>

Is using the raw twig filter required when using renderHtml() or am I doing something wrong?

engram-design commented 2 years ago

I'll say for the moment - yes the raw filter is required when calling renderHtml() but not for directly outputting the field with no function call. Previously, you needed to do {{ entry.vizyField | raw }} as both methods would return a HTML string, not the rendered HTML for the page. I managed to find a way to get around this with Twig, but it only applies for when using it as a string.

We're even trying to use Template::raw() for renderHtml() but that doesn't make a difference.

So technically the docs are incorrect in that they're aren't exactly the same, but trying to get to the bottom of it!

engram-design commented 2 years ago

Fixed in 2.0.1

JeanLucEsser commented 2 years ago

Hey @engram-design, sorry about this but this is not totally fixed.

When looping through the field, renderHtml() still needs the raw filter. Let's take this exemple (adapted from your doc):

{% for node in entry.vizyField.all() %}
    {# If this is a Vizy Block node, handle that differently #}
    {% if node.type == 'vizyBlock' %}
        {# shortened for brevity #}
    {% else %}
        {# Render using the default #}
        {{ node.renderHtml() }}
    {% endif %}
{% endfor %}

This won't work unless we use |raw.

Plus 2.0.1 has a related regression when looping over content inside a node and outputting text. We need to use |raw on text now.

JeanLucEsser commented 2 years ago

BTW, trying to apply attributes on the previous example won't work either. Doing this won't have any effect:

{{ node.renderHtml({
    paragraph: {
      attrs: {
        class: 'text-black'
      }
    }
  }) }}
engram-design commented 2 years ago

Right, so node.renderHtml() and field.renderHtml() are technically two different things, but that's fixed up now (I only fixed the latter - forgot about the former). Fixed for the next release. To get this fix early, change your verbb/vizy requirement in composer.json to:

"require": {
  "verbb/vizy": "dev-craft-4 as 2.0.1",
  "...": "..."
}

Then run composer update.

Plus 2.0.1 has a related regression when looping over content inside a node and outputting text.

What's the issue with this?

You've never been able to pass in classes to node.renderHtml(), only at the field-level. It's something I might look at, at some stage though, but it does get a tiny bit complicated with mark definitions - not to mention you're going to need to pass in the config for a node, every time you call node.renderHtml() instead of defining it only once at the NodeCollection level (with field.renderHtml())

JeanLucEsser commented 2 years ago

latest dev-craft-4 returns this error when trying to show a Vizy field in the CP.

[01-Jun-2022 08:23:36 Europe/Paris] PHP Fatal error:  Declaration of verbb\vizy\nodes\VizyBlock::renderStaticHtml(): ?string must be compatible with verbb\vizy\base\Node::renderStaticHtml(): ?Twig\Markup in /app/vendor/verbb/vizy/src/nodes/VizyBlock.php on line 156
[01-Jun-2022 08:23:36 Europe/Paris] PHP Warning:  Function must be enabled in php.ini by setting 'xdebug.mode' to 'develop' in /app/vendor/yiisoft/yii2/base/ErrorException.php on line 50
[01-Jun-2022 08:23:36 Europe/Paris] PHP Fatal error:  Uncaught yii\base\ErrorException: Function must be enabled in php.ini by setting 'xdebug.mode' to 'develop' in /app/vendor/yiisoft/yii2/base/ErrorException.php:50
Stack trace:
#0 {main}
  thrown in /app/vendor/yiisoft/yii2/base/ErrorException.php on line 50
JeanLucEsser commented 2 years ago

What's the issue with this?

Still related to raw, before 2.0.1 I would do this:

<p class="p-text">
    {% for nodeContent in node.content %}
            {{ nodeContent.text }}
    {% endfor %}
</p>

Now I have to use {{ nodeContent.text|raw }}.

You've never been able to pass in classes to node.renderHtml(), only at the field-level.

I'm ok with this but it feels like you should if you can do it at the field level.

I would argue that very few people needing Vizy or using it for what it is use renderHtml at the field level. Using blocks requires that you loop over nodes and default to renderHtml at the node level for paragraphs and such. I know you can define templates directly in the block settings in the CP and just do renderHtml at the field level but I'd bet few developers do it that way. I know I have my twig routine inherited from dealing with matrix. We may disagree on this though. ;)

Again, I'm not saying you should add this functionality, but it feels bizarre to be able to modify rendering at the field level and not at the node level when a field is just a collection of nodes. I know it's technically way more complex.

Anyway, not an issue for me, it was merely a thought!

engram-design commented 2 years ago

That CP-based error should be fixed in the craft-4 branch now.

So the text of a node won't necessarily be HTML, and would be correctly plain text. In your case, is text HTML content, and without | raw it doesn't render as HTML?

There's some security concerns to address if we're decoding HTML, so keen to see an example of your text value and node.

I'll say we agree to disagree on that front, but can understand where you're coming from. You should be able to render a Vizy field with a single line {{ entry.vizyField }}, passing in an object to tweak WYSIWYG nodes and their classes (think adding Tailwind classes), and then providing templates for each block. It's one of the ways we encourage people to work in a modular fashion, so that you're not constructing a giant single template with conditionals for each node, then each Vizy Block type.

But like I said, I understand where you're coming from, and I'll look at adding that functionality at the node-level anyway, because it makes sense to have that flexibility, and we can also leverage what we've implemented so-far.

JeanLucEsser commented 2 years ago

As you may know we have a lot of accentuated characters in French.

So to take an exemple, without | raw this:

Je suis accordéoniste de formation, depuis l’âge de 4 ans.

Would render as:

Je suis accord&eacute;oniste de formation, depuis l&rsquo;&acirc;ge de 4 ans.

I don't mind having to use raw but I think I shouldn't have to and why did it change between 2.0.0 and 2.0.1.

engram-design commented 2 years ago

Ah yes, makes sense. I'll need to do some further testing on this, because we encode HTML (and HTML entities like special characters) when saving to the database, and decoding when rendering. For this reason, we'd hate to render potentially malicious text content, so I'll test a little more.

engram-design commented 1 year ago

Circling back to this (sorry it's been a while!), I can see the issue with text with special characters but to my knowledge, it shouldn't actively be an issue when rendering those characters.

To illustrate, I'll start with your text: Je suis accordéoniste de formation, depuis lâge de 4 ans.. Adding those to the editor looks correct:

image

And using the following Twig:

{% for node in entry.vizyModeRichText.all() %}
    {{ node.renderHtml() }}

    <p class="p-text">
        {% for nodeContent in node.content %}
            {{ nodeContent.text }}
        {% endfor %}
    </p>

    {# Using raw #}
    {{ node.renderHtml() | raw }}

    <p class="p-text">
        {% for nodeContent in node.content %}
            {{ nodeContent.text | raw }}
        {% endfor %}
    </p>
{% endfor %}

All produce the same identical result:

image

Now inspecting the source code:

image

I can see those values are indeed encoded, but browsers will interpret those HTML entities and render them correctly.

I'm keen to know if you're seeing anything different, and if this is still an issue on the latest versions.