| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | UI Change |
Issue Summary
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.
Comments
#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: Make nodes have no body field by default. Remove deprecated APIs for body field
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!
#39
Any update on where this stands? It's important.
#40
This patch will conflict with #553306: Make nodes have no body field by default. Remove deprecated APIs for body field, it would be best to wait after that other one lands.
Other than that:
+++ modules/node/content_types.inc 21 Aug 2009 06:09:27 -0000@@ -186,13 +186,17 @@
+ $instance = field_info_instance('body', $type->type);
The signature of the function has changed meanwhile. Sould be field_info_instance('node', 'body', $type->type)
This review is powered by Dreditor.
#41
subscribe / bump
#42
#553306: Make nodes have no body field by default. Remove deprecated APIs for body field is blocked on #571654: Revert node titles as fields, so maybe this patch should go forward now.
Most probably needs a reroll though.
#43
Anyone?
#44
#45
This still conflicts with the body UI patch. However I've rerolled everything which doesn't conflict for now. Only @todo would be applying the teaser length variable to the instance settings in the upgrade path, between node_update_7004() and node_update_7005() that's a bit of a mess - at the moment we create per content type variablesin 7004, then we'd delete them all in 7005.
I'm tempted to suggest we commit this without the upgrade path, then do the upgrade path fixes in #553306: Make nodes have no body field by default. Remove deprecated APIs for body field - will look at re-rolling that patch again now.
#46
The last submitted patch, summary_length.patch, failed testing.
#47
Should be a formatter setting, not an instance setting.
#48
inherently, this is a formatter setting, trouble is there"s currently no UI for formatter settings. No good UI model could be found so far... :-(
#49
The last submitted patch, summary_length.patch, failed testing.
#50
Tests pass with this one.
We could have no UI for this until there's one for formatters. Or we could mimic the D6 behaviour with a single variable for summary length and have that be checked when setting the 600 default. I think that's part of "remove redundant node body UI" though rather than this patch. If we need to fix it here, then we probably need to merge the issues.
#51
To summarize, the issue here is that we have per node-type variables for teaser length, but you can attach this field to any entity type/bundle. However I'm downgrading this from critical, just a normal bug IMO.
#52
Formatters are what it's displayed on the "Manage Display" page?
#53
Sorry, only loosely following currently - if we don't read from the 'teaser_length_[node_type]' variable any more, shouldn't this affect the UI as well ?
#54
@yched: I'm assuming we sort out the UI issue in #553306: Make nodes have no body field by default. Remove deprecated APIs for body field which also has an up-to-date patch.
#55
Note : Work-in-progress in #796658: UI for field formatter settings, which would be the "right" fix for this thread.
#56
Here is a small fix as I pointed in #796658-14: UI for field formatter settings
'text_trimmed' - size for bundle (node type) was ignored see 'text_summary_or_trimmed' implementation.
$size = variable_get('teaser_length_' . $instance['bundle'], 600);should be moved out of loop#57
So , just to summarize :
#50 should be spot on after #796658: UI for field formatter settings lands - patch over there should be ready now, just waits for someone to push the RTBC button ;-)
Still todo here :
- Remove the 'Length of trimmed content' input on the content type edit page (in the 'Display settings' vertical tab)
- Implement hook_field_formatter_settings_form() and hook_field_formatter_settings_summary() for text fields (current patch in #796658: UI for field formatter settings includes that as an example, but that code should be removed before it gets in, this belongs to this issue).
This includes deciding on the input type. Currently the 'content type edit' form uses a dropdown select (with 10 options from 200 to 2000 chars by increments of 200). A free textfield input would allow more flexibility.
Ideally, it would be a select dropdown with predefined lengths + a 'custom length' option that shows a textfield for custom length input.
But #states don't work in ajax-updated subforms until #768128: ajax callbacks can't apply #attached is solved - which is blocked on the infamous #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework.
- modify node_update_7006() so that the 'body' instances are created with teaser display using the legacy length from 'teaser_length_[node_type]' variables (and drop those afterwards)
- + provide a HEAD2HEAD patch
This counts as a UI change, so we should get #796658: UI for field formatter settings and this one in before beta.
#58
#796658: UI for field formatter settings is in !
Any taker ?
#59
Summary:
- Adds field formatter settings for 'text_trimmed' and 'text_summary_or_trimmed' formatters
- Removes 'teaser_length' settings for node types
- Adds upgrade path for 'teaser_length' variable in node_update_7011()
- Fixes a test
PS: I dont know how to set 'trim_length' setting with drupalPost() so simply updates instance. Suppose tests for UI should be done in #796658: UI for field formatter settings
#60
Update incorporated into node_update_7006() as proposed #57
#61
Previous patch was wrong, missed hunk from text.module
#62
+++ modules/node/node.install 18 Jul 2010 06:17:22 -0000@@ -496,10 +496,26 @@ function node_update_7006(&$sandbox) {
node_add_body_field($node_type, $node_type->body_label);
+ $trim_length = variable_get('teaser_length_' . $node_type->type, $default_trim_length);
+ $instance = field_info_instance('node', 'body', $node_type->type);
+ $instance_changed = FALSE;
+ foreach ($instance['display'] as $view_mode => $view_mode_info) {
+ if ($view_mode_info['type'] == 'text_trimmed' || $view_mode_info['type'] == 'text_summary_or_trimmed') {
+ if (!isset($view_mode_info['settings']['trim_length'])) {
+ $instance['display'][$view_mode]['settings']['trim_length'] = $trim_length;
+ $instance_changed = TRUE;
+ }
+ }
+ }
+ if ($instance_changed) {
+ field_update_instance($instance);
+ }
+ variable_del('teaser_length_' . $node_type->type);
it's a bit sad that we have to create the field instance and then update it just after that, but that's how the update is structured, and we don't want to have node_add_body_field() handle special update-only code to handle trim_length.
Can we just add a code comment above this block ?
Also, it seems we should also drop the 'teaser_length' variable at the end ?
+++ modules/node/node.test 18 Jul 2010 06:17:22 -0000@@ -565,11 +565,11 @@ class SummaryLengthTestCase extends Drup
+ $instance = field_info_instance('node', 'body', $node->type);
+ $instance['display']['teaser']['settings']['trim_length'] = 200;
+ field_update_instance($instance);
Without JS, the 'Manage display' form is a regular multistep form, so several drupalPost() with NULL as $path for the non-1st steps should work.
However, agreed that testing the UI is not in the scope of this specific test, so it's OK to API-update the instance instead.
I plan to add a full-fledged testing suite for Field UI's 'Manage display' side, which is currently under tested (if at all).
So : aside from the minor 'remove teaser_length variable' item, this looks RTBC to me.
44 critical left. Go review some!
#63
Added returning of $instance to node_add_body_field()
Added comments about conversion and specially about teaser_length which user in aggregator upgrade.
See also: node_update_7004() and aggregator_update_7001()
PS: need help to make comment better!
#64
#65
+++ modules/field/modules/text/text.module 18 Jul 2010 19:14:56 -0000@@ -260,11 +261,51 @@ function text_field_formatter_info() {
+function text_field_formatter_settings_form($field, $instance, $view_mode) {
@yched: I thought we'd pass $form + $form_state now? Did we miss that?
+++ modules/field/modules/text/text.module 18 Jul 2010 19:14:56 -0000@@ -260,11 +261,51 @@ function text_field_formatter_info() {
+ if ($display['type'] == 'text_trimmed' || $display['type'] == 'text_summary_or_trimmed') {
Idea: Could we strpos($display['type'], '_trimmed') !== FALSE here to make this field formatter setting form element re-usable for other formatters?
+++ modules/field/modules/text/text.module 18 Jul 2010 19:14:56 -0000@@ -260,11 +261,51 @@ function text_field_formatter_info() {
+ '#title' => t('Length'),
"Maximum length"?
+++ modules/field/modules/text/text.module 18 Jul 2010 19:14:56 -0000@@ -260,11 +261,51 @@ function text_field_formatter_info() {
+ '#size' => 20,
20 is a bit large for a text field that accepts a number, no?
Rest of the patch looks good to me.
Powered by Dreditor.
#66
Fixed. Screens + patch
- strpos() is great idea
- Maximum length for setting, same string for field settings already
- Formatter now uses format_plural because it has a actual data
- Suppose 10 is enough
#67
- Hm, I hate to nitpick, but #66 is misleading : for the 'summary or trimmed' formatter, the summary will be used if present, else we trim the body. The summary can have an arbitrary length, so stating 'Maximum length: n chars' or 'Trimmed to n chars' is wrong. This is the length applied when trimming, so I'd go for 'Trim length; n chars' (if that's correct english ?)
- strpos($formatter_name, '_trimmed') : why not, but it doesn't make a real difference in the end; text_field_formatter_settings_form() is only called for formatters defined by text.module (like most field-widget-formatter ops, hook_field_formatter_settings_form() is not an actual hook but rather a callback, general conceptual flaw in Field API). Can stay as is, though.
- still todo : delete the 'teaser_length' variable in the update ?
- as sun noticed, the full signature of hook_field_formatter_settings_form() includes $form and $form_state. This was added late in #796658: UI for field formatter settings, and the dummy implementations in the demo patch over there probably overlooked those.
#68
Re-roll with
- used 'Trim length' and 'Trim length' .': '. settings - suppose better to have short summary and less strings to translate
- used strpos() - code looks shorter :)
- fixed function suignature for $form and $form_state
there's no ability to remove 'teaser_length' variable because it's value used in aggregator upgrade path as pointed in #63
#69
'teaser_length' variable : OK, missed that comment, sorry.
+++ modules/field/modules/text/text.module 24 Jul 2010 01:24:44 -0000@@ -273,23 +314,24 @@ function text_field_formatter_view($enti
+ $size = isset($display['settings']['trim_length']) ? $display['settings']['trim_length'] : 600;
the isset is not needed, $display['settings']['trim_length'] will always be set.
(2 occurrences)
Other than that, this looks RTBC.
38 critical left. Go review some!
#70
Oops, CNW was what I meant.
#71
Re-roll without checking
$display['settings']['trim_length'], let's see errors|notices if something goes wrong!#72
Field API does take care of putting everything in place to avoid modules dealing with field, widget, formatter settings to litter their code with isset()s.
If it fails doing so, then it's the sign that something is wrong, and we are better off not hiding the warnings under the carpet :-).
Tanks, andypost !
#73
Sorry, but I'm bumping this to 'critical'.
It is the last WTF regarding "body as field'. The current 'Length of trimmed content' form input in the 'node type' edit form is way too harsh (affects the display of all 'trimmed / summary_or_trimmed' formatters on all text fields in all view modes), and irrelevant for text fields on non-node entities, which are currently all blocked to 'trim to 600 chars'.
It's only now that #796658: UI for field formatter settings is in that we're able to fix this in a sane way, and we don't want the patch to be turned down later because of a UI change (removes the 'Length of trimmed content' form input in the 'node type' edit form, moves it as a formatter setting on the 'Manage display' screen).
#57 has a patch summary.
#74
No API change, BTW.
#75
Looks like an important improvement. Code looks good so committed to CVS HEAD. Thanks.
#76
Thks Dries.
Created an issue for HEAD2HEAD: #877064: #504564 - Make summary length behave with fields
#77
Automatically closed -- issue fixed for 2 weeks with no activity.
#78
hi,
does this fix the problem with trimming the rss feeds content in core D7?
right now in core D7 there is no way of trimming the feeds
thanks,
njardim