Problem/Motivation
Since moving the Body field to the Field system in Drupal 7, the <!--break--> method of teaser splitting no longer works with Filtered HTML or Plain Text text formats. #18 offers a good analysis of the problem, which is essentially that in Drupal 6, teaser break processing jumped the queue and happened before the filter system. (TODO: Is that true? Would be good to investigate exactly how D6 and D7's code paths have diverged here.) In Drupal 7, the filtering happens later and filter_xss() is stripping out the comment.
Proposed resolution
One suggestion in #37 was to allow HTML comments to pass through filter_xss(). However, it was pointed out in #38 that we've had SAs related to HTML comments in the past. (TODO: Is that actually true? Citation needed. I searched security.drupal.org for "html comment" and came up empty handed.)
The solution that seems to have the most buy-in at the moment is temporarily switching the <!--break--> string to another string in order to escape filter_xss() unscathed. This would have the benefit of also working with other filters which might strip comments, such as http://drupal.org/project/iframe_filter.
Remaining tasks
Review and test the latest patch. However, validation of the approach is necessary, which means some legwork on:
- Investigate and document exactly why D6 deals with this situation fine and D7 doesn't.
- Investigate and document why #559584: filter_xss() and Line break filter break HTML comments didn't solve this problem.
- Get security team input on the options we have for fixing filter_xss() to deal with the teaser splitter (and other HTML comments) instead of being forced to hack around it.
Original report by thamas
The 'break' tag does not work with Filtered HTML, this may create problems at D6 to D7 upgrade.
Comment | File | Size | Author |
---|---|---|---|
#121 | regression_break_tag-881006-121.patch | 1.82 KB | cilefen |
#110 | core-break_tag_filtered_html-881006-110.patch | 1.89 KB | koppie |
#106 | core-break_tag_filtered_html-881006-106.patch | 1.89 KB | jenlampton |
#104 | core-break_tag_filtered_html-881006-103.patch | 782 bytes | jenlampton |
#93 | 881006-93.patch | 3.72 KB | xjm |
Comments
Comment #1
thamasComment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe break tag is typically not used anymore in the new "Text and summary" field, and we have an upgrade path. Do you have specific concerns about the migration path?
Comment #3
thamasSee http://drupal.org/node/546608 about the break tag in D7 and #881018: Body field "Show summary in full view" option
Comment #6
guusbosman CreditAttribution: guusbosman commentedI ran into this when testing D7 beta 2.
http://drupal.org/node/546608 says that the
tag is still used in D7. Also, the text in a node preview still mentions the break tag ("You can insert the delimiter "
" (without the quotes) to fine-tune where your post gets split.").
If we're still supporting the break tag, I think that it's confusing if the Filtered HTML filter doesn't work with it. In D6 the break tag works okay with Filtered HTML.
I often use the break tag at the end of a Body when I don't want the trimmed version to be shorter than the full version.
Comment #7
tobiasbComment #8
garpy CreditAttribution: garpy commentedI tested this without the patch and was able to recreate the bug. I applied the patch and it fixed the bug. Working on local environment in MAMP. I used Drupal 7 beta3 release.
Comment #9
garpy CreditAttribution: garpy commentedComment #10
tobiasbComment #11
sun.core CreditAttribution: sun.core commented1) "html" should be all upper-case.
2) Seems to exceed 80 chars.
3) Missing trailing period.
[something] is a commonly used macro tag syntax used by contributed modules. This temporary replacement keyword should be more unique.
Powered by Dreditor.
Comment #12
sun.core CreditAttribution: sun.core commentedComment #13
sun.core CreditAttribution: sun.core commentedSorry for spamming, but this also needs tests.
Comment #14
calebgilbert CreditAttribution: calebgilbert commentedI get the same issue with the
<!--break-->
tag not working as it did in previous versions of Drupal, even with the Full HTML filter.Comment #15
vito_swat CreditAttribution: vito_swat commentedI think this is major as this cause regression.
I checked this and my results:
1. In full HTML
<!--break-->
works.2. In filtered HTML don't work.
3. After adding
<!--break-->
to allowed tags it also don't work4. In Plain text this also don't work and
<!--break-->
is visible to the end user.Looks like something is really broken here.
Comment #16
federico CreditAttribution: federico commentedIt doesn't work for me in full HTML, if works only if I enable "Limit allowed HTML tags" option
But also,
Convert line breaks into HTML (i.e.<br> and <p>)
stops working after thetag, causing the loss of format in nodes with the break tag.
To reproduce:
1- Install drupal 7 dev, do not touch filter module.
1- Create a new node using full html format
2- Paste this
I couldn't apply the patch
Comment #17
sunThis is indeed a major problem. I'm entirely not sure why the break tag is stripped in D7, but it must have been caused by one of these issues.
Comment #18
scitoIn fact there are two issues:
1) Issue #16 results from #559584: filter_xss() and Line break filter break HTML comments and is a bug of the line/paragraph filter.
$open = ($chunk[1] != '/' || $chunk[1] != '!');
is always true in the function_filter_autop()
.Better is
$open = ($chunk[1] != '/' && $chunk[1] != '!');
Changes in the patch: I've modified the filter.module and improved the tests. Note: I commented out (marked with TODO) the iframe test since it seems not useful for the line/paragraph filter. Should we treat the
<iframe>
tag as the other tags such as<pre>
in the line break filter? Maybe this was the intention.$chunks = preg_split('@(<!--.*?-->|</?(?:pre|script|style|object|!--)[^>]*>)@i', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
Note: I've replaced in the tests the "found" message text with "expected" as this seems clearer.
2) The issue described in #1 to #15 seems to be a consequence of #372743: Body and teaser as fields.
In Drupal 6 the teaser break tag seems to be treated early in the processing, i.e. before the filters. (Note: Teasers are stored in DB field node_revisions.teaser.) In Drupal 7 the body is an ordinary field and the break tag for the teaser is treated later in the processing, i.e. after the filters. Filters can do what ever they want with the content, e.g. filtering out of comments such as the break tag. One approach is to "protect" the break tag through a temporary replacement as this was proposed in #7. But this is more a workaround than a "real solution". There is no guarantee that this replacement always works. Maybe someone has a better idea.
I've implemented this workaround as a first step in
_text_sanitize()
of the text.module and added some tests: a combination of display formats (summary_or_trimmed and trimmed) and core filters (Full HTML, Filter HTML, Plain text).We should probably split these two issues for the further resolution.
Comment #19
karschsp CreditAttribution: karschsp commented#18: breaktag_881006_18.diff queued for re-testing.
Comment #20
scitoI created for the problem described in #16 the new issue #1029108: Comments confuse line/paragraph filter and changed the component of this issue.
Comment #21
scitoI'm working on another approach where the body is split by the break tag before sanitizing. Teaser and body are filtered/processed separately for 'text_trimmed' or 'text_summary_or_trimmed'.
Comment #22
scitoThis new patch saves the
<!--break-->
for trimmed displays. A pseudo column 'value_for_trimming' is used by_text_sanitize()
in these trimming cases.This approach seems more proper and feasible.
It's not possible to process teaser and body separately for all 'normal' values (i.e. no trimming cases) since some filters might "correct" missing end tags for the teaser. This is unwanted in a full node view situation. Thus, the new pseudo column 'value_for_trimming' was introduced.
Currently, the 'plain_text' filter encodes and displays
<!--break-->
. Is this the wished behavior? Or should the break tag be filtered out?Comment #23
bfroehle CreditAttribution: bfroehle commentedThis from the earlier patch should either be included or split off into a different issue.
Do you mean
>
? Or just<!--break-->
?Comment #24
scitoI created a separate issue: #1029108: Comments confuse line/paragraph filter. I think it's easier attacking them independently.
I fixed the comment problem.
Comment #25
yched CreditAttribution: yched commentedsubscribe for now
Comment #26
jenlamptonThink this got moved to the wrong component ;)
Comment #27
yched CreditAttribution: yched commentedActually, no. The summary generation does belong to text field code now.
I most probably won't have time to look into this before drupalcon.
Comment #28
yched CreditAttribution: yched commentedComment #29
Frits1980 CreditAttribution: Frits1980 commentedsubscribing
Comment #30
vito_swat CreditAttribution: vito_swat commentedCurrently, the 'plain_text' filter encodes and displays
<!--break-->
. Is this the wished behavior? Or should the break tag be filtered out?For my common sense break tag definitely should not be visible no matter which built in or exotic filter you use! This is special, drupal only tag to act as teaser splitter. Nothing more.
Comment #31
daftu CreditAttribution: daftu commentedsubscribe
Comment #32
Agileware CreditAttribution: Agileware commentedIn #2 Damien said:
Do you mean there is a migration path from
<!--break-->
to using the summary part of the "Text and summary" field?Because I just upgraded a site and still have 12000 nodes that have the
<!--break-->
comment in the body and 110 of those 12000 don't have any text in the body_summary.Comment #34
scitoBumping and tagging as per standard policy.
Comment #35
scito#1087440: HTML Purifier break Drupal 7.x <!--break--> handling is an issue of the HTML Purifier module concerning this problem.
Comment #36
webernet CreditAttribution: webernet commentedI've been testing D6 to D7 upgrades and this is definitely a major issue as I don't want to convert all nodes to full HTML.
Comment #37
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, it seems that you need to add
!==
to the list of allowed tags for the comments to be kept. Not sure if we wouldn't rather want to allow comments to pass-thrufilter_xss()
by default.Comment #38
sunI'm pretty sure that we had several SAs pertaining to HTML comments in the past, so I don't think that making filter_xss() ignore comments is an option.
I think we need to go back to one of the earlier patches and go with the approach of replacing
<!--break-->
with a different delimiter.Comment #39
webernet CreditAttribution: webernet commentedRevised version of #7.
Uses time() to make a (hopefully) unique substitute.
Needs tests.
Comment #40
aspilicious CreditAttribution: aspilicious commentedtrailing whitespace
Powered by Dreditor.
Comment #41
webernet CreditAttribution: webernet commentedFixed the whitespace and added a basic test.
Comment #43
webernet CreditAttribution: webernet commentedWithout the commented out sections.
Comment #45
webernet CreditAttribution: webernet commented#43: break_tag-881006-3.patch queued for re-testing.
Comment #46
scitoUsing a replacement text for breaker tag is not a reliable method. You do not have control over what filters do with your replacement text. The patch of #43 uses the time. Imagine, there might be a (dump) filter which replaces all numbers with images or 'number words'.
Patch #24 avoids this. The breaker tag is not passed to filters. But it's a more complex solution.
I propose to include the tests of #24 in other patches. They are independent of the actual patch.
Comment #47
webernet CreditAttribution: webernet commentedThe substitute text is only used within the one filter in order to allow the break tag to bypass filter_xss().
The temporary replacement text isn't seen by any other filters as the original <--break--> text is restored within the same filter, before the next filter even sees it.
Comment #48
Clean CreditAttribution: Clean commentedI've been banging my head and wondering about this for days, I thought I messed up something. At the moment, seem like WYSIWG don't work with summary field also. I have to copy the source code from body field to the summary to actually get things right.
This is ugly and unacceptable, but at least it work on front end. Not sure about impact on SEO anyway.
This is a rally big issue because almost every content website need it ;(
John
Comment #49
xjmTagging issues not yet using summary template.
Comment #50
ibex CreditAttribution: ibex commentedWe can fix the built-in core filters (proposition #47) or build an architecture that fixes/avoids the problem for every filter including contributed filters (proposition #24).
Maybe we could split this issue for fixing the core filters in 7.x in a simple way and improve the architecture in 8.x.
Comment #51
webernet CreditAttribution: webernet commentedI hate to bump this, but this is a major regression and it's still at needs review two months and three releases since the latest patch was posted... the issue itself is over a year old.
Comment #52
varkenshand CreditAttribution: varkenshand commentedI agree this is serious and strange behaviour.
In my case I have noticed (D7.7 and D7.8) that the break tag works as intended on a clean install and in a new node.
In my test site however, the tag is ignored when there is an image present at the top (generated with imagecache, that is). Once the image is there, all breaks are ignored. This makes the taxonomy term view unusable.
Comment #53
leoklein CreditAttribution: leoklein commentedsubscribing
Comment #54
nico.knaepen CreditAttribution: nico.knaepen commentedsubscribing
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedthis looks good to go to me, it's a very simple patch, with a test.
Comment #56
chx CreditAttribution: chx commentedhow come #559584: filter_xss() and Line break filter break HTML comments didnt fix this?
Comment #57
chx CreditAttribution: chx commentedComment #58
drmonkeyninja CreditAttribution: drmonkeyninja commentedIncluding < !-- > (ignoring the spaces between the brackets) in your allowed HTML tags for the input format will ensure that the break is observed.
Comment #59
catch#43: break_tag-881006-3.patch queued for re-testing.
Comment #61
smitty CreditAttribution: smitty commentedsubscribe
Comment #62
jenlampton@smitty - no need to 'subscribe' anymore. Try the "follow" link at top right :)
Comment #63
smitty CreditAttribution: smitty commented@jenlampton - I tried it. But every time I came back to this issue there was "Follow" again instead of "Following" (in the meantime it seems to work). But anyway: even "Following" seems to do nothing. I couldn't find any place where this notification shows up and I didn't get a message about your new posting.
Is there any documentation about this "Follow" button?
Comment #64
jenlamptonIt's surely documented somewhere, I just don't know where. For me "follow" works exactly the same way as adding a comment. it adds issues to the list on my dashboard (which is where I check on them).
And since we're off topic and I'm feeling bad about that, patch re-rolled since core directory move. looks like the old patch still applies cleanly to D7.
Comment #65
varkenshand CreditAttribution: varkenshand commentedStill persistent ignoring of the break tag, in version 7.10. Both filtered and full html.
Comment #66
jenlampton@varkenshand even after applying the patch?
Comment #67
varkenshand CreditAttribution: varkenshand commentedI never applied a patch in my life, to be honest. In general I use the latest dev versions of modules and the most recent stable Drupal.
Update: I tried several programs to apply patches but frankly they are the worst software I have seen in ages (Windows 7 environment). What I would like to do is
- Download the module
- Apply patch to file
- Upload the module again to test.
This does not seem to be possible at present.
Comment #68
jenlampton@varkenshand We've provided a patch that is meant to fix the problem, but before the fix will be included in the next version of Drupal, we will need some help testing it.
You can get some information on applying patches for mac or for windows if you'd like to help us out. :) It's actually really straightforward if you are on Linux, or if you are already using git.
Comment #69
rangermeier CreditAttribution: rangermeier commentedthe patch from #64 works for me (on drupal 7.10). thanks!
Comment #70
steinmb CreditAttribution: steinmb commentedConfirm that #64 apply cleanly against D7.10, and even more important, it works :)
Comment #71
ralphb CreditAttribution: ralphb commentedI can confirm that #64 works against 7.12.
I also agree with #18, though, that the patch is merely a kludge that may break any time. Adding time() is cute but pointless, you could use any fixed random string for that purpose.
Comment #72
oriol_e9gI think that we need better tests, not only test _filter_html() function. We also need test a real node creation with the break tag.
Only tests patch should fail, complete patch should pass.
Comment #74
oriol_e9gThe tests fail as expected, the full patch pass.
Comment #75
jenlamptonWorks for me :)
I believe this is still an issue in D6 as well, or is that being tracked in this other issue?
#222926: HTML Corrector filter escapes HTML comments
Comment #76
catchNormally we'd use REQUEST_TIME rather than time().
Since we don't need the time at all, can we do something like hash the break tag, or use an actual random string or number. Either way it needs a comment as to why that's needed rather than a static string.
Comment #77
j0rd CreditAttribution: j0rd commentedI'm running into this problem with my trimmed_plaintext module. I need this issue resolved before I can support breaks.
I can't test the latest patch as it was made for D8 and not D7.
With that all said, as a module developer who's having to deal with this, I have a problem with the way Drupal is handling this. Not sure if the patches resolve my particular qualm, but never the less.
In hook_field_formatter_view($entity_type, $entity, $field, $instance, $langcode, $items, $display) I would much prefer that $item['summary'] always be populated with the appropriate summary, be this caused by a BREAK tag or by the end user splitting the field into two parts. It doesn't really make sense, when a BREAK is used, not to have this field populated. I assume this is done, to create simplicity and not having to synchronize a full body with BREAK and the summary it would create. It would on the other hand stop bugs from happening in field_formatter modules.
Comment #78
oriol_e9gTrying again, I have used md5 instead of sha1 or similar because the first is faster.
Only failing tests in #72
Comment #79
oriol_e9g@j0rd This is a Drupal 7 version for testing purpose.
Comment #80
oriol_e9gComment fix.
Comment #81
xjmThe comments here are a little confusing. The name
$teaser_breaker
is also a little odd.This class needs a docblock.
"Delimitator" is not a word. It should be "delimiter." (This change should also get the summary under 80 chars.)
I'll reroll to clean these things up.
Comment #82
xjmOh, also, "summary" is misspelled here.
Comment #83
xjmAttached:
<!--break-->
rather than the whole text. (We just need a placeholder that is not found in the text; no need to have the overhead of md5()ing the whole$text
.)t()
from assertion message text. (Reference: #500866: [META] remove t() from assert message).Comment #85
xjmWhoops. Deleted one line too many. Let's try that again...
Comment #88
xjm*grumble*
Comment #89
artfulrobot CreditAttribution: artfulrobot commentedI've used this patch except that I've swapped the unnecessary md5 calculation for a literal string.
Comment #91
xjmThat's a good suggestion. However, the patch needs to be rolled against 8.x. Also, the comment should be on the preceding line so that it doesn't go over 80 characters.
Comment #92
artfulrobot CreditAttribution: artfulrobot commentedPerhaps I should have not submitted a patch and just described it. So here goes:
The
md5("<!--break-->")
is unnecessary because the md5 of a constant is always going to be constant.Drupal is heavy enough without adding further inefficiencies IMHO.
Perhaps xjm could incorporate into patch @ #88 ?
Comment #93
xjm:P
Comment #93.0
webchickAttempting to summarize the issue to date. This probably still needs work.
Comment #94
webchickThe approach in this patch makes my spidey sense tingle, so I read through the issue looking for answers to some questions. I didn't find them, but while I was at it I updated the issue summary. ;)
The following is still unclear to me:
- Why exactly is D6 fine with this situation, and D7 is so not? #18 comes closest to answering this question, but it's still a little vague on specifics. I'd love to know what's the actual flow/missing function/whatever in D6 vs. D7 that causes break tags to get screwed up?
- chx's question in #56 seems to still be unanswered. Why didn't #559584: filter_xss() and Line break filter break HTML comments fix this indeed? Related to the above. Possibly answered by the above.
- Damien's proposal in #37 is how I'd expect to fix this. Namely, if our problem is with filter_xss(), let's fix it there, and just allow comments. This was discounted in #38 saying that "I'm pretty sure that we had several SAs pertaining to HTML comments in the past, so I don't think that making filter_xss() ignore comments is an option." However, I search security.drupal.org for "html comment" and found no such issues (doesn't mean they don't exist, just means I'd love to have this option 'officially' ruled out before we do this weird "temporary string switching" behaviour).
If anyone could lend some clarity to these points, I'd feel a lot more comfortable committing the approach in #93.
Comment #95
borazslo CreditAttribution: borazslo commented#88 with #93 Rocks!! Yeah! And thx!
Comment #96
xjmComment #97
litmusdesigns CreditAttribution: litmusdesigns commented#58 is correct.
I can confirm that adding
<!-->
to allowed formats fixes the issue with DrupalBreak and Filtered HTML without applying a patchComment #98
catchThat seems like a better fix to me than the md5() trickery. Let's add that as a default.
Comment #99
xjm@catch -- I think HTML comments were rejected as a solution in #38 (see also webchick's review in #94). So maybe we need a security review on whether adding them is safe?
I'm also curious as to how this regression came into being. I spent probably half an hour looking into it a couple weeks ago and could not find a reason for it when comparing the D6 and D7 code for teaser generation.
Comment #100
smitty CreditAttribution: smitty commented#58 / #97 works fine: After adding
<!-->
to allowed formats the break in the teaser is correct.BUT: At the same time you get a line-break in the full text at the position where the
<!-break->
is entered.Comment #101
herzbube CreditAttribution: herzbube commentedAdding <!--break--> to the Filtered HTML format (as suggested by #58) does not work for me. I am on D7.14, having upgraded from D6 about a week ago.
I have tried all sorts of things but couldn't get this to work:
Comment #102
smitty CreditAttribution: smitty commented@herzbube: Just leave out the Word "break". Only enter
<!-->
.Comment #104
jenlamptonThe attached patch allows the HTML comment tag for the Filtered HTML text format, as suggested in #98
This would solve all problems for sites upgrading from D6 to D7 (to D8?), and for sites that have not changed the allowed tags for the filtered HTML text format. I think it's not as ideal as #93 for a few reasons. For starters, I'm not sure this will work for sites that have changed the allowed tags on the Filtered HTML text format, or as in my case, created another text format with the HTML filter and a different set of tags than the default.
Additionally, I think we may need to document what
<!-->
means somewhere, since that was not needed in previous versions of Drupal.Comment #105
xjmTo consider the approach in #104, we need to make very sure any tags within the
<!-->
still get stripped. Let's try rolling #104 together with the earlier tests form #93, and maybe also explicitly add a test that<!-- <script></script>-->
and variants gets stripped properly?Comment #106
jenlamptonI don't know how to incorporate the tests from #93 into this new style Symfony testy thingies, but I did some testing of my own, that failed :/
I created a node with the content:
And the resulting HTML on the page was:
We need to make sure the script tags get stripped somehow if we go with this approach, and it would also be nice if we didn't get extra unwanted P tags in the original content.
Oh, but I did I also rework the settings form to include a checkbox for allowing HTML comments (checked by default) instead of having to allow a funny-looking tag to the list of allowed tags, I like this UI better.
I also like this general approach better (allowing all safe HTML comments instead of only the break tag) because it also solves the problem for contrib modules like paging and ad, which are currently broken since they can't allow their stuff to get injected into node bodies (as HTML comments) until this gets fixed. Allowing only the break tag via md5 hash won't help them at all.
Who has strong regex fu? :)
Comment #107
drummWe ran into this on Drupal.org, #1830022: Getting involved page content. Looks like we can work around it in D7 by appending
' <!-->'
to filtered HTML's allowed tags in a custom update hook.Comment #108
ergophobe CreditAttribution: ergophobe commentedI wouldn't say I have strong regex fu and I'm not totally sure I understand what you're trying to achieve. But assuming it's just the use case above, I can make a regex for that.
Perhaps I've oversimplified, but I'm just stripping out attempts to inject script tags in and stripping them out without prejudice.
Essentially this is just a "lazy dot" match (.*?) that also catches newline (\n) and return (\r) in case it's multiline input and by taking them in any order it's agnostic with respect to Mac, Win, Lin.
I've also created an atomic group (the ?> operator) that essentially says to match the whole thing as a group. I can't think of a practical case where this will change the match, but what it does is offer some efficiency if the lazy dot fails to match (i.e. there is no closing script tag). Essentially, what would happen otherwise is, failing to match, the regex has to backtrack, but with the atomic group it says "if you don't find this full expression including the script tag at the end, then it's malformed and just keep going."
Limitations:
<script>blah blah<script>bad stuff</script> halb halb</script>
it's going to outputhalb halb </script>
. Not too tragic, it just leads to messy output, but that's an edge case.It does expect some other possible trickery in event someone is trying to fake out the regex. I don't know just how garbled code can get and a browser will attempt to parse it, but I added the [^>]* matches in the script tag, even the end tag, so that the following are at least made safe, even though the output will not always be well-formed:
<script attr="value">bad stuff</script garbageattribute="garbage">
- removed entirely<script title="tricky ></script> stuff">bad stuff</script>
- this will end up with malformed HTML, but it should be safe. It will match on the first</script>
which is really part of the value of the title attribute and it will end up outputtingstuff">bad stuff</script>
. Not great, but I think that should be safe.There might be other possibilities, but I think that should get rid of the case you posted originally and many, many more, but should be pretty safe in terms of leaving allowable code alone.
Wthin the limits of formatting allowed on d.o, here's the gibberish input I tested against
Comment #109
jenlamptonFor the record, we're still telling people to use this broken feature on the node preview page. the text says :
If we can't actually get a fix in for this in D7, can we at least remove the text that tells all our content creators to try this broken feature? :)
I'm changing status back to 'needs review' because I'd like people to look at the patch in 106 again. I'm not sure this has any actual security implications, since whatever gets inserted inside a HTML comment will be - get ready for this - commented out.
Comment #110
koppie CreditAttribution: koppie commentedI'm re-uploading @jenlampton's patch from 109 since the Drupal bot never tested it for some reason. Green stripe, baby! Come on! :-D
I'm removing "needs backport to D6" because (a) I believe it works in D6, and (b) Jen agrees that it doesn't need to be backported, and she's the one who added the tag in the first place (13 months ago).
Comment #112
Atratus CreditAttribution: Atratus commentedFor the sake of anyone needing to deal with this in a way that doesn't mess with the core, here is how I have dealt with this in a custom module. The following code will add two filters for the text formats. Use the "mangle" filter as the first filter in the text format and the "unmangle" as the last filter. It also deals with extraneous p tags wrapping the break inserted by the wysiwyg editor that I am using.
Comment #112.0
Atratus CreditAttribution: Atratus commentedFormatting issues.
Comment #113
calebgilbert CreditAttribution: calebgilbert commentedAfter playing with things some more I wonder if this whole issue isn't actually more simple than it seems...
...the break tag seems to work normally for me after simply going to a content type's "manage display" settings and changing the "body" field format from "Default" to "Summary or trimmed". This begs the question what "Default" even means if not "Summary or trimmed" (see screenshot), but none-the-less the break tag works as expected for me if I change this setting.UPDATE:
I was on the right track, but changing the body field on the default display settings page is not quite right. Fwiw, and based on my tinkering I believe the following steps demonstrate how the core display settings could/should have shipped in order to have the break tag work properly out of the box:
1. On the default display settings leave the body field to "Default". We need to create a teaser display setting instead. To do this select the "Teaser" option underneath the "Use custom display settings for the following view modes" collapsible fieldset and click save. (screenshot)
2. Choose the teaser display settings you just created and then change the body field on that page to "Summary or trimmed". The break tag behavior should now work for you as expected. (screenshot)
Comment #114
TWD CreditAttribution: TWD commentedsubscribing.
May try #112 by Atratus and report.
Comment #115
TWD CreditAttribution: TWD commentedThe custom module in #112 Atratus works like a charm!
But it would help other noobs like me I think to explain explicitly the thought process behind it and the necessary configuration setting.
The problem is that core strips out the break tag from filtered HTML.
So Atratus "tricks" the filter by disguising the tag using the .md5 PHP hash function. That's what the "hide_break" callback function is all about.
Then once it has passed the filters, he undisguises the tag using the "fix_break" callback function so that it gets rendered into HTML.
The important point to note is that this custom module adds two "filters" into the Configuration/Content Authoring/Text Formats screen.
1. You need to enable both:
"Prevent <!--break--> from being stripped by mangling "
and
"Restore mangled <!--break--> tags "
2. In the Filter Processing Order on the same page you need to drag the "Prevent Stripping" process so that it is just above the Limit Allowed HTML tags and drag "Restore Mangled" so it is just below the Limit Allowed HTML process.
Comment #116
jhedstromAny mention of the
<!--break-->
tag was removed in #1510544: Allow to preview content in an actual live environment. Moving this back to 7.x.Comment #117
w00 CreditAttribution: w00 commentedI was bitten by this bug today.
I'd noticed it working occasionally, but mostly not, in previous Drupal encounters.
So I set up a new site yesterday and was pleasantly surprised that my Summary was broken as expected.
Today I added a page or tweaked a page, and saw it *not* working.
Spent far too much time diagnosing the issue. Once done, I used the new-found knowledge to find this page. And I see it's been an issue for... 5 years?!?
As far as I can tell, either I need to:
Really, none of these options are palatable, not even reasonable.
I'll admit I haven't read all 116 comments that precede mine, but as of Drupal 7.34 (applying 7.35 this evening or tomorrow), this issue persists and there doesn't seem to be any good reason.
After the SQL-cleansing bug a while back, now this? I'm thinking something's really wrong here. I understand that bugs can happen (i.e. SQL or this one), but that it hasn't been addressed in the main branch is nothing short of ridiculous, frankly.
Comment #118
herzbube CreditAttribution: herzbube commented@w00 The workaround that I have been living with for 3 years now (and which has been mentioned in the older comments a couple of times) is to insert <!--> into the list of allowed HTML tags. It's not too bad, which is why I suspect that the issue hasn't been given more attention. Just make sure that you add <!--> and not <!--break-->.
Comment #119
w00 CreditAttribution: w00 commentedherzbube, Thank you for your reply.
That does, indeed, work.
Not pretty, but acceptable.
Thanks again.
Oh, and for anyone that finds this thread and isn't sure how to accomplish this:
Comment #120
Michael_Lessard_micles.biz CreditAttribution: Michael_Lessard_micles.biz commentedThe quickfix in #118 above works in Drupal 7.38
as long as your Content Types have the proper Manage Display settings, namely Teasers (custom) set to Summary or Trimmed.
I am migrating from D6 to D7 at the moment.
Nb: I fully support the indignation that this is not fixed in core by now. Hope it is fixed in D8, as D6 content is planned to be importable by D8. This bug causes a regression/migration hurdle (as if it wasn't a painful process already), where all your nodes might appear fully where teasers are supposed to be shown.
Comment #121
cilefen CreditAttribution: cilefen commentedAccording to #116, all mention of this inline tag was removed in Drupal 8.
This is @jenlampton's patch, backported to Drupal 7 and with a hook_update_N() to enable the setting.
Comment #123
guusbosman CreditAttribution: guusbosman as a volunteer commentedHaha, it took me 5 years from trying an early beta (comment #6) to finally making the switch to D7. Good too see that there's a work-around (#118 worked fine for me).
Comment #124
massiws CreditAttribution: massiws commented#118 worked fine for me too.