Read More link is always visible on teaser.

I have no settings to trim the length of a post. I was unable to find them. Thought I was fortunate to find this, but no such setting exists for my site for pages or stories.

I was looking for the setting because I have the same problem reported here: "No trim shows Read More on front page"

Why a "Read More" when it is all displayed?

According to the code we have in node.module, we don't have any way to know when the teaser is different than the full view, so we can't fix that.

  // Always display a read more link on teasers because we have no way
  // to know when a teaser view is different than a full view.
  $links = array();
  if ($view_mode == 'teaser') {
    $links['node-readmore'] = array(
      'title' => t('Read more'),
      'href' => 'node/' . $node->nid,
      'attributes' => array('rel' => 'tag', 'title' => strip_tags($node->title))
    );
  }

I don't know who agreed to change the behavior of the read more link but for me it is serious degradation as Drupal 6 and before the read more link is behaving more or less like user would expect. Now when we have fields in core it is more difficult to check but still not impossible.

I propose something like that:

1. check if every field in full view is shown in teaser. If no, show read more link
2. check if every shown text field has the same text in teaser and in full view. If no, show read more link

This will provide more intuitive and expected results. The read more link will be shown only if there is really something more to show (more fields, or more text in a field) ie. no read more link if the only difference between teaser and full view of article is the image style used and BTW for node types like old image, users tend to click on images anyway (there's no problem to have image linked to content).

According to point 2 there should be the way to allow other (contrib) fields to be checked for consistency or even make user decide about checking but I think that this part can wait for D8.

~

Can anybody verify if this is behavior exists also in current dev? Then could change the version of this thread and it would receive more attention.

Confirming on the latest dev. I agreed this is a regression in behaviour from D6. 'read more' should not show if there is no more to read!

Used to work like this on D6:

  // First we'll overwrite the existing node teaser and body with
  // the filtered copies! Then, we'll stick those into the content
  // array and set the read more flag if appropriate.
  $node->readmore = $node->teaser != $node->body;

Then hook_link() would check for that flag.

~

joachim:
We cannot rely on the code from D6 as fields in core feature eliminated the mandatory body field. Case is more complicated and I guess that's why they put the read more link always on but this is not it supposed to work in my opinion.
I will try to provide some preliminary patch in 2-3 days.

I guess then we can expand the logic to this:

- is there a 'body' field on this node?
  - yes:
    - does its summary differ from its full text?
      - yes: show 'read more'
      - no: show nothing
  - no: show 'read more'.

Contrib modules can step in and provide more complex logic for cases where there is no body field :)

I'd like to implement logic described in #4

I've been wondering about this. It's weird, would like to see it fixed. #11 reads like it's more upgrade-from-D6-friendly?

I assume a proper fix must check all fields as proposed in #4.

I think it's worth having this problem fixed since readers could be (systematically) mislead in certain types of websites, e.g. posting of short news.

@vito_swat: Already a progress with your patch?

Subscribe. Read more links have always been a thorn in my side, and it seems like they are still nowhere near as intuitive or helpful as they should be. The logic to check for whether all the fields are the same, etc. could be an extra performance-sapping procedure, unless the read_more flag can be cached, like it is in D6.

@ibex: patch is progressing slowly. It's more complicated than I expected, especially that I'm not that familiar with fields as I'd like to be.

Here's first try. In general it's the implementation of algorithm given in #4

What the patch does:
1. Bring back node->readmore property in node_build_content(). Sets to false by default.
2. Checks if any field hidden in teaser mode is shown in full mode in field_default_view(). If so than it sets readmore property to true.
3. In text_field_formatter_view() it checks if any text field is shown as trimmed or as summary or trimmed then checks if trimmed version is the same as full version. If not it sets the readmore property to true. <- this mimics the D6 behavior for body field
4. Allow any field to set the readmore property by hook_field_formatter_view()
5. If any test will set the readmore property to true, Read more link will be shown.

Patch is lacking any test code as I'm not good at it. Please someone step in and help with it.

corrected line endings for testbot.

subscribe

Fields hook_field_formatter_view() implementations are responsible for correct readmore flagging with this solution. Each field must decide if two display types differ. If nothing is implemented, no "Read more" link will be shown. E.g. for tags field there will no "Read more" link be shown if the field is formatted as "Plain text" in the teaser and as "Link" in the full view. I think this is ok.

The patch looks good to me. But is this an API enhancement for fields? Do we need to document it somewhere?

I've made a minor white space improvement.

I've written several tests:

1. Teaser and body are equal; without tag.
2. Teaser and body are equal; with a tag.
3. A field (tags) is hidden, but is empty.
4. A field (tags) having a value is hidden.
5. Teaser is trimmed.
6. Summary is set.

Given every field gets checked, I think some benchmarking might be needed.

I second the benchmarking - this could be at least a minor performance drag. The 'read more' status of each node should be cached, and probably determined at the time of the node submission/update, as it was in D6.

Where was it stored in D6?

Joachim - I presumed it was stored in the node or node_revisions table, but it looks like I'm wrong. Hrm... /me goes to check the logic of the $readmore... and realizes that the $readmore status is set in node_prepare(). Ah well.

@joachim & geerlingguy
I'm not sure benchmarking will provide us any useful and meaningfull data here as the changes in the code are not that significant as you expect.

