Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
1. Create a new 'Article' node, type in a body text, but do not use the 'Edit summary' link - leave the alone.
Expected result:
The [node:summary] token outputs the trimmed version of the body field.
Actual result:
The [node:summary] token returns no output at all.
Comment | File | Size | Author |
---|---|---|---|
#126 | drupal-1300920-126.patch | 3.99 KB | pjcdawkins |
#114 | drupal-n1300920-114.patch | 3.98 KB | DamienMcKenna |
#104 | drupal-n1300920-104-d7.patch | 3.98 KB | idflood |
#104 | drupal-n1300920-104-d8.patch | 1.27 KB | idflood |
#100 | drupal-n1300920-100-d7.patch | 3.85 KB | idflood |
Comments
Comment #1
Dave ReidThis blocks us from providing a default description for nodes in Metatags.
Comment #2
salvisThis is a problem for Subscriptions, too.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe
Comment #4
andrenoronha CreditAttribution: andrenoronha commentedsubscribe
Comment #5
gumanist CreditAttribution: gumanist commentedIt 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.
Comment #7
gumanist CreditAttribution: gumanist commented#5: the-node-summary-token-does-not-output-anything-for-body-fields-without-a-manual-summary-1300920-5.patch queued for re-testing.
Comment #8
salvisThank you for your patch, gumanist!
Trailing white space.
Improve wording of comment, e.g. "Try getting trim length from teaser display, otherwise set default value."
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.
Comment #9
salvisComment #10
andypostelse should be on next line
600 should be removed. Use text_summary() that have a special variable for this
12 days to next Drupal core point release.
Comment #11
FiNeX CreditAttribution: FiNeX commentedWhen this issue will be fixed on D8, will a backport be available for D7 ? Thanks
Comment #12
salvis@andypost: Thank you for your comments.
Here's an updated patch.
@FiNeX: I sure hope so...
Comment #13
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooks good except:
// Try getting trim length from teaser display, otherwise set defa|80 columns here
Comment #14
salvis@Niklas Fiekas: Thank you for your comment.
Wrapped the comment and changed 'set' to 'use'.
Comment #15
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAwesome.
Comment #16
webchickWe should have a test for this w/ the use case in the OP, no?
Comment #17
mkadin CreditAttribution: mkadin commentedNot 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:
trimmed result is shown in the comment. If i do include a summary, it works properly.
Comment #18
FiNeX CreditAttribution: FiNeX commentedThe patch works fine here using D7.10 (tested with Meta tags module).
Comment #19
mkadin CreditAttribution: mkadin commentedAdded 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!
Comment #20
CarbonPig CreditAttribution: CarbonPig commentedsubscribe
Comment #21
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedWell done, thank you.
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.
Comment #22
mkadin CreditAttribution: mkadin commentedFixed that comment line. Here's the new patch.
Comment #23
mkadin CreditAttribution: mkadin commentedComment #24
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSorry. I didn't see this comment, that needs a trailing dot. (Now we're down to the *really* nitpicky stuff.)
Comment #25
mkadin CreditAttribution: mkadin commentedUpdated.
Comment #26
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLet's also give the tesbot just the test. This should fail.
Comment #28
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThank you, mkadin. I think #25 is good to go.
Comment #29
sunIt looks like a simple if/elseif/else control structure would be more readable and easier to understand instead of this break in between.
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.
Comment #30
salvisThank you, sun!
Implemented sun's suggestions and refactored redundant code.
Comment #31
andypostSuppose we should extend test to check for
<!--break-->
probably this could be cleaned in _text_sanitize() with check_markup()Comment #32
salvis@andypost: I don't quite understand what you mean and whether this really belongs into this issue.
Please provide a patch if it does.
Comment #33
heyyo CreditAttribution: heyyo commentedI 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.
Comment #34
andypostProbably duplicate about
<!--break-->
#1387560: HTML correction problem with text_summary in case of <!--break-->Comment #35
mkadin CreditAttribution: mkadin commentedandypost, 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?
Comment #36
salvis#1387560-5: HTML correction problem with text_summary in case of <!--break-->
Comment #37
romansta CreditAttribution: romansta commentedI applied the patch on Drupal 7.12 and it works.
Comment #38
salvis#30 has tests.
Comment #39
salvisHere's #30 again.
Comment #40
InTheLyonsDen CreditAttribution: InTheLyonsDen commented#39 Patch worked on Drupal 7.12 and Metatags. Thanks!
Always patching core. ;/
Comment #41
salvis#39: summary_token.1300920.30.patch queued for re-testing.
Comment #43
salvisRe-rolled.
The changes are only in the surrounding lines.
Comment #44
salvisComment #46
salvisHmm, more re-rolling needed (LANGUAGE_NOT_SPECIFIED, langcode).
Interdiff against 30/39.
Comment #47
L-four CreditAttribution: L-four commentedShouldn'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.
Comment #48
drupalsim91 CreditAttribution: drupalsim91 commented#46: summary_token.1300920.46.patch queued for re-testing.
Comment #49
salvis#46: summary_token.1300920.46.patch queued for re-testing.
Comment #50
nodecode CreditAttribution: nodecode commentedcan i ask what is holding up this patch?
Comment #51
salvis#46: summary_token.1300920.46.test_only.patch queued for re-testing.
Comment #52
salvis#46: summary_token.1300920.46.patch queued for re-testing.
Comment #53
salvisThe fact that no one is reviewing it.
EDIT: ... and that it needs to be re-rolled. Will do that tonight...
Comment #55
salvisHere's the re-roll of #46.
Nothing has changed except for the layout of the tests.
Comment #56
ParisLiakos CreditAttribution: ParisLiakos commentedThis makes more sense to me as well.. and +1 for the flexibility;)
Comment #57
Anonymous (not verified) CreditAttribution: Anonymous commentedI can live with this but I would rather the new token be [node:trimmed] and [node:trimmed:N] where N is a specified number of characters to trim to and [node:trimmed] uses the default site settings value.
Comment #58
salvisTrimming 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?
Comment #59
pminf#55: summary_token.1300920.55.patch queued for re-testing.
Comment #60
manos_ws CreditAttribution: manos_ws commentedWhen 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?
Comment #61
Anonymous (not verified) CreditAttribution: Anonymous commented@manos_ws: Do not hijack this issue for your support request. Please open a new issue with a Category of "support request".
Comment #62
manos_ws CreditAttribution: manos_ws commented@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.
Comment #63
DamienMcKennaRerolled.
Comment #64
Summit CreditAttribution: Summit commentedWould love to see this patch in D7 related to: http://drupal.org/node/1295524
greetings, Martijn
Comment #65
DamienMcKennaPer xjm on IRC:
Comment #66
DamienMcKennaUpdated patch that resolves the issues xjm mentioned (see comment #65).
Comment #68
salvis(If you put the test-only patch first, which needs to fail, then the issue won't drop to NW.)
Comment #69
jacobbednarz CreditAttribution: jacobbednarz commented#66: drupal-n1300920-66-tests.patch queued for re-testing.
Comment #71
ParisLiakos CreditAttribution: ParisLiakos commentedsee #68
Comment #72
andypostThis 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()
Comment #73
salvis#68 doesn't apply to "Unable to apply patch"
Comment #74
salvisHere's the needed re-roll. The only changes are in the surrounding lines.
I'm not sure about #72.
Comment #75
salvisComment #76
ParisLiakos CreditAttribution: ParisLiakos commentedsalvis, could you reroll #66 maybe?
your rerolled patch contains t() on test asserts
Comment #77
salvisOk, rerolled 66, sorry...
#72 still needs an answer.
Comment #78
DamienMcKenna@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.
Comment #79
andypostI'm ok with introducing validation in another issue, but this one should be postponed then
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.
Comment #80
DamienMcKennaI see your point. Lets remove the inline type checking and if you start another issue for the validation I'll help with it.
Comment #81
andypostFiled #1808942: Add element_validate_integer_positive() validation for text_field_formatter_settings_form() trim_length
Comment #82
DamienMcKennaAnyone 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
Comment #83
dman CreditAttribution: dman commentedAs 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.
Comment #84
DamienMcKennaLets try this - updated per #80.
Comment #85
lyricnz CreditAttribution: lyricnz commentedThanks 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.
Comment #86
andypost#84: drupal-n1300920-84-d8.patch queued for re-testing.
Comment #88
gregglesStraight re-roll of #84 to get it to apply.
Comment #89
greggles*ahem*
Comment #90
dman CreditAttribution: dman commentedThat smells nice. Test looks good!
Comment #91
gregglesIn #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.
Comment #92
DamienMcKennaThanks greggles!
Comment #93
DamienMcKennaJust tagging that this will need a backport to D7 after it's added to D8.
Comment #94
catchThis 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.
Comment #95
DamienMcKennaA 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.
Comment #96
lyricnz CreditAttribution: lyricnz commented@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
Comment #97
gregglesThanks, lyricnz. Seems like RTBC to me.
Comment #98
idflood CreditAttribution: idflood commentedI 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:
Applying the patch this notice is replaced by the following:
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.
Comment #99
gregglesNeeds work for the trim_length notice.
Comment #100
idflood CreditAttribution: idflood commentedSo 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:
Comment #101
gregglesFor 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.
Comment #102
gregglesI didn't purposefully remove those. Hmmm.
Comment #103
salvisThe D7 and D8 text_summary() defines the default trim length:
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.
Comment #104
idflood CreditAttribution: idflood commentedThanks @salvis. Here is an updated patch based on #55. There is also a d8 patch with just the default trim length added.
Comment #106
idflood CreditAttribution: idflood commentedSo 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)?
Comment #107
salvisD8 needs to go first anyway and it doesn't have a test yet.
Comment #108
salvis#104: drupal-n1300920-104-d8.patch queued for re-testing.
Comment #109
idflood CreditAttribution: idflood commentedLooks 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?
Comment #111
alexweber CreditAttribution: alexweber commentedIf 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?
Comment #112
greggles@alexweber - d8 was (partially) fixed in this issue and it still needs work before it can be backported. Maybe you can help with that?
Comment #113
alexweber CreditAttribution: alexweber commented@greggles, sure lemme check it out :)
Comment #114
DamienMcKennaThe patch from #104 rerolled, the only difference that the patch to node.tokens.inc was bumped 29 lines.
Comment #115
DamienMcKennaComment #116
leanderl CreditAttribution: leanderl commentedGreat patch. I'm not sure where this fits in, but if you are using media module to embed images this cleanup comes in handy
Comment #117
DamienMcKennaWe don't need the "backport to d7" tag anymore.
Comment #118
DamienMcKenna@leanderl: That will be handled automatically by the text_summary() function which will run the appropriate text format over the string.
Comment #119
leanderl CreditAttribution: leanderl commented@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.
Comment #120
DamienMcKenna@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.
Comment #121
GaëlGHere'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.
Comment #122
DamienMcKennaAs another temporary solution, I've turned the patch from #114 into a patch for Metatag that will be in the next beta release.
Comment #123
deanflory CreditAttribution: deanflory commentedI 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?
Comment #124
tstoecklerI 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.
Comment #125
lsolesen CreditAttribution: lsolesen commentedDoes not apply to Drupal 7.23.
Comment #126
pjcdawkins CreditAttribution: pjcdawkins commentedI'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. )Comment #127
pjcdawkins CreditAttribution: pjcdawkins commentedComment #128
s_leu CreditAttribution: s_leu commentedTested 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.
Comment #129
DamienMcKenna@s_leu: No, just use the patch.
Comment #130
jenlamptonPatch in #126 works for me too. Thanks guys!
Comment #132
stuart.crouch CreditAttribution: stuart.crouch commented126: 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.
Comment #133
tryitonce CreditAttribution: tryitonce commented#134 is right - this was not about node summary token - so I will find another place for it ....
Comment #134
pjcdawkins CreditAttribution: pjcdawkins commentedI don't understand what #133 is about, it isn't about the node summary token.
As per #130 this is RTBC
Comment #135
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!
Comment #137
DamienMcKennaWoohoo! Thanks everyone!
Comment #139
David_Rothstein CreditAttribution: David_Rothstein commented