The [node:summary] token does not output anything for body fields without a manual summary

This is a problem for Subscriptions, too.

subscribe

It is possible that node have no summary for body, because of 'text_summary_or_trimmed' format.

This patch validates is summary exists and if no it tries to get options from teaser view to get summary. If teaser view not exists or its type differs from 'text_summary_or_trimmed', that default value substituted for trim length.

#5: the-node-summary-token-does-not-output-anything-for-body-fields-without-a-manual-summary-1300920-5.patch queued for re-testing.

Thank you for your patch, gumanist!

+++ b/core/modules/node/node.tokens.inc
@@ -134,12 +134,32 @@ function node_tokens($type, $tokens, array $data = array(), array $options = arr
+        ¶

Trailing white space.

+++ b/core/modules/node/node.tokens.inc
@@ -134,12 +134,32 @@ function node_tokens($type, $tokens, array $data = array(), array $options = arr
+              // If we have teaser display use its information. If no set default value for trim length.

Improve wording of comment, e.g. "Try getting trim length from teaser display, otherwise set default value."

+++ b/core/modules/node/node.tokens.inc
@@ -134,12 +134,32 @@ function node_tokens($type, $tokens, array $data = array(), array $options = arr
+                $trim_length = 600;

I don't like the magic number, but it's just as magic in text.module, so we're following core practice.

Aside of these minor issues, the code works fine. However, I think we should avoid the code duplication between body and summary. I'm attaching a patch with the factorized code (and the changed comment) but no other changes.

+++ b/core/modules/node/node.tokens.incundefined
@@ -136,10 +136,27 @@ function node_tokens($type, $tokens, array $data = array(), array $options = arr
+            } else {

else should be on next line

+++ b/core/modules/node/node.tokens.incundefined
@@ -136,10 +136,27 @@ function node_tokens($type, $tokens, array $data = array(), array $options = arr
+                $trim_length = 600;

600 should be removed. Use text_summary() that have a special variable for this

12 days to next Drupal core point release.

When this issue will be fixed on D8, will a backport be available for D7 ? Thanks

@andypost: Thank you for your comments.

Here's an updated patch.

@FiNeX: I sure hope so...

Looks good except:
// Try getting trim length from teaser display, otherwise set defa|80 columns here

@Niklas Fiekas: Thank you for your comment.

Wrapped the comment and changed 'set' to 'use'.

Awesome.

We should have a test for this w/ the use case in the OP, no?

Not sure if webchick means an automated test, but I can confirm this patch works on a fresh install of drupal 8 with the exact use case in the summary. Added an article node with long lorem ipsum test, did not input the summary, used the following php code in a comment:

$my_node = node_load(1);
print token_replace('[node:summary]', array('node' => $my_node)); 

trimmed result is shown in the comment. If i do include a summary, it works properly.

The patch works fine here using D7.10 (tested with Meta tags module).

Added an extra step to the NodeTokenReplaceTestCase test in node.test to run tests when summary is not provided. Attached patch includes patch 14 and the new test. Probably not the most elegant way to test, but this is both my first test and one of my first core contributions so feedback is welcome!

subscribe

Well done, thank you.

+++ b/core/modules/node/node.testundefined
@@ -2298,5 +2298,33 @@ class NodeTokenReplaceTestCase extends DrupalWebTestCase {
+    // Load node (without summary) so that the body and summary fields are structured properly.

This comment is a bit too long. structured properly must be on a new line. This is a pretty minor thing. Other than that it looks good.

Edit: @CarbonPig: You no longer need to subscribe, we now have a nice green follow button above.

Fixed that comment line. Here's the new patch.

+++ b/core/modules/node/node.testundefined
@@ -2298,5 +2298,34 @@ class NodeTokenReplaceTestCase extends DrupalWebTestCase {
+    // Repeat for a node without a summary

Sorry. I didn't see this comment, that needs a trailing dot. (Now we're down to the *really* nitpicky stuff.)

Updated.

Let's also give the tesbot just the test. This should fail.

Thank you, mkadin. I think #25 is good to go.

+++ b/core/modules/node/node.tokens.inc
@@ -136,10 +136,29 @@ function node_tokens($type, $tokens, array $data = array(), array $options = arr
+              break;
+            }
+            if (!empty($items[0]['summary'])) {

It looks like a simple if/elseif/else control structure would be more readable and easier to understand instead of this break in between.

+++ b/core/modules/node/node.tokens.inc
@@ -136,10 +136,29 @@ function node_tokens($type, $tokens, array $data = array(), array $options = arr
+              if (array_key_exists('teaser', $instance['display']) && $instance['display']['teaser']['type'] == 'text_summary_or_trimmed') {
+                $trim_length = $instance['display']['teaser']['settings']['trim_length'];

Any particular reason for the array_key_exists() here?

Why isn't this checking the array key it actually tries to access? I.e.,

isset($instance['display']['teaser']['settings']['trim_length'])

That might even make the additional display type check obsolete.

Thank you, sun!

Implemented sun's suggestions and refactored redundant code.

Suppose we should extend test to check for <!--break--> probably this could be cleaned in _text_sanitize() with check_markup()

@andypost: I don't quite understand what you mean and whether this really belongs into this issue.

Please provide a patch if it does.

I applyed the patch #30 on my drupal 7.10, but nothing is displayed with metatags 7.-x.1.4, i'm using the theme omega and Display Suite.

Probably duplicate about <!--break--> #1387560: HTML correction problem with text_summary in case of <!--break-->

andypost, can we get some clarification on why you think these issues are related? This issue is about the token [node:summary] not displaying any content when no summary is inputted.

Your issue seems to be about HTML correction and the html comment break tag for summaries. Thus, it is specifically about when a summary IS inputted. Can you explain how these issues are related?

#1387560-5: HTML correction problem with text_summary in case of <!--break-->

I applied the patch on Drupal 7.12 and it works.

#30 has tests.

Here's #30 again.

#39 Patch worked on Drupal 7.12 and Metatags. Thanks!

Always patching core. ;/

#39: summary_token.1300920.30.patch queued for re-testing.

Re-rolled.

The changes are only in the surrounding lines.

Hmm, more re-rolling needed (LANGUAGE_NOT_SPECIFIED, langcode).

Interdiff against 30/39.

Shouldn't there be a different token for this like was mentioned here http://drupal.org/node/1295524#37.
I think it would make more sense if [node:summary] returned the contents of the summary field, empty or not.
Then we can create a new token such as [node:summary-or-trimmed:300] that is more self-explanatory and flexible.

#46: summary_token.1300920.46.patch queued for re-testing.

#46: summary_token.1300920.46.patch queued for re-testing.

can i ask what is holding up this patch?

#46: summary_token.1300920.46.test_only.patch queued for re-testing.

#46: summary_token.1300920.46.patch queued for re-testing.

can i ask what is holding up this patch?

The fact that no one is reviewing it.

EDIT: ... and that it needs to be re-rolled. Will do that tonight...

Here's the re-roll of #46.

Nothing has changed except for the layout of the tests.

I think it would make more sense if [node:summary] returned the contents of the summary field, empty or not.
Then we can create a new token such as [node:summary-or-trimmed:300] that is more self-explanatory and flexible.

This makes more sense to me as well.. and +1 for the flexibility;)

Trimming to specified size aside, what is the point of having [node:summary] return nothing?

I see two downsides:
1. It makes [node:summary] unusable, because you'll never know whether you're going to get anything or not
2. It's a deviation from core's normal behavior when you ask it for the summary.

I see no upsides. Why would we want to have that?

#55: summary_token.1300920.55.patch queued for re-testing.

When I have the shortcode module enabled and there is a shortcode tag in the teaser , the tag is included in the token. Does anyone knows how to remove this from the token?

@earnie: Sorry for it but I thought that there should be a [node:summary] token that strips every tag from it. But maybe that's another issue.

Rerolled.

Would love to see this patch in D7 related to: http://drupal.org/node/1295524
greetings, Martijn

Per xjm on IRC:

• Add more inline comments to node.tokens.inc.
• Provide a separate test-only file.
• Don't use t() on assertion messages.

Updated patch that resolves the issues xjm mentioned (see comment #65).

(If you put the test-only patch first, which needs to fail, then the issue won't drop to NW.)

#66: drupal-n1300920-66-tests.patch queued for re-testing.

see #68

+++ b/core/modules/node/node.tokens.inc
@@ -136,10 +136,29 @@ function node_tokens($type, $tokens, array $data = array(), array $options = arr
+                // Identify the requested length of the string.
+                $trim_length = NULL;
+                if (isset($instance['display']['teaser']['settings']['trim_length'])) {

This setting is required, do we really need to check its existence? Suppose patch needs to add #validation as positive integer for text_field_formatter_settings_form()

#68 doesn't apply to "Unable to apply patch"

Here's the needed re-roll. The only changes are in the surrounding lines.

I'm not sure about #72.

salvis, could you reroll #66 maybe?
your rerolled patch contains t() on test asserts

Ok, rerolled 66, sorry...

#72 still needs an answer.

@andypost: I'd vote to move validation of text_field_formatter_settings_form() into another issue. Also, I'd wager there are circumstances where $instance['display']['teaser']['settings']['trim_length'] is invalid.

This has one minor change: it verifies that $instance['display']['teaser']['settings']['trim_length'] is numeric before using it.

I'm ok with introducing validation in another issue, but this one should be postponed then

+++ b/core/modules/node/node.tokens.inc
@@ -136,10 +136,29 @@ function node_tokens($type, $tokens, array $data = array(), array $options = arr
+                $trim_length = NULL;
+                if (isset($instance['display']['teaser']['settings']['trim_length']) && is_numeric($instance['display']['teaser']['settings']['trim_length'])) {
+                  $trim_length = $instance['display']['teaser']['settings']['trim_length'];

Maybe better to make a hook_update_N() to make sure that it's numeric?
I think this kind of validation just silently hides errors.

I see your point. Lets remove the inline type checking and if you start another issue for the validation I'll help with it.

Filed #1808942: Add element_validate_integer_positive() validation for text_field_formatter_settings_form() trim_length

Anyone who needs this for D7 who doesn't want to either patch their core install or write a custom module for themselves should check out this sandbox that will temporarily resolve your issues: http://drupal.org/sandbox/dman/1810666

As this was opened in Oct 2011 against D7, and has been crippling metatag since then, I really wanted to get *a* fix onto a stable D7 site released today.
To avoid a core hack (by policy) I've instead rolled a stand-alone work-around as Node summary token for Drupal 7 sites who need it now.
(it uses hook_tokens_alter() to just fix the problem externally, based on the proposed code found in this issue)

This is just an FYI for anyone who finds their way here from a search. This issue can continue doing its D8 stuff - but for anyone who needs an actual D7 fix/work-around in the meantime, you may find this a clean, pluggable, non-hacky solution.

Lets try this - updated per #80.

Thanks for the patch mentioned in #83, dman. I'm amazed that this issue has been dragging on for more than a year.

That last patch looks good to me, but better to get a RTBC from one of the people involved with the issue.

#84: drupal-n1300920-84-d8.patch queued for re-testing.

Straight re-roll of #84 to get it to apply.

*ahem*

That smells nice. Test looks good!

In #16 this was kicked back for tests. It now has tests.
In #29 it was kicked back for code style, that was addressed.
There's some discussion of a new feature to provide summary-or-body, but that's a feature request and pretty off topic from this issue. Drupal core provides a summary if the user doesn't enter one in every other instance except for this token, so this is just about fixing that bug of the token breaking the pattern.
There's some more discussion of code style and Damien and andypost fixed some of that and moved some to a new issue.

I've now tested this more thoroughly and feel it's RTBC. Even though I uploaded #89 that's just a straight re-roll of Damien and other's work from earlier so that it would apply again.

Thanks greggles!

Just tagging that this will need a backport to D7 after it's added to D8.

This looks fine as a straight bugfix.

I'm wondering a bit if just rendering the field value would have helped instead, but token doesn't really appear to do that elsewhere. So committed/pushed to 8.x.

A starting point, this is the code from node.token.inc rerolled for D7. Please note that I haven't added any tests, I'm not sure where they should be added given the D8 tests had a whole separate node-tokens test file.

@DamienMcKenna: the same tests are in modules/node/node.test

Here's a reroll with the tests straight from D8, with following changes:
- LANGUAGE_NOT_SPECIFIED => LANGUAGE_NONE
- $node->langcode => $langcode
- $language_interface => $language

Thanks, lyricnz. Seems like RTBC to me.

I was hoping that the patch in #96 would fix an issue I had with metatag but it's not totally the case. The metatag module use by default the [node:summary] for some opengraph and this leads to the notice:

Notice : Undefined index: summary dans node_tokens() (ligne 142 dans /.../modules/node/node.tokens.inc).

Applying the patch this notice is replaced by the following:

Notice : Undefined index: trim_length dans node_tokens() (ligne 152 dans /.../modules/node/node.tokens.inc).

I don't know if this special case should be handled in this patch or if it's an issue of metatag module. At least the recommended trim length for the summary metatag is ~150 chars, not really the default body trim length.

Needs work for the trim_length notice.

So here is the same patch as #96 but with a little condition on the trim_length. I've defined it to be 300 by default, not sure if this is correct.
This gives:

$instance_settings = $instance['display']['teaser']['settings'];
$trim_length = isset($instance_settings['trim_length']) ? $instance_settings['trim_length'] : 300;
// Generate an optionally trimmed summary of the body field.
$output = text_summary($output, $instance['settings']['text_processing'] ? $items[0]['format'] : NULL, $trim_length);

For 6.x the default was 600 http://drupalcode.org/project/drupal.git/blob/refs/heads/6.x:/modules/no...

For 7.x I think it should be available per node type and is also 600.

I didn't purposefully remove those. Hmmm.

The D7 and D8 text_summary() defines the default trim length:

function text_summary($text, $format = NULL, $size = NULL) {
  if (!isset($size)) {
    // What used to be called 'teaser' is now called 'summary', but
    // the variable 'teaser_length' is preserved for backwards compatibility.
    $size = variable_get('teaser_length', 600);
  }

NULL should be passed if it's unspecified (i.e. change 300 to NULL in #100). We already had that 50 posts ago, but it got lost on the way.

The committed D8 patch in #89 produces the same notice if ['trim_length'] is not set.

Thanks @salvis. Here is an updated patch based on #55. There is also a d8 patch with just the default trim length added.

So the default trim length was removed since #79. However what if I configure my body field to appear untrimmed in teaser display mode (and others)?

D8 needs to go first anyway and it doesn't have a test yet.

#104: drupal-n1300920-104-d8.patch queued for re-testing.

Looks like it has been fixed for d8 in another issue:
#1852966: Rework entity display settings around EntityDisplay config entity

the commit: http://drupalcode.org/project/drupal.git/commitdiff/228831482600bb631ff2...
(search for "$settings = field_info_formatter_settings('text_summary_or_trimmed');" )

So maybe this can go back to d7?

If D8 has been fixed in a separate issue we gotta backport it to D7, at least to get Metatag closer to a final release, thoughts?

@alexweber - d8 was (partially) fixed in this issue and it still needs work before it can be backported. Maybe you can help with that?

@greggles, sure lemme check it out :)

The patch from #104 rerolled, the only difference that the patch to node.tokens.inc was bumped 29 lines.

Great patch. I'm not sure where this fits in, but if you are using media module to embed images this cleanup comes in handy

...
// Generate an optionally trimmed summary of the body field.
                $output = preg_replace("/\[\[.*?\]\]/s", '', $output);
                $output = text_summary($output, $instance['settings']['text_processing'] ? $items[0]['format'] : NULL, $trim_length);
              }
            }
            $replacements[$original] = $output;

We don't need the "backport to d7" tag anymore.

@leanderl: That will be handled automatically by the text_summary() function which will run the appropriate text format over the string.

@DamienMcKenna: I'm not sure, maybe I'm wrong. But does the text_summary() remove the image reference or does it turn it into an img tag?

The purpose for me with the snippet was to remove the "curly code" generated by media module, wanting to only have text characters in the summary. In some cases that I've encountered Wysiwyg editing with inline images leaves you with images inserted within the first paragraph of a body field, which is problematic for teasers, rss, meta tags etc. My use case was to grab a summary and then put it in a meta description.

@leanderl: Yes, the text_summary will leave in any tags that the text format defines appropriate, so for your use case you might look to using hook_tokens_alter() to run strip_tags() on the output of node:summary.

Here's a way to get a clean summary to use for the meta description tag (Metatag module). Don't forget to add the Media input filter in the plain text format, before HTML removal filter.

Ideally, this might be made available in a new token.

                // Generate an optionally trimmed summary of the body field.
                // Remove any unwanted non-HTML markup (Media,...).
                $output = check_markup($output, 'plain_text', $field_langcode);
                // Remove any remaining HTML.
                $output = strip_tags($output);
                // Trim text.
                $output = text_summary($output, NULL, $trim_length);

As another temporary solution, I've turned the patch from #114 into a patch for Metatag that will be in the next beta release.

I take it that since I'm using D7.22 and having this issue that this pretty important Oct. 2011 issue still persists? I'm going to try the #114 patch and see. Does this need to be marked reviewed and tested so it can be included in D7.23?

I just reviewed the patch in #114 and code-wise it looks great. It also comes with tests. So it would just need someone to try it out manually and then this can be set to RTBC.

Does not apply to Drupal 7.23.

I've attached a simple reroll of #114, for Drupal 7.23.

( The only difference as far as I can see was due to this commit, related to removing t() in node tests. )

Tested the patch in #126 and it seems to work fine. By the way is there any other token that i can use to get the summary of the node? [node:body:summary] isn't working.

@s_leu: No, just use the patch.

Patch in #126 works for me too. Thanks guys!

126: drupal-1300920-126.patch queued for re-testing.

I looked at the fail and it was a HTTP time out? Basically it looks like the bit that failed has no relation to anything entered in the patch. I found the retest button and pressed it. Hope I have overstepped my bounds.

#134 is right - this was not about node summary token - so I will find another place for it ....

I don't understand what #133 is about, it isn't about the node summary token.

As per #130 this is RTBC

Committed to 7.x - thanks!

Woohoo! Thanks everyone!