1. First of all not every field is checked but only fields hidden on the teaser (which is not so common in default profile there's no such case)
Performance hit: One field_get_display() and 2 ifs (if the build mode is teaser and field is hidden)
One if (eseif to be specific) in case the field is hidden but no other conditions apply.
No performance hit if field is shown (almost always the case)

2. The core will do another check for shown text field (for core node types only 1 additional check will be done).
Performance hit: one if.

Moreover teaser views are paged so I don't expect more than 50 node teases per page in very majority of cases. I don't think the performance hit if any (couple of ifs and no queries) will be noticeable to user.

@vito_swat - if that's the case, seems okay. But if we had to introduce any extra data/queries, this would be a different story.

Subscribe.

Would be VERY happy if you can just disable the 'read more' by removing the teaser in the fields display. If you turn off teaser mode, the whole node displays and the 'read more' should go away too.

This is a very simple, basic thing that a lot of new users are going to want to turn off on a new site and it should be just a checkbox.

Guys, is there a simple way to fix this? Having the read more link on your home page when there is nothing more to read looks really bad. I am not a programmer, please can you provide a simple way to stop this happening even if it means totally turning off read more?

I don't mean to sound rude, but I really do not understand how the devlopers do not see how bad this makes drupal appear. If I hadn't already invested a lot of time and effort into learning drupal which includes the purchase of 2 books I would find another CMS. How many 'new users' is drupal losing to this? I am currrently using CSS display:none to hide it :s

subscribe

subscribe

Which class did you apply the display:none to. I've tried a couple based on what I see in firebug and nothing seems to work. Thanks for any help you can give me!

This is starting to seem like an engineering vs common sense issue, like "if you understood how razzaframs rely on dinglehoppers then you'd see there's a good reason why your car's fuel warning light comes on even when the tank is full."

This is annoying enough that we wrote a module that gets around this *properly*, its sitting in the queue waiting review though.

@giraffian: what's the link for reviewing it?

This is Critical for me since it stops the upgrade of my sites from D6 to D7.
#21 works from me, but I think this should be a "Standard" functionality, not a module.

@giraffian and @joachim: What's missing here is a review. Code and tests are available. According to me the fix of vito_swat is well written. However I cannot review my own tests. Maybe you could give it a try.

I would be happy to get this fixed in Drupal core, hopefully 7.1.

subscribe

subscribe

subscribe

I've just noticed this problem as well... I don't think it's actually a problem though, ya see I realised what it is doing when you promote something to the front page is simply trying to fit in what might be a lot of different posts which explains why it has to do teasers for that, even if for the content type you turn teaser off, I think you're really only setting it to display with an unlimited teaser length. It still displays as a teaser "style".

I think the key to static front pages, is not to promote anything to the front page, but to create your node as a basic page as you intended, however once you know the node address (ie node/1) go into configuration -> system -> default front page, and change that to be the node/1.

Once you do this, it will all display correctly as a single static page. This is obviously not a problem on any other page in the site because you can make the menu items link directly to nodes anyway so that's how a static site would be created.

@ 43, For someone starting with a new Site its not a big problem, but if you plan to migrate from D6 and have thousands of pages relaying on the correct handling of "read more" - as it was in D6 -, you have here a very critical issue.

The point is that if the entire content of the node is displayed in the teaser, it shouldn't have a read more link because there's nothing more to read. After clicking 2, 3, 4 links and experiencing that, readers will cease and will miss content where there -is- something more to read.

Please test and review patch in #21. This is the only way it will get proper attention from the core commiters. Any more "subscribe" posts or complaining about the way D7 is working now will not help here.

@scito: thanks for the tests.

shouldn't the tests patch pass?

Hi.
I am a newbie to drupal.
Would really like to test your patch, but...
could someone please tell me how and where to implement the patch.

Just need a recipe. :-)

thx

david71, have you checked http://drupal.org/patch/apply?

See http://drupal.org/patch/apply

thank you for the link, will check it. Sorry guys, next time I´ll use the search field, promised.

Ok, just tried to apply a patch, but failed. Shame on me...

..then I found this video from Philbert with a cool workaround. Worked fine for me.

http://siteperspectives.com/node/3

I agree that this behavior isn't expected. It's very misleading, as a matter of fact. If there's a post on the front page, which is the full post, then there's a read more link and people click it thinking that there's something else. It leaves the impression that the site owner is just trying to increase page views. I have received quiet a few complaints from clients on this.

Until it can get patched, I have come up with a work around (albeit very ugly) using preprocess_node:

    if ($variables['content']['body'][0]['#markup'] != $variables['node']->body['und'][0]['safe_value']){
       unset($variables['content']['links']['node']['#links']['node-readmore']);
    }

It's probably (most likely) not the best way, but it does give the desired effect.

shouldn't the tests patch pass?

No. If these tests fail without the patched code and succeed with the patch, then this shows that the "right thing" is tested.

@intoxination: Is there a reason for not using the patch #21? If so, this might help us improve the patch. Thanks anyway for sharing your version.

How about all of you use display suite & stop crying...lol
Display suite allows you to control all of your displays.

If anyone wants to review the 'read more control' module its now got a sandbox project:

http://drupal.org/sandbox/Monochrome/1088582
And the original application: http://drupal.org/node/1056940

Needs reviewing by the community to become a full project.

@54, 55: this needs to be fixed in core, not polished over elsewhere.

@joachim - couldn't agree more, but check the age of this thread. Painting over it with a patch or a module at least gets people able to convert to d7 if they want to.

Could all those people perhaps try the patch?

I’ve tried it. Is this the proper place to discuss it? There are no issues for sandboxed projects, right?

I encountered the following issues:

installing: there is no core version in the .info-file so it can not be enabled on the modules page (saying it is not a drupal 7.x module). I put it in myself (core = 7.x) and than it was ok.

running: setting the Read More Settings to “When Required” only worked if the fields I have (link an taxonomy) had the “Read More Ignores Field”-checkbox ticked. But than the read more-link was not displayed on all other content-types as well. Those content types did not have the link and taxonomy field.

When I have some more time, I will try to fiddle around with it a bit more. The design of the module seems well thought out. Although a variable to use in .tpl-files could be handy as well (good old $readmore).

update: Forget what I said about other content types.

> There are no issues for sandboxed projects, right?

Yes there are -- follow the link and the issue queue is on the RHS.

Can we keep discussion here on topic to the matter of fixing core please?

How could I’ve overlooked that? You’re right, I’m sorry, I will post it over there.

Okay here's a reroll with fixes for code style and a bit more detail in the comments.

> The patch looks good to me. But is this an API enhancement for fields? Do we need to document it somewhere?

Quite possibly.

This also needs review from FieldAPI people. It seems to me that field_default_view() is quite high up in the field system to be putting something specific to nodes and teaser view, but I've no idea if there's a suitable place to put it so we catch all fields on nodes -- I suspect there is not.

I'm reassigning to that component so it gets attention. If anyone following this thread is at DC Chicago, I suggest you try to buttonhole people who have worked on FieldAPI!

@scito - I plan on testing the patch on my personal site in the near future. I don't like doing patches on client sites, since that delays how long it takes me to update their sites to a newer version. I also wanted to put a simple fix out there, since this ticket is one of the results when you search Google about the problem and 90+% of Drupal users out there aren't comfortable or lack the knowledge of applying patches.

Something I do see that could cause problems (including in my fix) is that text_summary does run through _filter_htmlcorrector() (if set in the filter) after creating the summary. In theory, it shouldn't cause any problems, but that's one of those things that really needs tested out.

Having said that, maybe a better fix would be a change to text_summary's return value. It's very simple in that function to know if text is being trimmed or not, so returning an array might be better, ie:

return array('output' => $output, 'trimmed' => false);

In core, text_summary is only used in text.module, aggregator and node, so changing the return value wouldn't be too much work to fix. OTOH, this would be constitute a pretty big API change that could affect modules in contrib.

Another thing that could be an issue (and where I agree with @jaochim that this needs to be reviewed by the FieildAPI people and maybe even the entity people) is that we are altering $entity in a formatter_view (albeit just adding a new item). A workaround I see for that would be adding a new attribute in the $element[$delta] array, ie:

$element[$delta] = array('#markup' => $output, 'trimmed' => $trimmed); //$trimmed would come from the returned array of text_summary()

This would require more logic node_build_content, searching through each text field and looking for trimmed being set to true before deciding if a read more link should be attached.

Again - I defer to @jaochim's suggestion though of having the FieldAPI people take a look before going this route. Since what I'm suggesting actually involves an API change in text_summary, which I wouldn't doubt is being used in contrib, that might be enough to put this off to D8.

@intoxination

You seem only to focus on trimming text fields which is not the only case to show readmore link. Consider hidden fields or some custom field that need to alter this property. Proposed patch tries to do as much as possible without breaking existing API so it can be transparent to every module created.

