[node:summary] token's value get trimmed incorrectly

Updating summary.

Probably this will not end up in 8.2.x; moving to 8.3.x

First, needs to be fixed on 8.4.x and after can be cherry-picked to 8.3.x.

looks great but this needs tests.

Thanks.

Added tests

Huh, DrupalCI PHP does not have mbstring extension enabled causing new tests to fail.

Unicode::substr() expects string input, and not a \Drupal\Component\Render\MarkupInterface

Also fixed coding standards issues

Hi! Taran2L

It looks great, just a few comments:

1. /**
   +   * User account used as node author.
   +   *
   +   * @var \Drupal\user\Entity\User
   +   */
   +  protected $account;
   +
   +  /**
   

   We need to keep the changes in the scope and at the minimum (so the core commiter can review this easier and quicker) let's revert this change, and all the $this->account changes, if you think to we really need these changes let's create a follow-up.

2. /**
   -   * Creates a node, then tests the tokens generated from it.
   +   * Tests all node tokens.
       */

   The same as above, not sure if it is really necessary change the comment.

3. -      $this->assertEqual($output, $expected, format_string('Node token %token replaced.', ['%token' => $input]));
   +      $this->assertEqual($output, $expected, new FormattableMarkup('Node token %token replaced.', ['%token' => $input]));
   

   The same as above, There is already an issue to replace all the format_strings methods (see #2228741: Replace calls to format_string() with Drupal\Component\Render\FormattableMarkup), so let's revert this as well.

4. . 'Fusce tempor tincidunt lorem vitae mollis.'; // 828
   Put the comment int is own line above the variable please (per https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...).

5. // Token [node:summary] should not pickup trim length setting of any other
       // formatter rather than "Summary or trimmed", even if setting name matches.
       // Fallbacks to "Summary or trimmed" default trim length setting (600).
       $view_display->setComponent('body', [
         'type' => 'text_trimmed',
         'settings' => ['trim_length' => 42],
       ]);
       $view_display->save();
       $expected_trimmed_to_600 = Html::escape('<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. '
           . 'Morbi diam dui, finibus et purus ac, elementum pretium augue. Fusce lacus nisl, feugiat sit amet blandit sed, mattis eu ligula. '
           . 'Duis laoreet dui non felis maximus fringilla. Maecenas tempor magna id urna dapibus, in elementum mauris consequat. '
           . 'Praesent in urna non felis fringilla ullamcorper interdum eu risus. '
           . 'Vestibulum id erat ultrices, varius est sed, dignissim dui. Vestibulum a dapibus nisl. '
           . 'Maecenas vestibulum nibh a aliquet mollis. Donec ac justo eget justo interdum faucibus. '
           . 'Aenean sit amet finibus turpis.');
       $this->assertNodeSummaryTokenReplacement($node, $expected_trimmed_to_600);

   This should also trims the string, no? I mean, the text_summary_or_trimmed just extends thetext_trimmed formatter:

   class TextSummaryOrTrimmedFormatter extends TextTrimmedFormatter { }
   

   So for this specific formatter the trim should work as well, at least for these two should work.

   Also the $expected_trimmed_to_600 is an invalid markup there is a </p> missing at the end of the variable.

   We still need to test that the value is trimmed correctly if the formater is text_summary_or_trimmed or text_trimmed and test if the trim_length is ignored if a different formated is used.

6. -    foreach ($tests as $input => $expected) {
   -      $output = $this->tokenService->replace($input, array('node' => $node), array('language' => $this->interfaceLanguage));
   -      $this->assertEqual($output, $expected, new FormattableMarkup('Node token %token replaced for node without a summary.', ['%token' => $input]));
   -    }
   

   Why did you deleted this part? this is where actually all the tokens are tested, now we are just testing the summary. can we please revert this part?

Great work!

Thanks.

Hi @gnuget,

Thanks for the review.

However I can't agree with couple items.

1 and 3 - this is minor refactoring. I don't think spamming drupal issue queue is a good thing. Reviewing multiple tickets instead of a single one takes less time
2 - test method description should include what is being tested, not how we do. Also check a new test method, this naming has a lot of sense IMO
4 - ok
5 - please take a look how [node:summary] token is being generated. It uses summary or trimmed version of body
6 - please apply patch and check how it works or pay more attention reviewing it in browser. It does test all the tokens and node summary

Thanks

1,2,3 I just wanted to help here, I've been asked to revert that types of changes before in other core issues (especially 3), if you don't want to it's ok, in the end, who decide if this is or not in the scope is the core commiter, as I said I just want to help :-)

5. I can see now that the text_summary_or_trimmed is being tested thanks, but I have doubts about this:

// Token [node:summary] should not pickup trim length setting of any other
  // formatter rather than "Summary or trimmed", even if setting name matches.
  // Fallbacks to "Summary or trimmed" default trim length setting (600).
  $view_display->setComponent('body', [
  'type' => 'text_trimmed',
  'settings' => ['trim_length' => 42],
  ]);
  $view_display->save();
  $expected_trimmed_to_600 = Html::escape('...');
  $this->assertNodeSummaryTokenReplacement($node, $expected_trimmed_to_600);

Not sure if I'm not understanding something but here you are using the text_trimmed formatter? because Drupal\text\Plugin\Field\FieldFormatter\TextSummaryOrTrimmedFormatter is empty and just extends TextTrimmedFormatter so that is why I was wondering if this formatter should behave as TextSummaryOrTrimmedFormatter and it should take in account the `trim_length` value (42) and trim the summary because right now you are testing to it's length should be 600.

So basically what I'm saying is that we might want to include text_trimmed in the node.tokens.inc file:

if ($display_options && $display_options['type'] === 'text_summary_or_trimmed') {
  $length = $display_options['settings']['trim_length'];
}

What do you think?

6. My bad, sorry!

Thanks!

Hi @gnuget,

I might have been a little bit too sharp in the previous comment . Sorry.

I can revert 1,2,3 - but let's have some other opinion on them

5 - At the moment trim length defaults to setting of `text_summary_or_trimmed formatter` (not text_trimmed). The values are both 600 now, nut could change in the future. Allowing both formatters creates an inconsistency with a fallback value. What do you think?

$display_options = entity_get_display('node', $node->getType(), 'teaser')->getComponent('body');
if (isset($display_options['settings']['trim_length'])) {
  $length = $display_options['settings']['trim_length'];
}
else {
  $settings = \Drupal::service('plugin.manager.field.formatter')->getDefaultSettings('text_summary_or_trimmed');
  $length = $settings['trim_length'];
}

New patch addressing 4 attached

Because of: #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase #15 doesn't apply anymore.

Here a straight reroll.

5 - At the moment trim length defaults to setting of `text_summary_or_trimmed formatter` (not text_trimmed). The values are both 600 now, nut could change in the future. Allowing both formatters creates an inconsistency with a fallback value. What do you think?

The trim_length used on text_summary_or_trimmed and in text_trimmed is the same (literally both use the same variable) If you change one the other is changed as well because is the same.

So right now if the user is using the formatted "trimmed" in the teaser view mode and set the "trimmed limit" to 250 the summary token will use this value and I think that is the expected behavior.

With the suggested fix on #4 this won't work in this way anymore, the summary token won't use the trim_length if the user is using the "trimmed" formatted and will use the default value (600), and the only way to set a different value will be forcing the user to use the "text_summary_or_trimmed" formatted in the body in the teaser view mode.

So I think we should update node.tokens.inc with:

if (isset($display_options['type']) && isset($display_options['settings']['trim_length']) && in_array($display_options['type'], ['text_summary_or_trimmed', 'trimmed'])) {
  $length = $display_options['settings']['trim_length'];
}

An alternative (not sure how easy would be to do it or even if it is a good idea) is making the [node:summary] token use the same formatter and not just the trim_length value, in this way we can at least guarantee that the token will contain exactly the same value as the body in the teaser view mode, and the expected behaviour using the Smart trim would be having a teaser with a specific number of words.

Another alternative would be make the Smart trim module stop using the trim_lenght and use an a different setting variable.

At this point not sure what is the best thing to do. :-/

Thanks for the reroll.

Add Trimmed as a second option for [node:summary] token.

An alternative (not sure how easy would be to do it or even if it is a good idea) is making the [node:summary] token use the same formatter and not just the trim_length value, in this way we can at least guarantee that the token will contain exactly the same value as the body in the teaser view mode, and the expected behaviour using the Smart trim would be having a teaser with a specific number of words.

Another alternative would be make the Smart trim module stop using the trim_lenght and use an a different setting variable.

This might be a good idea. Could be a follow-up issue.

Looks great, just i have two doubts.

1. +                if ($display_options && ($display_options['type'] === 'text_trimmed'|| $display_options['type'] === 'text_summary_or_trimmed')) {
                      $length = $display_options['settings']['trim_length'];
                    }

   is there a case when $display_options exists but $display_options['settings']['trim_length']; doesn't? I mean this is never going to fail? maybe using a custom formatter which doesn't rely on trim_lenght will make it fail?

Html::escape('<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Morbi diam dui, finibus et purus ac, elementum pretium augue.');
Before trim the output shouldn't remove tags or do something to avoid having an empty

tag? even if it is escaped it will looks weird in the summary token no?

Thanks!

maybe using a custom formatter which doesn't rely on trim_lenght will make it fail?

We explicitly expect text_trimmed or text_summary_or_trimmed formatter only. Both have trim_length. I think extra check is unnecessary.

Before trim the output shouldn't remove tags or do something to avoid having an empty tag? even if it is escaped it will looks weird in the summary token no?

This is interesting question. I believe this test runs in a very limited environment with only a few modules enabled, which makes the output not valid html (seems like html corrector filter is not being invoked)

In text.module:

// If the htmlcorrector filter is present, apply it to the generated summary.
if (isset($format) && $filters->has('filter_htmlcorrector') && $filters->get('filter_htmlcorrector')->status) {
  $summary = Html::normalize($summary);
}

And by default plain_text filter format does not have html corrector. See filter.format.plain_text.yml

We explicitly expect `text_trimmed` or `text_summary_or_trimmed` formatter only. Both have `trim_length`. I think extra check is unnecessary.

It is sometimes hard to me see this type of things. Thanks for your explanation.

#22 agree as well.

I haven't any other doubt, to me this is RTBC, we just need more opinions about 12.1, 12.2 and 12.3.

Thanks!

This is quite interesting comment to be honest.

[node:summary] uses this, when no summary is provided:

  $output = $item->processed;

and processed value contains already processed text, and then text_summary() applies filters for the second time!

Will update patch soon!

New patch that addresses issue with double escaping.

Minor naming fixes

I've re-rolled to 8.7.x.

Unfortunately the latest patch does not appear compatible with 8.8.x. Any chance of a re-roll?

Given that Smart Trim has over 50,000 installs and that [node:summary] is used for meta descriptions by the Metatag module, might I suggest this core bug could be adversely affecting the SEO of many thousands of Drupal sites.

I have done the patch on Drupal 8.9.x

I have also tried to fix the failed tests.

I have again uploaded a patch.

I've re roll to 8.9.1.

Fixed the failed test cases and re-rolled in 9.1.x

Just tested this out on Drupal 8.9 and it solved a bug I was running into.

I'm going to remove the "needs tests" tag here since there seem to be tests now, but if the tests are insufficient in some way perhaps that tag should come back.

Needs work for failed patch.

Will prioritise this with assistance.

A re-rolled patch against 9.3.x. Please review.

This patch is doing way more than outlined in the IS.

All of the feedback in #12 needs to be addressed, and the "Lorem Ipsum" text which is causing CSpell failure must be replaced with something else (make sure not to use anything under copyright).

#48 appears to work for 9.4.3

Addressed Drupal CS issues of patch #48.

There are lots of unknown words being used in core/modules/node/tests/src/Kernel/NodeTokenReplaceTest.php file.
So can we use cspell:ignore words or we will add these in the dictionary?

Per #49, those aren't English words and shouldn't be added to the dictionary, we need a piece of text other than Lorem Ipsum in there.