filter check_markup does not play nice with empty text fields value

Created on 24 October 2023, 8 months ago

Hello,

I was happy to discover the check_markup filter in this module.
Unfortunately, when using like this:
{{ my_entity.my_long_formatted_text_field.value | check_markup('restricted_html') }}
If field my_long_formatted_text_field is empty, the input value transmitted to check_markup is not an empty string but an empty array.
Which leads to an
TypeError: strlen(): Argument #1 ($string) must be of type string, array given in strlen()
which breaks the page
I would suggest this module handles the empty array scenario

In the mean time, I created my own custom twig filter, that does this and also does not break the whole page in case of an error

<?php
public static function safeRenderHtml($input) {
    try {
      if (is_array($input)) {
        $input = '';
      }
      return check_markup($input, 'basic_html');
    } catch (\Throwable $t) {
      Utils::logThrowableTracedMessage($t); // this is a custom function that logs the full stack trace in dblog / syslog but does not break the whole page
      return '';
    }
  }
?>

I would also suggest that this module give the option to not break the whole page in case an error occurs with a twig filter, but return an empty string instead and logs the error...

πŸ› Bug report
Status

Closed: works as designed

Version

3.2

Component

Code

Created by

πŸ‡«πŸ‡·France Nicolas Bouteille

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @Nicolas Bouteille
  • πŸ‡·πŸ‡ΊRussia Chi

    Formatted text field consists of two properties, format and value. However, when you print something like {{ node.body.value }} for empty field Twig does not get the <code>value property as you might expected. Instead it falls back to general TypedDataInterface::getValue() method that is supposed to return all field properties as array.

    I would suggest to check the emptiness of the field explicitly as follows.

    {% if not node.body.isEmpty() %}
      {{ node.body.value|check_markup('restricted_html') }}
    {% endif %}
    

    There is a general question for such cases. Should a library thrown an error when it is misused or try to handle mistakes silently.
    I think, hiding such issues is a disservice.

  • Status changed to Closed: works as designed 8 months ago
  • πŸ‡«πŸ‡·France Nicolas Bouteille

    Hey! Thanks for the quick response :)
    Ok I respect your opinion and I get that we are not on the same page on the subject ;)
    The solution you suggest

    {% if not node.body.isEmpty() %}
      {{ node.body.value|check_markup('restricted_html') }}
    {% endif %}
    

    is too heavy for my liking to be added everywhere in my code. I prefer my custom filter that handles it all in one line.

    As far as throwing errors is concerned, my belief is that not being able to display one piece of information somewhere on a page should not break the whole page.
    So for example if while developing you don't think about testing check_markup with an empty field, and someday in production there is an empty field, the page will be completely broken.
    With the solution I came up with, only the field in error is missing from the page, but the rest of the page is ok. But the error is logged in the system, and I actually send myself an alert whenever a new error is logged, so I can know about it right away and investigate and solve it even without nobody telling me about the issue.

Production build 0.69.0 2024