@vito_swat - actually my method takes into consideration hidden fields more than the current patch. One problem is that this patch doesn't take into consideration node's with multiple body fields, in which the trimmed field might have restricted view access through hook_field_access. Hook_field_formatter_view is called regardless of the view op on hook_field_access, so that field that the current user doesn't have access to view still gets parsed and can set $entity->readmore. An example I can think of:

- You have an employee node type. It contains 2 body fields; general description and manager description.
- Manager description has limited view access to only those in the manager role.
- A manager puts a break or summary in the manager field for an employee, but the general description field doesn't have one.
- A regular employee views that node in teaser/trimmed mode. The manager description field still gets parsed through text_field_formatter_view and since it is trimmed, it will set $entity->readmore, hence giving that employee a read more, even though the field that is actually being truncated they can't even view.

(I just tested this by having field_access() return false on all views and it does as I described above. Not really sure why hook_formatter_view is called when a user doesn't have access to view that field, but that is the case.)

Again - this is a problem with setting readmore in $entity, especially when it happens so early in the field system. That's why it really needs reviewed by the FieldAPI people and even the entity people.

IMHO something should be done to make this take all that into consideration. The best way I can think of is for each field to be looped through in node_module. If #field_read_more (a new attribute set from my comment in #63) and #access == true, then the read more link is set.

The other plus to my idea is that other/non-core-text fields could utilize text_summary() and easily find out if the field has been truncated or not.

I rather like intoxination's suggestion of adding a key to the field contents.

However, it would make this into a system where multiple parts have to work together:

A. hook_field_formatter_view() implementations have an extra key they can return, #readmore (this better conveys the meaning that 'trimmed', I think. This should be set to TRUE if what the hook is returning is different in some way to the full version of the field's data -- eg, shorter text, different image style, fewer items. The hook should collate this itself over all its deltas, so for ex: foo_field_formatter_view() is currently outputting only deltas 1 and 2, but it has 4 items in all: it must set #readmore to TRUE.

B. Somewhere further up, the #readmores need to be collated.

C. Somewhere else again (?), hidden fields need to be taken into consideration,

Overall, $entity->readmore is TRUE if either B or C request it.

Is that a reasonable (albeit rough) summary?

@intoxination: you sound like you've played more with the innards of all this than me. Care to have a go at a patch?

That's pretty much it. I believe the work of B and C can pretty safely occur in node_build_content, especially given the now entity nature of nodes. If any other entities want to take advantage of the behavior, then they can simple mimic the same procedure. At least the work of handling the new key will already be in place (and I agree - readmore sounds good).

I'll work something up this coming week. I got a lot on my plate this weekend, but should be able to start on it Monday. I got a couple ideas floating around in the noggin also that should keep text_summary intact so that it doesn't break the API if any other modules use it. At least a "temporary" work around so we can maybe get this in D7 and then change the return value of text_summary in D8, before the API freeze occurs.

Great! I look forward to seeing it. In the meantime, changing status back to 'needs work'.

Here's go number one at this. Keeping with my idea to not actually break the API of text_summary, it made that function a little more ugly. I got some commenting in there, but not much yet. I figured we can get people testing it at least.

Thanks! I think it's potentially simpler than the earlier approach, but I've a few questions...

First though, you're missing the tests scito wrote -- those need to be in.

Second, I don't see where you're accounting for fields simply hidden on teaser view.

+++ b/modules/field/modules/text/text.module
@@ -341,10 +345,13 @@ function _text_sanitize($instance, $langcode, $item, $column) {
+ * @param $return_status
+ *   Set to true if you would like an array returned, including 'text' and
+ *   'trimmed'. If trimming occurs, array key trimmed will be set to true.
  * @return
- *   The generated summary.
+ *   The generated summary or an array containing the keys summary and trimmed if $return_status is true.
  */
-function text_summary($text, $format = NULL, $size = NULL) {
+function text_summary($text, $format = NULL, $size = NULL, $return_status = FALSE) {

This kind of change really isn't going to fly, I'm afraid. Even if we were still in development, a function that changes its return is a bit iffy :/

Instead, what about a wrapper to this which merely passes on the $text and on its return, check if there's a difference.

So:

function needs_a_good_name($text, $otherparams...) {
 $summary = text_summary($text, $otherparams...);
 if ($summary == $text) {
  // Nothing changed
  $trimmed = FALSE:
  }
  else {
   // Summary is shorter.
  $trimmed = TRUE;
  }
  // Return an array accordingly.
}

+++ b/modules/node/node.module
@@ -1370,15 +1371,32 @@ function node_build_content($node, $view_mode = 'full', $langcode = NULL) {
+    foreach ($node->content as $item){
+      if (isset($item['#access']) && $item['#access'] == 1){
+        foreach ($item as $key=>$value){
+          // check to see if the key is a property or child. We want children only here
+          if ($key === '' || $key[0] !== '#') {
+            if (isset($value['#readmore']) && $value['#readmore'] == TRUE){

I would put this logic in a helper function in field module which returns TRUE if the entity it's passed is in a 'read_more' state. This would allow modules other than node to use this too if they wish -- it makes sense too, given it's the fieldAPI modules that put in the #readmore in the first place.

This also saves you the ugly 'continue 2'! :)

Finally, to anyone making patches -- please take time to check your code style, eg things like 'TRUE' rather than 'true', and how to indent and bracket. If you're not entirely familiar with the Drupal coding guidelines, I strongly recommend you use Coder module to review your code before rolling the patch -- it saves everybody time :)

Powered by Dreditor.

Yeah I haven't done the tests yet. Just wanted to get a rough patch out there before getting to involved in it.

I'm not really sold on the hidden fields. First off, fields not shown on the "teaser" view shouldn't have summaries or breaks set in them, since they would never be used. If we decide that a field shown in full view, but not shown in teaser view should trigger a readmore, then which fields? Text only or all of them? I can see that leading to other issues also, such as people who put an attachments list on their posts, but that only shows on the full view and they don't want a readmore for it.

If you look, by default it doesn't alter the return value, only if the 4th parameter - $return_status - is set to true, does it then change to return the result array. That's a new parameter I added in to keep backwards compatibility with the function. It might be worth going back to the string compare equality check though, as in the first patch. There still might be some weird scenarios where a string isn't actually truncated, but would not be equal to the original string with the truncated string running through _filter_htmlcorrector() (if set in the filter), but those should be very, very fringe cases and I think the same would even occur in D6.

Putting the readmore check in a helper function is a great idea. I think the biggest question is if it should be in filter.module or text.module. This is where getting some input from the FieldAPI people would be nice.

And yeah I noticed the lowercase true and false's. I really need to get coder reinstalled on here. If only we could make a patch to add more hours to the day lol.

> If we decide that a field shown in full view, but not shown in teaser view should trigger a readmore, then which fields?

All of them. Read more means 'If you click this link you will see more stuff than you can see right now'. That includes longer text fields, more items from a field, hidden fields. Not sure about bigger pictures though -- but that's a bikeshed I think :)

> but that only shows on the full view and they don't want a readmore for it.

Then it'll be up to them to customize it with the theme or alter hooks.

> If you look, by default it doesn't alter the return value, only if the 4th parameter - $return_status - is set to true, does it then change to return the result array. That's a new parameter I added in to keep backwards compatibility with the function.

Yeah... that's the problem -- the code has to jump through all these hoops to figure out what kind of thing it should return. That really is ugly, sorry.

> I think the biggest question is if it should be in filter.module or text.module.

Hmm good question. I'd say text module.

The hidden field issue is going to have a serious performance impact though. The only way to know if there are fields showing on full view, but not teaser view is to also generate a full view, since you have to take into consideration optional fields that might not be filled out (hence all fields will have to be loaded, their data calculated and their #access checked).

I'm afraid readmore is pretty much going to have to pertain to fields only that are available in teaser view, or we are going to have to stick with the current behavior as-is. I would love to see if there's an issue that discussed this original change in D7 to see why it went this way. I haven't been able to find one.

Oh I see what you mean -- this case:

node X has field Foo on it, and field Foo happens to be set to hidden on teaser AND on node X is empty anyway.

We don't want field Foo to request a readmore in this case, as it has nothing to show.

Would the $node object actually hold the $node->field_foo data, or is that cleverly skipped at the node_load() stage?

The fields are loaded in the entity loader (node_load()), but the field_access checks aren't done until field_attach_view, which is in node_build_content.

Once scenario I really see that this could break in is with OG. I have a D7 site running OG and I have the group field showing on the full view, but in field access it's disabled for everyone (that way it only shows to user 1 for debugging). Since that field is there and the access check is never done, every group post would show a readmore because of that field that is only visible to user 1.

See also #221257: text_summary() should output valid HTML and Unicode text, and not count markup characters as part of the text length

Here's another go at the patch. I put the tests in this time, using mostly the ones @joachim wrote since the test outcome is the same (just dropped the hidden fields test).

- I went back to just doing a string equality that checks for readmore and then sets the #readmore attribute on the return array.
- Moved the checking of #readmore on renderable fields to a helper function in text.module (text_has_readmore()).

> mostly the ones @joachim wrote

BTW -- I didn't write those tests, they were from an earlier patch I rerolled and did a bit of work on.

+++ b/modules/field/modules/text/text.module
@@ -445,6 +455,37 @@ function text_summary($text, $format = NULL, $size = NULL) {
+function text_has_readmore($elements){

It's the whole node rather than just the text we're asking about right?

Also, it should read_more, not readmore, throughout -- note the way in Drupal we say pre_render and don't mashwordstogether ;)

So node_has_read_more(). Or entity_has_read_more().
I don't think it belongs in text module either.

+++ b/modules/field/modules/text/text.module
@@ -445,6 +455,37 @@ function text_summary($text, $format = NULL, $size = NULL) {
+      foreach ($element as $key=>$value){

Don't use $key => $value unless you can possibly avoid it. Also, if you only want children, use element_children(). And watch the spacing ;)

+++ b/modules/field/modules/text/text.module
@@ -445,6 +455,37 @@ function text_summary($text, $format = NULL, $size = NULL) {
+        // check to see if the key is a property or child. We want children only here

Capitalize the first word of the sentence; end with a full stop; line break at 80 chars.

This is nitpicky I know but a patch will NOT be committed to core unless it passes these standards -- it won't even get to RTBC. Please please try to follow them -- otherwise it just means another iteration of work on it. It's quicker if the patch author follows them, rather than the reviewer have to go through and pick up on them all.

+++ b/modules/node/node.test
@@ -2146,3 +2146,141 @@ class NodeTokenReplaceTestCase extends DrupalWebTestCase {
+    $this->assertNoText('Read more', t('"Read more" does not appear in the default listing.'));

These could do with more detail. If I read a test report, I wouldn't know what sort of test had failed and why -- as I currently am doing with the testbot's latest result...

Also, your patch seems to include the patch... :/

Powered by Dreditor.

Also, I suspect the implementation will need to be recursive. Perhaps something like this (where I've omitted the docblock in the name of space):

function node_has_read_more($elements) {
  // Early-return if the user does not have access.
  if (empty($elements) || (isset($elements['#access']) && !$elements['#access'])) {
    return FALSE;
  }

  // Return if #read_more is set on this element.
  if (!empty($elements['#read_more'])) {
    return TRUE;
  }

  // Iterate through children.
  foreach (element_children($elements) as $key) {
    if (node_has_read_more($elements[$key])) {
      // A child element has #read_more set to TRUE.
      return TRUE;
    }
  }

  // Neither this element, nor any child elements had #read_more set.
  return FALSE;
}

If someone else wants to take a go at this, then go ahead. I'm not really going to have anytime for at least the next week, as I'm heading out of town.

Be nice to be able to quite simply do without them. Please don't anyone suggest display:none.

Subscribing. This bug is embarassing for D7 :-/

I would love to see if there's an issue that discussed this original change in D7 to see why it went this way. I haven't been able to find one.

#372743: Body and teaser as fields

I'm back and here's another go with the following:

- Changed function to field_has_read_more and moved to field.module and made it a recursive function.

I'm not really sure about readmore -v- read_more. Going back throughout Drupal history it has always been referred to as readmore. Also going from #82, we are in D7 and entities now. Calling it node_has_read_more really isn't accurate, as different entities could utilize this (ie: users or comments). Also placing it in field.module seems more appropriate as other fields could use this down the road.

I've tested it out somewhat and it seems to be working good. More testing will be needed though.

We'll see how the tests go, too...

Checking to see if this conflicts with #221257: text_summary() should output valid HTML and Unicode text, and not count markup characters as part of the text length

It actually might not be a bad idea to merge both issues. There shouldn't be any conflicts, but given the extremely close relation, I could see something popping up down the road.

I propose to add another test to the case -> check if some text here <!--break--> will trigger readmore. It should not.

subscribe

subscribe

Patch #89 is working great for me so far

Bumping and tagging as per standard policy.

You can help this issue by posting a review:

• Decide whether all code and comments comply with Drupal coding standards.

• Decide whether any included tests are necessary and sufficient.

• Decide whether the patch actually solves the problem. There should be a reproducible test where the current code fails and the patched code succeeds.

A review that affirms success in all of the above should also:

• Change the issue status from "Needs Review" to "Reviewed and Tested By the Community".

Otherwise, the review should:

• Explain what is wrong with the proposed patch and/or supply a corrections to it.

• Change the issue status from "Needs Review" to "Needs Work".

Patch in #89 is working great. I hope this is moved into D7 Core.

Patch #89 has been working well for me

Patch #89 has been working well for me, it should be set to "Reviewed and Tested By the Community"

I installed a clean version of Drupal 8.x-dev. Then I created a test article. The Read more link appears on the front page even though the entire article (one word) appears. I applied the patch. The Read more link no longer appears. Reviewing the code I only found a missing space between a function and the open bracket:

function field_has_read_more($elements){

I did not see any other problems with coding standards.

From the discussion and a quick look at the patch it looks like the tests supplied are sufficient.

Re-rolled based on updated #221257: text_summary() should output valid HTML and Unicode text, and not count markup characters as part of the text length

Bah. Fixed that missing space.

(sigh) trying again, minus my custom changes to the standard profile...

#19: better_read_more.patch queued for re-testing.

Corrected patch, also includes latest patch from #221257: text_summary() should output valid HTML and Unicode text, and not count markup characters as part of the text length

Ran coder_review and updated as per its recommendations.

(sigh) The description for text_summary() says that it produces trimmed output, yet actually doing so breaks stuff all over the place.

Marking RTBC as before.
/me ducks!

The original patch in #87 was not related at all to #221257: text_summary() should output valid HTML and Unicode text, and not count markup characters as part of the text length, so I've no clue why this issue got extremely hi-jacked by merging another major issue into this one.

For this patch to be remotely reviewable and committable, we need to go back to the patch in #87, either directly, or by extracting the relevant parts from the latest patch (in case there have been any changes).

Re-rolled as requested, but the two patches are related; they do overlap; they do break each other; and they were merged at the suggestion of intoxination in #823380-90: Read More link is always visible on teaser..

Removed some debugging code I had inadvertently left in.

Also attaching an interdiff for completeness.

RTBC as before, again.

This patch requires an in-depth review and sign-off from entity/field system maintainers.

The approach taken is interesting, but as well debatable on its own. It looks like it hard-codes even more logic for the 'teaser' view mode. That might very well be all we can do for D7, but I'm not sure.

I'm also not sure whether this patch possibly has a negative impact on performance, or whether the recursion into renderable elements happens within a cached code path.

Lastly, the patch contains a lot of unrelated (and partially wrong) code clean-ups. It would be helpful to remove those to facilitate more focused reviews and to make faster progress.

@sun

Lastly, the patch contains a lot of unrelated (and partially wrong) code clean-ups.

Can you point to at least one of the "partially wrong" bits?

Subscribe for now. Are there any specific comments I should read to get a summary of the discussion leading to the approach taken in the patch ?

This is basically leaving it to formatters to say "I'm -probably- displaying a shortened version of what is -probably- available on the 'full' view mode". That's a somewhat reasonable assumption for the 'trimmed' and 'summary_or_trimmed' formatters for text fields, but that does introduce some one-off logic about *node* *teasers* into some entity-and-view-mode-agnostic formatter code.

At the same time, it leaves out the case where a 'read more' link would make sense just because the full node displays a larger/different set of fields. Someone rightfully pointed that those cases would be fairly difficult to detect, and IIRC that's the main reason why the 'Body as field' patch just made every teaser get a 'read more' link. 'Read more' is a notion coming straight from the pre-CCK era, and is difficult to map to a fieldable world...

This being said, on the plus side of the patch, the #read_more property makes it easier to provide 3rd party logic.

Still pondering...

#116: better_read_more-823380-116.patch queued for re-testing.

Patch #116 doesn't work with following code under 7.2 (worked under 7.0, teaser cut option is 2400 chars)

<table  border="1" width="100%">
	<tbody>
		<tr valign="top">
			<td style="border-right: 1px solid;">
				<p>
					Die Viteritas GmbH ist eine bankenunabh&auml;ngige Servicegesellschaft, welche sich auf die professionelle Verwaltung von Kundenkonten (Managed Accounts) mit diskretion&auml;ren und automatisierten B&ouml;rsen-Handelssystemen spezialisiert hat.</p>
				<p>
					Wir sind ein Team von erfahrenen und professionellen H&auml;ndlern mit Zertifizierung als Technische Analysten mit Spezialisierung auf den Bereich Forex-, Rohstoff- und CFD Trading. Unser Ziel ist das Erreichen einer langfristig, &uuml;berdurchschnittlich hohen Performance f&uuml;r unsere Kunden im aktuell volatilen Marktumfeld.</p>
				<p>
					Investoren k&ouml;nnen &uuml;ber ein Managed Account (verwaltetes Depotkonto) investieren. Dabei vermitteln wir die Handelskonten bei einem Broker und unterst&uuml;tzen unsere Kunden bei der Kontoer&ouml;ffnung.</p>
				<p>
					&nbsp;</p>
				<ul style="list-style-type: circle;">
					<li >	High Quality Papiere</li>
					<li >
						Transparente Strategien</li>
					<li>
						Erleben sie unsere Trading-Strategien live</li>
				</ul>
				
			</td>
			<td valign="top" width="30%">
				<span style="color: rgb(169, 169, 169);">Rechtlicher Hinweis: Die Angaben auf unserer Homepage stellen keine Anlageberatung dar, sondern dienen ausschliesslich der Beschreibung einzelner ausgew&auml;hlter Aspekte des dargestellten Produkts. Dieses Angebot richtet sich an Anleger die in der Schweiz wohnhaft bzw. domiziliert sind.</span></td>
		</tr>
	</tbody>
</table>

sorry for #122, Patch worked after setting the Teaser cut size to 3400 chars.

The patch in #116 is not against 7.2; it is against 8.x.

And you haven't given enough information to identify a problem or code a fix.

Given the above data in the body of a node, and a teaser length of 2400 characters, please provide your expected output from text_summary. (In other words, what would the teaser contain?)

With that minimal information, I can write a test. If the test fails, I can fix the code so that the test passes.

@ #124 - the teaser is empty, all the html code is in the body, but as I stated in #123, after setting the "Trim length" to 3400 (2400 works also) there was no problem - The "read more" Link didn't appear after
applying this patch (even if it is for 8.x).

So ... I'm confused. Is there a problem, or not?

There is no problem :-)

#62: 823380.drupal.field-teaser-read-more.patch queued for re-testing.

I am confused, what should I apply for Drupla 7?
Patch of #128 oder the "better ..." patch of #116 ?
or nothing, since its for Version 8?

I'll re-roll patches shortly.

Patches for 8.x and 7.x.

@Morn and other d7 users should apply better_read_more-221257+823380-131-d7.patch to a 7.x git checkout (not the same as a release version).

Unassigning myself as I've really done all I can do at this point.

Opened alternative proposal: #1185936: Rename the "Read more" link to "View on separate page"

+++ b/modules/node/node.testundefined
@@ -2146,3 +2146,141 @@ class NodeTokenReplaceTestCase extends DrupalWebTestCase {
+}
\ No newline at end of file
-- 

Add a newline

Powered by Dreditor.

No need to put needs work for only that :)

It's unfathomable and incredibly short-sighted that the functionality of the $readmore variable was deleted in D7. I am a designer, not a coder; this GIT system looks way too hairy. I will try my best to spoof how the system worked in D6 by creating a field so that I can create an ad hoc link on a node-by-node basis, as needed.

This is being worked on. And, since its going to require review before getting into 8.x, and then more review before being considered again for 7.x, it would be best if you could help review this or offer constructive suggestions for improving the way the read more link works.

While it does stink that the read more system works so poorly in drupal 7, things will get better... And there are definitely ways to work around this situation.

My suggestion is to restore the functionality of the $readmore variable. Whatever the code was underlying that variable in D6 and earlier, I'd like to see it exposed for all to see. I have searched and searched, to no avail. All I see is "$readmore: Flags true if the teaser content of the node cannot hold the main body content," in http://api.drupal.org/api/drupal/modules--node--node.tpl.php, under Node status variables.

It seems to be that testing for, in shorthand, $teaser != $page is a part of the full picture, but other factors are at play, as others have pointed out. So, the question is, just how did that variable flag as true?

If metamorphmuses asks nicely and specifies which version of Drupal he is using, I might be persuaded to roll him a separate patch. Alternatively, he could learn to roll his own.

pillarsdotnet, I appreciate that you and others have been working to get a patch done. My ire is directed at the collective decision to remove functionality in D7 that existed in D6; my attitude is that successive versions should always augment, and only deprecate when there is a clear consensus that a particular feature is obsolete (e.g. the deprecation of HTML frames, although even those are still supported).

As to the patches that rely on GIT, if I decide they are worth trying, I will learn how to go that route. Thank you for offering up assistance.

Like I stated above, I am going to try to implement a read more field that can be applied to an individual node as needed.

Metamorphmuses:

I have a simple block of code at http://drupal.org/node/823380#comment-4191332 that you can use to tell if a node is showing truncated text or not. You can easily put it in a hook_preprocess_node in the template file.

As far as the version, changing this back to 7 makes it much less likely it will get attention from the core developers. Development is currently on D8 and the practice is for items like this to be made available in the version on current development and then backported to the current release.

The read more link doesn't work properly in 6.x either. See #201854: "Read more" link sometimes shown when it shouldn't be, or not shown when it should.

The Patch #131 for D7 works also for 7.4

Does patch #131 resolve the problem of the read more link displaying on node teasers that have more information to be displayed? If so, where do you apply the patch? I am enquiring for drupal 7.

Thanks

@144, yes, but it could be that you have to adjust the the teaser length for your content types

do you apply this to the node module?

do you apply this to the node module?

I used Git bash for windows and applied the better_read_more-221257+823380-131-d7.patch to Drupal 7.4
Put the patch file in your downloaded d7 Main diretory and issue (cd to the directory) with git bash
git apply --verbose *.patch

The Patch #131 for D7 works also for 7.7 (when will it be integrated in D7?)

@Morn

when will it be integrated in D7?

Probably never.

The problem is that, since an arbitrary number of fields can be attached to any node type, and each field can individually be turned on or off for the teaser display, there is no easy way to test whether the teaser is equivalent to the full node. The patch in #131 merely tests whether all the "text_with_summary" fields have a summary that is equal to their full content. While this is probably useful for many users who are merely seeking to upgrade a D6 site to D7, the whole approach needs to be re-thought and re-worked to become core-worthy.

One approach is to rename the "Read more" text to something more descriptive.
See #1185936: Rename the "Read more" link to "View on separate page"

> The problem is that, since an arbitrary number of fields can be attached to any node type, and each field can individually be turned on or off for the teaser display, there is no easy way to test whether the teaser is equivalent to the full node.

Of course there is -- if the field is not being shown on the teaser, then the teaser is not equivalent to the full node.

I've not been following this patch for a while, but I thought it was doing that?

Without this patch, the amount of time needed for the Upgrade D6->D7 increases a lot.
Comparing with other CMS systems the drupal core upgrades are an horror for me.
Specially when my costumers dodn't like the Option "View on the separate page"

Re. Comment #52: the condition in the first line should be '==' not '!='
i.e.:

if ($variables['content']['body'][0]['#markup'] == $variables['node']->body['und'][0]['safe_value']){
   unset($variables['content']['links']['node']['#links']['node-readmore']);
}

Many thanks for this - I didn't want to run a patch against core, and dropping this into my node preprocess function in template.php worked great for me.

bump

Seems to me this is a bug and was accidently changed to feature request.
At 150+ comments in, I think this issue could do with a summary on the approach taken for the proposed solution to give reviewers an easier way to get up to speed.

Is there a simple (or not simple) way to disable the read more link on teasers? Whether on the front page or not? Thanks.

This is not a support forum.

added a summary

I agree with yoroy -- this is a bug, because it's a regression from D6.

Added a summary.

oops. messed it up. editing the OP for a summary is a total crack way to do it and I hate having to write log messages anyway

I was adding one at the same time. Hopefully, I successfully merged our separate efforts.

Re-rolled #131, removing unrelated clean-ups.

(sigh) The new comment tests added by #983632: New comment marker does not work are unnecessarily testing for the existence of the "Read more" link and failing when it does not exist.

Commented out the failing tests.

@#159: yup, combined summary looks great :)

I give up. There are too many tests that depend on the existence of the "Read more" link. How about we hide it via CSS instead of removing it altogether?

Use xpath queries to check for class existence / nonexistence in the link.

Crosspost with hopefully a slightly improved summary.

And as I read the docs, I realize that it should have been 'element-hidden' rather than 'element-invisible'.

> How about we hide it via CSS instead of removing it altogether?

I thought you were kidding when I read that but I see you've added it to the summary.

That really doesn't seem like the best way... is it that hairy? :(

That really doesn't seem like the best way... is it that hairy? :(

Here's the patch and interdiff. Judge for yourself.

Reputedly this looks spammy to search engines.

Please supply a reference to support your assertion. Reputed by whom?

If that is true, having your front page take an SEO hit is not good.

The .element-hidden class is used for collapsible fieldsets, and the .element-invisible class is used to display alternative text to screen-readers. I've never heard of them being SEO-unfriendly. There's a lot of Drupal code that uses these features:

bobvin@bowie:~/www/8.x$ git checkout 8.x
Switched to branch '8.x'
bobvin@bowie:~/www/8.x$ egrep -r 'element-(hidden|invisible)' includes modules profiles themes | wc -l
51

Fifty-one times in D8 core. And five more added by this patch:

bobvin@bowie:~/www/8.x$ git checkout 823380
Switched to branch '823380'
bobvin@bowie:~/www/8.x$ egrep -r 'element-(hidden|invisible)' includes modules profiles themes | wc -l
56

Dunno how that extra semicolon got there.

Describe the 'element-hidden' strategy.

Weird. Installs locally; tests pass. Testbot problems? Trying again, and also removing the trim() calls added for compatibility with #221257: text_summary() should output valid HTML and Unicode text, and not count markup characters as part of the text length.

> The .element-hidden class is used for collapsible fieldsets, and the .element-invisible class is used to display alternative text to screen-readers.

Yes, and those are both good cases. When fieldsets are collapsed they are hidden, and screen readers need additional context that in visual output is superflous.

This is hiding something so our tests still pass... doesn't smell good to me at all. It seems rather akin to the times when a themer I'm working with hides something with CSS rather than properly using the theme to remove its actual output.

Wordsmithing.

Open up a separate issue to clean-up and/or remove the tests that assume the "Read more" link is always present.

Marking this issue "postponed" until then.

EDIT: updated summary to reflect the 'postponed' status.

We shouldn't have major bug reports in a 'postponed' status. Downgrading.

It being postponed doesn't change its severity!

I do appreciate you feel burned out by all these failing tests though :(

Well, unless one of you is willing to open an issue to rewrite all those patches, it's going to be "won't fix".

Meanwhile, this issue is blocking other improvements to Drupal core.

See Drupal core code freeze, code thaw, issue count thresholds.

It's a major end-user experience WTF to click on a "read more" link and not be given anything more to read.)

The user does get something more to read, or at least something different. The teaser display is rendered differently from the full node, even if no text was trimmed and no fields were removed.

That's why I opened #1185936: Rename the "Read more" link to "View on separate page"

@#181 by a1tsal

OK, I have opened #1300568: Fix tests that wrongly assume all teasers have a Read more link

Great. Now someone should (at least) identify the files and line-numbers where those tests are located.
(Posting here rather than there because I do not want to subscribe to that issue.)

I hestitate to jump in with something which seem a distraction, but a useful interim solution for D7, may be a contrib module adding a 'no read more link' check-box field to each content type, and removing the Read More link if it is checked?

@#183 by John_B:

a contrib module adding a 'no read more link' check-box field

I look forward to reviewing your project application.

In response to some of the recent comments:

1. Changing the status of a bug to reduce the blocking count is not the way to manage our bugs, and makes the blocking count meaningless. If this bug blocks ongoing development, then so be it: that means that ongoing core development is here on this bug.

2. I've not looked at the code since pillarsdotnet flagged this, but it seems to me that the tests should be fixed or ripped out as part of this patch? I'm not sure we need a separate issue to fix or remove those tests, though I can see that having an issue to concentrate on just those might help simplify the work.

3. There is already a sandbox module for fixing this problem. I gave a -1 its promotion to full project because making a contrib module to work around a problem in core is not the way to do things. It has been done before, but that's no reason to do it again.

Before you rip out tests that someone else wrote, you'd better coordinate with whoever wrote and/or approved those tests.

We have tests for a reason -- to reduce the possibility of regressions. Somewhere, somehow, there was a bug which was fixed, and a test was written to make sure the bug stays fixed. You rip out that test without a deep understanding of why it's there and you destroy the whole quality assurance system.

It would probably take me four to six hours to research (using git blame and reading through issues and patches) exactly why each and every one of those tests were written. (Although I'm sure some core developers could tell you from memory, good luck getting their attention!)

At that point, we still would not have buy-in from the core developers, (see #118 above), so not the resultant patch would be even less likely to be accepted.

Which makes this entire issue very nearly a "won't fix".

Really, we're at comment #186 and there has not been a single positive review from anybody in MAINTAINERS.txt -- this issue is doomed.
EDIT: Retracted; see #188

@joachim Thanks for the heads up on the workaround module whose application you rejected. You probably mean Read More Control.

A contrib module only as a workaround for a core problem may be inelegant. However, it could save hundreds of developers delivering thousands of Drupal sites to end users most of which have this (superficially simple) problem which makes Drupal look bad. Maybe the project's application should be reconsidered. I am certainly going to try the sandbox module.

Sent contact emails to catch, Frando, ysched, and bjaspan as follows:

As an Entity/Field System maintainer, could you please look at:

Issue #823380: Read more link is always visible on teaser

It is currently stalled because it lacks "an in-depth review and sign-off from entity/field system maintainers." and also because the generally agreed-upon solution conflicts with tests that assume the "Read more" link is always present.

See related: #1300568: Fix tests that wrongly assume all teasers have a Read more link.

Even a decision of "closed (won't fix)" would be better than nothing, at this point.

Think I found the problem with the xpath query in #174. Re-rolling

This is essentially the patch from #162 combined with the one from #1300568-4: Fix tests that wrongly assume all teasers have a Read more link

Messed up the patch in #190 -- here's another try:

Another try at the element_hidden approach.

And this is the alternative "rip out the failing test" approach.

Re-rolled.

And again.

@pillarsdotnet - You are missing a blank line before @return in the docblock for field_has_read_more().

@#198 by Lars Toomre on October 7, 2011 at 6:21am:

@pillarsdotnet - You are missing a blank line before @return in the docblock for field_has_read_more().

Thanks; corrected in both patches and re-rolled.

Added link to blocking issue.

Update links and remaining tasks.

Okay, now that both approaches pass tests, this issue is officially postponed until we get a response from catch, Frando, ysched, or bjaspan.

Re-opened and marked as blocking this issue:

• #983632-46: New comment marker does not work

• #1204658-5: Always use query metadata to specify the node access base table

Correct the order of blockers; we've been waiting for a review a lot longer than we've been waiting on tests.

A quick search gave no clean results for a nice integrated method of handling teaser / body / readmore. There are about 5 or 6 related issues, but this appears to be the most active related thread. I am about to tackle this using multiple fields, but would love it if I didn't have to.

Requirements / Outline

A WYSIWYG on both Teaser / Body with different filter format options.
Teaser generated by:
a) Teaser field only
b) Trimmed body field
Read more shown if (default)
a) Both fields are present AND teaser != body

Planned fields

field_summary - text_long
field_include_teaser - list_boolean
field_body - text_long
field_readmore - list_integer << Set default override in Content type settings

Final UI

Summary
[ Textarea ]
[*] Include summary on full page view
Body
[ Textarea]
Read more
(*) Use summary and body fields to determine if a read more is required
( ) Always show a read more link
( ) Never show a read more link

Implementation

From this approach in D6, I would only really only would need to add a toggle in the node view hooks and a new conditional teaser_or_body view field.

In D7, then there is also:
a) Requires a way to upgrade from the existing body field or a way to convert this to text_long
b) A way to stop adding the default body field and replace with this field collection

Thoughts? Similar existing modules? I really need a solution in the next few days, so I will be starting this today. I've started a forum thread http://drupal.org/node/1318254 to continue this discussion and to stop polluting this thread with additional noise.

Well, two weeks have passed with no input from any of the core field/entity maintainers, so I'm afraid this is a "Won't fix".

Please re-open this issue if you are a core field/entity maintainer and are willing to review one or both of the patches in #199.

Sorry, but that's ridiculous. Plenty more time passes with bugs in this sort of state without input from maintainers, and maintainers simply won't see a bug set to won'tfix, because if you're a maintainer you probably don't bother reading those.

@pillarsdotnet - I have also waited for months for any attention for some patches I've worked on or would like to go in... If you're really disgusted with how things are going, rather than mark an issue as closed (especially since many others would still like the issue resolved), just unfollow the issue and forget about it. Your patch will likely be further tested at some point, and I'm pretty sure your username will be associated with the commit if it ever comes to happen.

Please stop marking these large issues as "won't fix" simply because they've been stalled.

Shouldn't this be needs review

Yes, please don't won't fix issues just because they're stalled. I have issues that have been at CNR for months or years (and I've also been on holiday for most of the past three weeks). There's also no impasse here that needs feedback from a maintainer, the latest patch has barely been reviewed by anyone at all.

So I only see three options here:

1. Leave 'read more' on every teaser, mark this won't fix.

2. Just remove 'read more' support or make it optional or something.

3. Something like #99.

I'm not sure which is preferable but since this is an unintended regression from D6 we should try to do #3 probably.

If we go with #3 then #99 looks like it's along the right lines. However I'm not sure about whether this should be an entity level construct (i.e. $content['#readmore'] that Field API could then set but we allow other code to see it too), or a field API construct as set in the patch.

@Catch -

I believe #2 really shouldn't even be considered an option. I could imagine a site upgrading to D8 and suddenly having full nodes appear on multi-node pages. The only way I see that working is if a core-optional module is provided to provide 'read more' that is enabled by default.

#1 might not be bad, but if that were the case I believe that the trimming functions should actually be moved into a hook so that other modules could implement their own 'read more' or remove the core function all together. Possible something along the lines of hook_textfield_trim or something like that. (Technically you can override this now, but that requires overriding the formatter through hook_field_formatter_info_alter(). One problem I see there is that the module overriding it has to then make sure to handle/call the _text_sanitize and if this is overlooked we have a very serious security issue.)

I heavily favor #3, especially given the attention this issue has and going from the complaints I have heard about it. Placing of the construct is an interesting question though. 'Read more' is tied so closely to the entity level and field API system. I lean more towards this being constructed on the entity level though, since the functionality actually depends upon the presence of a display mode that utilizes the teaser function and the entity has to handle the site structure to display the different modes.

But I would say that in 99% of the cases out there, 'read more' is used solely by the node module. I can also see that changing as people start getting more comfortable with entity API and field API. An example would be a forum module that doesn't rely upon nodes/comments, but rather their own tables (this came to mind because of something I might be doing for a client). Given that I heavily lean towards keeping this in the field API system and letting the entities act upon a flag set by the the formatter.

Here's another use case I can think of if we go the #3 in field API route:

A publisher creates a special entity for the books they sell. In it they allow visitors to read a selected chapter and the introduction:
- A text field for the introduction that can be trimmed.
- A text field for the sample chapter that can be trimmed.

Now the entity in the teaser display can look the the #read_more flag and append a "continue reading" or whatever link to the bottom of that field, both of which would go to separate urls (/book/####/introduction, /book/####/sample).

I really believe keeping it in field API and making it something that can be easily implemented and modified while not giving false positives could lead to some really interesting things done down the road.

Update links.

Updated issue summary.

I closed #1300568: Fix tests that wrongly assume all teasers have a Read more link as a duplicate. Reasoning: D7/8 core currently has the assumption that a teaser always implies a "Read more" link. If we are changing that assumption in this issue, then we should also correct the tests that include that assumption in this issue, and add more sophisticated test coverage for the more sophisticated behavior (which the existing patch already does.

Attached:

• Adjusts patch for the core/ directory move.
• Updates existing tests to remove implicit assumptions from other tests (so the issue is no longer blocked by any other)
  • The assertions in the comment link tests are out of scope for the comment link tests anyway. Whether or not the comment count is shown is unrelated to whether the "Read more" link is shown.
  • The assertions in the node access test are changed to instead check that the node title links to the node. (The intent in that test is to confirm that nodes are linked or not based on access control, and the title serves this purpose just as well as the "Read more" link.)
• Clarifies API documentation for the added API functions and test methods.
• Adds various other code style cleanups.

It does not address @catch's comment above:

However I'm not sure about whether this should be an entity level construct (i.e. $content['#readmore'] that Field API could then set but we allow other code to see it too), or a field API construct as set in the patch.

Updated issue summary.

+++ b/core/modules/node/node.testundefined
@@ -1100,8 +1100,8 @@ class NodeAccessBaseTableTestCase extends DrupalWebTestCase {
-      foreach ($this->xpath("//a[text()='Read more']") as $link) {
-        $this->assertTrue(preg_match('|node/(\d+)$|', (string) $link['href'], $matches), 'Read more points to a node');

There should probably also be a test to check if the read more link actually points to a node (or even better, points to the correct node; the one created in the test), as this check is removed by the patch.

Well, the removed test coverage wasn't intended test coverage (and has nothing to do with read more links; rather, it's a node access question), but we can add an additional assertion to the new tests.

chx also pointed out that (e.g.) $readmore should be $read_more.

Waiting for the bot to post any reroll.

But by removing it we lose the unintended test this gave us. I agree it should be removed, but it would be great to have a test to check if the read more link actually works (points to the correct node).
Maybe by using clickLink and assertText for the node title?

Right, but not in the node access tests. :)

Definitely not. The new tests added by this issue would allow this to be tested easily by adding clickLink and assertText on the title (or some more clever way).

Interesting. So it looks like the comment count display is dependent on the presence of the read more link. It shouldn't be. So we'll need a solution that decouples that as well.

Actually, come to think of it, there's a case to be made that the "Read more" link should be present when there are comments. Because, even if not in the node body itself, there's more to read in the comments. Can anyone confirm what the D6 behavior is for a node where the teaser is equal to the body, but there are comments?

In D6 comments don't matter. The read more link is totally dependent upon the node/teaser. Having it dependent upon comments should really be something for contrib to handle. The case being is that not everyone uses Drupal comments. Some may use 3rd party systems such as Disqus. In that case it would be up to that module to decide if read more should show for comments and connect with the Disqus API to determine if comments are present or allowed on that node.

Thanks Jamie! To clarify, the read more link:

• Should always be present on a node when the summary is not the same as the body. (Provided by node.module, I think.)
• Should potentially be present on a node when the summary is the same as the teaser, but there is other information to view. (Provided by the module that provides the other information. In the case of comments, comment.module would provide this, so it would not occur if comment were disabled.)

But if the second bullet doesn't happen in D6, then we probably need to fix comment.module now for the failures above.

This also raises the larger question of what the link actually means. For example, what if certain fields are displayed on the full node, but not the teaser? Should those count? The thing that bothers me the most about the patch above is special-casing longtext with summary fields. Really it's a node.module functionality and I don't think code for it should appear in the Field API at all...

+++ b/core/modules/node/node.module
@@ -1416,7 +1416,9 @@ function node_build_content($node, $view_mode = 'full', $langcode = NULL) {
-  if ($view_mode == 'teaser') {
+  // Only show the "Read more" link if the view_mode is 'teaser' and at least
+  // one field has the '#read_more' property set to TRUE.
+  if ($view_mode == 'teaser' && field_has_read_more($node->content)) {

So if we want something more generic, how about:

- move calling of field_has_read_more() to field module's implementation of hook_node_view().
- other modules such as comment can then opt to set the read_mode flag on a node in teaser view in their own hook_node_view() implementation.

That's what I meant was in the case of showing the read more link if node == summary and comments are there should be handled by the module that handles the comments (core, disqus, intense debate, etc.).

In the case of multiple text with summary fields, this does pose an interesting situation. Perhaps something along the lines of each field that has a true summary (summary != body) a flag is set on that individual field. The node module then checks each text with summary field and if one of those fields has that flag set then the read more link is displayed (same as D6).

This route would open up the possibility of a contrib module to do more advanced read more functionality. For example I have a node with 2 text + summary fields. Field 1 has an actual summary, but field 2 does not. A read more link could be appended to field 1's output and the link actually anchored to that field on the node view or even pull in the rest of the field via AJAX.

What this does is brings us more inline with the D6 functionality that everyone seems to want back, but opens us up for far more advanced features that work well with D7's field system.

Awesome, thanks @joachim and @Jamie Holly. That sounds like a much better direction for me as well. I'd go so far as to move the functionality out of the field module entirely, if possible.