Make summary length behave with fields
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Needs usability review |
Now the trimmed text is not saved in the database.
This is not necessarily bad IMO, as I'd like the body to be trimmed on render if a summary is not set (meaning that you don't need to re-submit each node when you change the teaser length). The problem is that the text is not correctly trimmed on render either, but instead it's always trimmed to 600, or whatever the general teaser length is, ignoring the setting of the content type (teaser_length_$type).
Also a minor problem is that on content type edit page "Length of trimmed post" will always display 600 because the value is hard coded (it should be variable_get instead).
I didn't attach a patch cause I would like to discuss/know if the summary should be saved as trimmed if no summary is set or if we change the behavior and trim the text on render.
#1
Patch provided.
@catch told me in IRC that the trim should be done on render. Given that this patch does the following:
1. The default value for teaser length is "remembered", both on admin/build/node-type/type page and inside node_configure (but I can't find where that page is "hidden" now).
2. Changed theme_field_formatter_text_summary_or_trimmed so it passes the size to text_summary.
3. Changed the text so it's accurate because now if you change the teaser length it will affect old nodes too.
I'm not happy with how the size is passed to text_summary and I'm open to ideas.
Where is the old post settings page gone to? I remember it should be a local task or such, but I can't find it. If it doesn't exists then we need a follow up issue to add it back or remove the node_configure function cause it's not used anywhere as far as I can see.
#2
Since is desired behavior to not save the summary if no text is in the summary field, I change the title so it better describes the problem (I hope).
#3
Should we have some tests for this? :-)
#4
I think we should. I'll try to read some documentation about how to write a test after I submit a new patch for alert to ui.dialog issue.
#5
While working to write this test I noticed a strange thing. In the html of the simpletest browser there is no closing p tag for the summary, while that tag exists in a real browser, but if I changed the body to a shorter string the closing p tag was added.
Back on topic. This really needs a review cause I don't know if the test is strong enough.
I don't know if a test is needed to check if the value displayed for teaser length is the right one.
On a side note, this test should have been put in place way before this issue. :)
#6
I think we're close. Given a good re-roll, I see myself committing the next version of this patch. Thanks for working on the tests!
A couple of comments left:
- The help texts seems to be off -- copy-paste error from other test?
- getInfo() seems to have unrelated texts too.
- The $raw variable is a bit odd and prone to errors (with the spacing). I wonder if we can do something that is a bit more robust to theme changes etc. Is there a word that shouldn't appear after the 200 characters mark? If so, we could simply check the existence of that specific word.
#7
Yes, the test I did initially also tested the view of the page (because the PageView test actually test access to the edit page), but then I decided to keep the test only for the summary length but forgot about the description.
That was my point in saying that the test is not very strong. And a very good suggestion.
Patch with suggested modifications.
#8
works!
#9
Committed to CVS HEAD. Thanks!
#10
Sorry, didn't have the time to look at this earlier.
+ $size = variable_get('teaser_length_' . $element['#bundle'], 600);in theme_field_formatter_text_summary_or_trimmed():This seems pretty tied to nodes. Only nodes have teaser_length_[NODE_TYPE] variables right now. Text fields can be applied to any fieldable entity.
If we had no legacy, trim length would be a formatter setting, letting you freely specify different trim lengths in teasers, rss feeds, etc etc...
This is what makes sense, really. In D7, 'trimmed version of a text' are not in any way specific to nodes.
I'm not exactly sure of how we move there from our legacy behaviors, but it might be worth pondering a little. Currently, a 'trimmed' text field on a user is hardcoded to 600 chars, period :-(.
+ whatever we end up with, we should also implement it in theme_field_formatter_text_trimmed, not just theme_field_formatter_text_summary_or_trimmed()
Also, in D7 'trimmed version of a text' is now called 'summary', and the word 'teaser' is dedicated for 'a display mode for an object (a build-mode)'.
The "Body as field" patch specifically avoided messing with variable names, but if do do work around those, we should IMO take the opportunity to rename consistenly, and provide upgrade paths.
#11
@yched
I agree with you, but... for now a problem is solved and it doesn't brakes anything. We can discuss now about how the system should work and I would be happy to do it if I receive some pointers.
I expressed my concern about how the size is passed to text summary. I'm not familiar with the field api yet, so I don't know if any field with summary will have a bundle field in the table, but since the same it's also used above in the same function I thought it's pretty save to assume it will and use it and that make the size to not relate to nodes only.
IMO the size should be passed in the object so you can change it (maybe I want my node to show 300 characters in main content area, 600 in the highlight area and 100 in a custom area), but this is for another issue I think.
I think if we continue in this issue we should rename it so it describes better the new problem.
The trim is done at the size of teaser_length, size you can't change (or I can't find the page to do it), or even if you could, would be for all the text trimmed fields, which is a little restrictive.
#12
At the very least we should update theme_field_formatter_text_trimmed() to use the same code that was just added to theme_field_formatter_text_summary_or_trimmed()
#13
It wouldn't mean nothing. Adding a code that will always return 600 in the current state. It's easy to set a variable teaser_length_$bundle but we need to do more than that IMO.
I'll try to familiarize my self with the field API and see if I can come up with something.
#14
OK here is a first working patch. I think it still needs some work, but as it is now it works.
What it does:
1. It adds a summary_length value inside settings for text_with_summary fields.
2. Make that actually work (the value is passed to text_summary() inside the 2 theme functions - theme_field_formatter_text_trimmed() and theme_field_formatter_text_summary_or_trimmed())
3. Wrapped the summary length setting on the content type edit form in an if so it doesn't display if the content type has no body.
4. Changed $item to $items in a few places inside text_field_sanitize in text.module (it seemed like an error to me to have $item there, but feel free to tell me if I'm wrong).
TODO
Remove the teaser_length variable for good and create an update path.
I would like to be able to pass the length in the build type (so for example we have a rss build with a different summary length and use that on display for rss).
Bumped to critical because now the text_with_summary field type "hardcodes" the summary length and I think this is still a bug.
We still need a better name for the issue.
#15
The last submitted patch failed testing.
#16
This patch moves the summary_length value form the field instance to the formatter (in 'teaser' in node's case). Because of this I had to add the formatter for RSS too.
This means that it removes the todo for build modes, since rss for example can define it's own length (eventually, because right now inherits teaser length).
#17
#18
The last submitted patch failed testing.
#19
Lets see what the bot thinks now.
LE: That's better.
#20
#21
Same patch as before, but with an indentation fix and no killed kittens (I'll open a new issue for that, the fix, not killing kittens).
LE: kittens saved in #509220: text_field_sanitize() error
#22
The last submitted patch failed testing.
#23
I really like the addition of RSS teaser length. Subscribing. In this patch, should we provide an upgrade path for teaser_length to teaser_length_%contenttypes ?
#24
Sorry for letting this slip by, this should definitely happen.
The patch is good Field API-wise, reroll + testers welcome, I'm a little short on time right now :-/
#25
#26
Came in to review this in the LA codesprint. Unsure how to review it as it appears from comment #9 that Dries has already considered the issue fixed and committed a patch to core.
Should the thread and patches beyond comment #9 be a separate issue?
#27
#28
The patch committed was a quick fix that took care of the effect but the cause of it must be solved too.
#29
The last submitted patch failed testing.
#30
#31
Confirming that patch in #30 works. Thanks tic2000
I tested the following.
-Trimmed text. Save node. View node.
-Edit same node again. Trimmed text. Save node. View node.
-Trim Page at
node/add/page-Trim Article at
node/add/article-Length of trimmed posts 200, 600, 2000, Unlimited.
Tested with.
-Drupal 7 (September 9, 2009 - 05:11) fresh install.
-Garland theme
-Firefox 3
-Site OS: Ubuntu Server 8.04 LTS
-End user OS: Windows XP
#32
Thanks for the testing Onopoc
Trimming text works without the patch. This patch adds the code so this could become a field setting and exposed in the ui, and not a special case like it is now.
But to tell you the true I didn't follow the field changes recently, and mainly the field ui ones and I don't know if this is not already in in one form or another.
#33
Thanks for clarifying tic2000. Putting status back to 'needs review'.
How can this be tested?
#34
People familiar with fields can answer that question better than I can. how/if/when :)
#35
The last submitted patch failed testing.
#36
Needs re-roll.
Badly needed patch.
Node Title and and Body are now handled as fields,
yet they are 'special cased' and show on the content type edit page.
They shouldn't appear here at all, they just be fields with widget settings and formatter settings, like every other field.
#37
The issue at
#553306: Remove redundant body and title node attributes & UI
is going to remove the title and Body field setting from the content type admin page, and move them to fields.
#38
+1 I think this is really important. Any update on where this stands? I'm ready to test, provide feedback, and cheer on this work of leveraging Fields API to banish the cruft from Drupal!