Posted by gpk on December 18, 2007 at 12:35am
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | node.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
Previously we didn't have summaries and the teaser was (at least without any contrib. teaser modules) guaranteed to be no longer than the body.
However it is easy now with a basic installation to create a summary (stored in the "teaser" field in {node_revisions}) that is actually longer than the body (stored in "body" field in {node_revisions}). In this situation the "Read more" link is not displayed (see http://api.drupal.org/api/function/node_prepare/6).
While this won't be the case most of the time it could occur in a real-life situation if a user opts for a non-conventional usage of the summary/body fields. There is nothing to warn against this.
Comments
#1
Correcting title ...
Also in 2nd para it should say "create a summary ... that is ... longer".
You have to clear the "include summary in full view" checkbox for the problem to occur.
#2
From my point of view, the "Read more" link should always be shown if the teaser test doesn't exactly match the body text. This should be completely irrespective of length. We may compare lengths first, but in case length is equal, we have to compare char by char.
#3
>We may compare lengths first, but in case length is equal, we have to compare char by char.
This could improve performance I guess, but perhaps PHP does something like this already?
#4
Indeed, if the length is different, we need to show read more. If the body is shorter, then there was a longer summary (ie. a lead to an image, which is obviously shorted in number of chars in the HTML). If the body is longer, then you might have a teaser or summary, but the read more links are required. If they are of the same length, then they still might be different in content, and need to be checked for equality. Only in this last case, if they are equal should the read more links be removed.
#5
I think it's agreed the heuristic for showing the Read More link should be exactly match.
#6
I think a simple != test would do this just as well. Would also handle the conceivable (albeit unlikely) situation when one or other of teaser/body is empty string, and the other is a different type that amounts to the same thing when displayed but a different type (e.g. NULL). It might be possible for this to occur with module-provided teaser, for example.
#7
Easy.
#8
Patch works for me and looks good.
#9
Works for me too. Also checked we don't get a Read more link if body and teaser are empty. Unsurprisingly no problems here.
#10
Agree, != is better than !==.
#11
Oops.
#12
Good catch. Looks like a trivial patch. Committed #7. Still RTBC for 7.x.
#13
Hey, last commit prior to release of 6.0 :-D
#14
Haha, I might have noticed that too. :)
#15
#16
Erm, AFAICS the patch at #7 has not been committed to 7.x. (Yet.)
#17
Oh yeah, my bad.
#18
http://drupal.org/cvs?commit=100631
#19
Sorry, wrong branch
#20
In fact that solution is wrong: if the body is empty the read more link is incorrectly shown, as reported by #241742.
Here is a patch for 6.x that solves the problem.
#21
Sorry, but I don't think that it totally corrected yet. I've implemented it on my 6.1 site (replacing that single line at approximately line no. 1035), but still, if I "Split summary" at the very end of a post, the "Read more" link is shown on the front page.
I've tried clearing cache, reediting the post (join summary, save, split summary, save) and just plain waiting for a day or so, but nothing helps. Any ideas of what I'm doing wrong?
#22
The problem is caused by the addition newlines along with the
<!--break-->delimiter (the delimiter ends up in the body field in the database (but not the teaser) in the {node_revisions} table).If "Show summary in full view" is checked then the body consists of the teaser (which has \r\n added at the end after any whitespace is first trimmed) plus the delimiter plus another \r\n.
If "Show summary in full view" is not checked then the body consists of the delimiter plus trailing \r\n.
In http://api.drupal.org/api/function/node_build_content/6, the delimiter is stripped out, but not the trailing \r\n.
Hence if "Show summary in full view" is checked then the body consists of the teaser (with its trailing \r\n) plus another trailing \r\n.
If "Show summary in full view" is not checked then the body I think consists of \r\n.
So one way of fixing this is to put trim() round everything, which should not affect the rendered output.
Also in this patch I've fixed the comment that precedes the line in question, so that it makes more sense now.
#23
Comments slightly improved.
#24
This is a true problem, but all solutions so far (including mine) are horrible. We shouldn't have to trim and compare too potentially large strings (the body and the teaser, which can easily be several kilobytes each) to decide if we need to show the read more link.
#25
I agree, but how else do we do it?
I'm assuming that PHP is sensible about string comparison and checks the lengths before doing bytewise comparison.
#26
Seems to be that pretty much the only other option, is to store $readmore somewhere with the node revision. There is probably another way, but I don't know what it is. We area already comparing two long strings in 6.x, this patch would only add the trim, which is probably not much of an additional performance hit.
#27
If I may comment - I've only encountered the feature as a user so far and have just been searching the site for a solution to a related problem ("Read more" not going away anymore even with body = teaser, as discussed here, referencing this discussion).
So I came to this very thread with people wondering how to determine when "Read more" should be displayed in the first place, then one where the non-reversible nature of split/join is discussed (split adds line breaks that join does not remove, discussed here, leading to an inconsistent user experience as nicely described in this comment) and some discussion about how the "show summary in full view" checkbox should be handled.
In my opinion these are all just symptoms of the same underlying problem, which is that it's not yet been defined clearly enough what exactly the feature is actually supposed to achieve and how. I mean the idea is clear, but it's not been really fleshed out in detail. There's a good summary of the issues here, and another set of aspects being discussed here.
Lots of issues, most of them conceptual. I definitely don't want to offend anyone but my impression is that the split/join feature as such might not be ready for prime-time just yet - as it is it seems to be causing more grief than joy. Which may be acceptable in the 7.x discussion as that's a dev version right now, but as I understand what I've read the feature under discussion is exactly what's being used in the current 6.x (6.6 right now). I'm posting here because I couldn't figure out a better place for posting it - if there is one then please let me know.
In any case and getting back to the topic of this discussion, I believe that most of the difficulty in finding out whether to show "Read more" or not results from the fact that Drupal doesn't know whether text A ("intro") is meant to be part of text B ("body") or not. If yes, it would be a teaser or lead (as defined here), otherwise a summary.
My question is: why store it twice in the first place? If the text is split, then really split it, not duplicate part of it to start with - that's where the problem starts. If an intro is stored separately from the rest of the text then there will never be a question whether there should be a "Read more" or not - just look if there is a "rest of the text". Solved.
UI-wise, how about rather than having a checkbox "show summary in full view" or similar, use radio buttons for "teaser/lead (start of full text)" vs "summary (separate text)" or the like. Or if you want to be fancy (and really want to modify the text as it's split/joined), make it three: "teaser (split text mid-flow)", "lead (starting paragraph/s)", "summary (not displayed with full text)". Either way, solved as well.
As mentioned above, I'm much in favour of not changing the text at all when splitting it and let the writer decide if they want to have a line break or paragraph after the teaser/lead. It's easy enough to add or not add a line-break at the end of the intro, or rather having or not having one in place before hitting the "split" button, and it also makes sure that re-joining leads to exactly what was there before splitting. Not doing unexpected things under the hood removes a lot of ambiguity on the user-side, IMO, and gets rid of any second-guessing and tricking out the system. Anyway, that's probably just silly old me.
As for the rest, does that all make sense or am I missing something?
#28
Coming back to my original concern of the "Read more" link staying around even if/when the teaser is all there is and the body completely empty (as far as the user can tell, that is): there is a thread discussing that here which has been marked as a duplicate with reference to this discussion here.
From an outsider's perspective, the two issues don't look like near duplicates to me but I'll assume that Damian Tournoud has a reason for saying what he does in his comment.
However, what I'd really like to know is how to go about finding a solution to my problem - is that a known problem and is there a solution - if not, is this the correct thread to raise it or should I start a new one? Any advice appreciated.
TIA,
Ronald
#29
Can you try applying the patch at #23 to see if that fixes it? http://drupal.org/patch/apply
If you aren't comfortable with patching then you can make a backup of node.module and make the change by hand. Only one actual code line is changed, replace the one prefixed - with the one prefixed +
Have a good play around with it and see if you can break it.
#30
Thanks gdk.
I see what the code does but when I fixed the data in the database, I not only needed to remove additional line breaks, which "trim()" would get rid of, but also a "!--break--" tag, which would break things if you pardon the pun.
#31
With this patch the
<!--break-->tag should be handled correctly.. (elsewhere in node.module).#32
Marked #34135: Calculation of $node->readmore seems a bit flawed (sets to TRUE when it shouldn't be) as duplicate. However 5.x may need revisiting once it's fixed in 6.x and HEAD.
Also #167646: read more link in rss even if the complete content was made to be shown in the teaser via trailing <!--break--> looks related.
#33
Suggested way forward: go with the approach at #23 for now (will work for 6.x and 7.x) but add tests for 7.x.
Then come up with a better way of doing this in 7.x (tests will still be useful). I think we are a bit constrained in terms of approaches to solve this for 6.x.
#34
#35
Indeed it is, many thanks!
[Edit: oops, I hit reply to post #31, so what I meant to say is that the patch does fix the problem for me.]
#36
Hm, does not seem to work for me with 6.9 - the problem is back.
Any ideas?
#37
Did you reapply the patch at #23? The affected function node_prepare() (http://api.drupal.org/api/function/node_prepare/6) is unchanged since about Drupal 6.1 AFAICS.
#38
Yes, I did. Also checked the actual database record and removed an extra paragraph with a non-breaking space but that didn't help, either. The only difference between the teaser and the full text is the actual teaser break.
#39
OK well thinking out loud, since the node_prepare() function is unchanged since 6.1 the problem can't be that the patch at #23 has stopped working ... what is more of a possibility perhaps is that it never worked properly.
Can you try clearing all caches (on the site and browser)?
Then if it still doesn't work, post the exact contents of the database teaser and body fields here in between <code> tags, and I can try it on my 6.9 site to see what's going on.
#40
Might have been the cache - I haven't changed a thing but cannot find a page with the problem anymore, so probably a false alarm - sorry & thanks for your advice.
#41
OK thanks, good to know that the patch still works :-)
#42
Any chance that this patch could be rolled into the 6.x core?
#43
See #33. AFAIK this won't be fixed in 6.x until it has been addressed in 7.x, unless it was decided that a different approach would be adopted in 7.x (which would essentially decouple a 6.x from 7.x). This is the standard way that bugs like this are addressed, and helps ensure that fixes are handled consistently across versions, and also that fixes do actually work and don't break anything else.
#44
Well, why not incorporate the fix into 7.x and then from there into 6.x?
I'd say this is definitely a bug, so it should be fixed. If it is later decided to go with a different approach in 7.x then ok, it's decoupled, but the fix will at least be in 6.x.
If not, then only good that the fix is in. Or in other words: I don't understand why the approach you describe would stop this patch from going into core.
Am I missing something?
#45
No, you're not missing anything. What I perhaps didn't make clear is that the impasse is that someone needs to find the time to do the necessary test coding for the 7.x version of the patch.
#46
I understand, thanks.
#47
Cool :-)
#48
Hm, thinking about it again, having patched a number of upgraded Drupal versions in the meantime, I am not quite sure that I understand. There is an obvious bug, and a patch for it, which modifies exactly one line of code. I can hardly read PHP and don't know the Drupal coding and testing guidelines, but surely there is no test for every single line of code, rather than blocks of code with a defined functionality, or am I wrong there? Since that modified line of code only affects when/whether the "Read more..." link is generated, which is not working correctly without the patch, isn't there a test missing or failing each time without the patch?
I don't want to be annoying, just trying to understand what the real issue is with rolling this patch into the actual code - what would speak against doing so?
#49
Bump, this is still an issue for D7... any thoughts, action?
#50
@49: Fancy writing some tests to go with #23? Probably the existing patch needs revisiting as well as IIRC the way the teaser is handled in the UI has changed, which may affect the logic of#22 (which is still good for 6.x).
No one (including me) likes the approch of #23 but I think there may actually not be a better way without changing APIs and we've probably missed that opporunity!
@48: you are right on the ball, we are completely missing tests here, and although exceptions are made the norm is not to commit a bugfix unless tests are in place 1. to demonstrate that it has been fixed and 2. to prevent the bug from being un-fixed in the future.
#51
In D7 (up to rc3) readmore link is hardcoded to always on, so this is not the issue in D7.