Problem/Motivation

We noticed that ampersands were being double-escaped when smart_trim was involved with processing a field for display via EntityViewDisplay. This happened when the render array that was created had #format set to NULL.

The format is expected to be provided by $item in the viewElements() function of the SmartTrimFormatter.

See https://git.drupalcode.org/project/smart_trim/blob/8.x-1.2/src/Plugin/Fi...

Proposed resolution

At the moment I've written a workaround using hook_entity_display_build_alter to set the #format to 'basic_html' when #formatter is set to "smart_trim". But it feels like the format should be set in the configuration for the field or field formatter somewhere...

If someone could show me the correct way to set #format for the smart_trim field formatter that would be great, thanks!

Here's some code which seems to work for us at the moment:

/**
 * Fix double-escaped output.
 *
 * Set default #format to basic_html for fields processed by smart_trim.
 * (Only when the #format key exists and is set to NULL).
 *
 * Implements hook_entity_display_build_alter().
 *
 * @param array $build
 * @param array $context
 * @return void
 */
function mymodule_entity_display_build_alter(&$build, $context) {

  // Loop through fields being built for display.
  foreach (Element::children($build) as $field_name) {

    // Use local variable to work with field element.
    $element =& $build[$field_name];

    // Only consider fields being formatted by smart_trim.
    if (isset($element['#formatter']) && $element['#formatter'] == 'smart_trim') {

      // Loop over render arrays built by smart_trim.
      foreach (Element::children($element) as $item_key) {

        if (!isset($element[$item_key]['#type'])) {
          continue;
        }

        // Set default value to 'basic_html'.
        if (
          $element[$item_key]['#type'] == 'processed_text' &&
          array_key_exists('#format', $element[$item_key]) &&
          is_null($element[$item_key]['#format'])
        ) {
          $element[$item_key]['#format'] = 'basic_html';
        }

      }
    }
  }
}
CommentFileSizeAuthor
#24 3108420-fix-2.1.0-24.patch743 bytesa.dmitriiev
#12 add_format_setting-3108420-12.patch2.51 KBdennis_meuwissen
#11 add_format_setting-3108420-11.patch2.52 KBa.dmitriiev
#4 add_format_setting-3108420-4.patch2.15 KBmcgowanm
#2 add_format_setting-3108420-2.patch2.73 KBp-neyens

Issue fork smart_trim-3108420

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

code_brown created an issue. See original summary.

p-neyens’s picture

p-neyens commented
StatusFileSize
new add_format_setting-3108420-2.patch2.73 KB

At the moment there is no setting for the format.

In the provided patch i added a setting for the format. The patch is based on the field formatter "Advanced Text" of the advanced_text_formatter module.

I hope it is enough.

p-neyens’s picture

p-neyens commented
Status: Active » Needs review
mcgowanm’s picture

mcgowanm commented
StatusFileSize
new add_format_setting-3108420-4.patch2.15 KB

I tweaked the patch a bit so by default it defers to trying to use the item's format and only asserts a format when one is explicitly set.

markie’s picture

markie commented
Status: Needs review » Needs work

Hi there,
Can you please checkout the issue fork and add your patch to it? We are trying to test the fork / merge system.

Kevin.- made their first commit to this issue’s fork.

Kevin.- opened merge request !3

kevin.-’s picture

kevin.- commented
Category: Support request » Bug report
Status: Needs work » Needs review
a.dmitriiev’s picture

a.dmitriiev commented
Status: Needs review » Reviewed & tested by the community

The patch #4 works for me, thanks. As by default the format is plain_text, make sure that you changed it to some that has HTML processing so that the problem with encoded ampersands is solved, because plain_text still shows them encoded for me (maybe it is some cache related problem).

Also patch worked for 1.3 version.

Thanks.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented
Version: 8.x-1.2 » 2.0.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I think adding a "Format" select box to the formatter configuration will be a bit confusing for most users (including me). I don't understand why we can't just set the format to "plain_text" when it is not set - why do we need to add the Format select? If the Format select is absolutely necessary, then there should definitely be some help text explaining why it is there. Maybe even hide it in a closed-by-default "Advanced" fieldset?

Also, let's move this to the 2.0.x branch.

Finally, this needs a (unit, preferably) test.

-mike

a.dmitriiev’s picture

a.dmitriiev commented
StatusFileSize
new add_format_setting-3108420-11.patch2.52 KB

Re-roll the patch for 2.1.x branch

dennis_meuwissen’s picture

dennis_meuwissen commented
StatusFileSize
new add_format_setting-3108420-12.patch2.51 KB

The patch from #11 contains a mistake in line 65, this is probably supposed to be $format ?: $item->format instead of $format ?? $item->format. Otherwise leaving the format selector set to Default will use an empty string as the text format, which does not exist and causes a "Missing text format ." in the Drupal logs. And the output of the formatter will be blank. Attached is a patch with that issue fixed.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented

Thanks to everyone for their assistance with this one so far.

Before we move forward with a fix, we really need to write a test for this issue. In order to do that, we need a set of clear instructions to reproduce the issue. Here's what I did (on the 2.1.x branch).

  1. Created a new "Text (plain, long)" field.
  2. Set that field to be trimmed using Smart Trim.
  3. Created a node with the field that starts with the phrase "Fire & ice".

When viewing the Smart Trimmed field, the output is: "Fire & ice". I assume this is the issue.

Looking at both the MR and recent patch, I'm not sure why a new "Filter" config is necessary (or wanted). Both the patch and the MR allow the site builder to select the text format to be used for the smart trimmed content. Why should this be an option? Why shouldn't the smart trimmed content use the same filter as field? What am I missing here?

If the issue is just the fact that when a plain text field is smart trimmed, it outputs & instead of &, then that is the issue that needs to be fixed.

Thoughts?

-mike

lostcarpark’s picture

lostcarpark commented

So what I think is happening:

  • When a format is specified, Smart Trim is encoding the & but the format rules aren't generally modifying this.
  • When no format is specified, plain text is used, which encodes &, but as Smart Trim already encoded it, the resultant double occurs.

The Format selector in the patch feels like a hack to me. and it feels a bit of a blunt instrument to add a default format to the FieldWidget in case the format isn't specified.

I'm left wondering should it be Smart Trim's job to encode the & at all? If the field format is going to take care of encoding, should we not just leave encoding for the format to encode if require?

If encoding & is required when the field is formatted, then I would suggest adding a check of the format, and if not specified, convert the & back to & rather than add a new property to Smart Trim.

markie’s picture

markie commented
Issue tags: +Patch to MR

I do remember there was an issue a long time ago about the ampersand being borked after trimming, so I do want to make sure our output is encoded. I definitely don't want to add a new property or setting as we are trying to stream line the config... pondering...

either way this needs tests and the MR needs updating to the latest branch (though I think I can do it)

ultimike closed merge request !3

ultimike opened merge request !62

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented
Version: 2.0.x-dev » 2.1.x-dev
ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented
Assigned: Unassigned » markie
Status: Needs work » Needs review
Issue tags: +ddd2023

Soooo - I learned a lot about issue forks on this task - hence all the branches and what not. Thanks to @xjm at Drupal Dev Days for showing me how to create a new issue fork branch and MR - this is a skill I'll be utilizing in the future.

Regardless, I believe I found a much simpler solution for this issue, and have added tests to demonstrate.

D10 tests are passing. D9 tests will pass once #3365797: Read more link always displayed in some settings is merged into 2.1.x

-mike

lostcarpark’s picture

lostcarpark commented

I agree with the approach here. The test looks good.

The pipeline failed, but it seems to be a reflection error on TruncateHTMLTest, so I suspect the problem may be the branch you are merging against, and nothing to do with this change. Hopefully some tinkering with the repo can fix that.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me!

-mike

  • markie committed 888607d2 on 2.1.x authored by ultimike 
    Issue #3108420 by ultimike, markie, p-neyens, Kevin.-, a.dmitriiev,...
markie’s picture

markie commented
Status: Reviewed & tested by the community » Fixed

Thanks for the help. Merged MR and added to changelog for next release.

a.dmitriiev’s picture

a.dmitriiev commented
StatusFileSize
new 3108420-fix-2.1.0-24.patch743 bytes

Uploading the patch for version 2.1.0 (without test), because there is no new stable version that includes the patch yet.

lostcarpark’s picture

lostcarpark commented

Just for future reference, you can get the patch file by appending .patch to the MR URL.

For example, the patch link here is: https://git.drupalcode.org/project/smart_trim/-/merge_requests/62.patch

There is no need to upload patch files when using merge requests.

a.dmitriiev’s picture

a.dmitriiev commented

The change to tests doesn't apply to 2.1.0, that is why the patch was uploaded with only change to formatter class. I am not trying to hunt for credits here.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

norman.lol’s picture

norman.lol
he/him
Primary language German
Location Berlin
commented

There is no need to upload patch files when using merge requests.

There's one famous downside to using MR patches. Imagine someone else pushes new code to the MR. The patch will change. Now imagine someone pushes malicious code to the MR. And you pull it in without knowing.

Static patches are still the preferred method until GitLab lets us pull in patches by commit reference.

lostcarpark’s picture

lostcarpark commented

@leymannx, while I agree static patches are best, it's not necessary to upload the patch file just so it can be statically linked. I save all patches a directory in my project repository so Composer is only applying local patches. That prevents any danger from the patch (whether a file or auto-generated from MR) being modified, or just getting deleted from the issue thread, and ensures the patches being applied are what you think they are.

I do take the point that patch files are still useful for applying the change to a different version branch.

yannickoo’s picture

yannickoo
Location Berlin
commented

FYI it's also possible to open a specific commit in GitLab and then just append .patch which might be safer when downloading patches from remote.