Posted by sun on November 21, 2009 at 11:02pm
5 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | task |
| Priority: | critical |
| Assigned: | sun |
| Status: | closed (fixed) |
Issue Summary
Same as #626354: Remove #process pattern from number field and others.
Extracted from #414424: Introduce Form API #type 'text_format'.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal.text-field-process.0.patch | 15.57 KB | Idle | Failed on MySQL 5.0 ISAM, with: 15,210 pass(es), 6 fail(s), and 8 exception(es). | View details |
Comments
#1
Probably wiser to turn this into a separate patch after all...
Will review asap.
#2
Contains debugging code, because the value of 'format' isn't carried over in node previews.
#3
Fixed it. :) This one is ready to fly.
#4
Looks like we can drop text_theme() now?
#5
Nice catch!
#6
The last submitted patch failed testing.
#7
2 testfailures. Once those are cleared, this is RTBC.
#8
As I wrote back in #414424: Introduce Form API #type 'text_format', I think we should get rid of the $field_key = $base['#columns'][n] abstraction, and use the actual column names for text field ('value', 'format', 'summary').
This abstraction only makes sense in options.module widgets, since they're more tricky (*god* they are). The code here is now simple enough that a different field type with different columns that wants similar widgets can just reimplement their own. Those widgets here will be used for text fields, period. Text.module is the #1 people look for sample code, let's keep it simple.
I have an updated patch ready if we agree on this ;-)
Other than that, minor remark:
+++ modules/field/modules/text/text.module 22 Nov 2009 00:14:21 -0000@@ -594,140 +583,14 @@ function text_field_widget_error($elemen
+function text_field_widget_formatted_text_value(&$element, $edit = FALSE, &$form_state) {
Not sure why we take $element by reference ?
We don't need it, and that doesn't seem standard for form.inc's _value callbacks either ?
Also, all other widgets now use a form_set_value() in a validate callback for these sorts of things (turning widget values into field values).
Hopefully #414424: Introduce Form API #type 'text_format' will get rid of this anyway, but shouldn't we be consistent meanwhile ?
I'm on crack. Are you, too?
#9
Test failures: previously text.module displayed 'Full text' as the title of the textarea. Bug introduced by the 'JS show/hide summary' patch. This patch rightfully changes that to display the field label, but then node.test lines 915 and 949 need to be changed from
$this->assertRaw('Full text', t('Body field was found.'));to
$this->assertRaw('Body', t('Body field was found.'));[edit: er, 'Baz' for line 949, actually...]
#10
Implemented all of the remaining suggestions and fixes.
#11
rerolled after #645850: Remove unused varaibles in text module. I'm surprised that the bot didn't report stale patch yet.
+ minor changes.
- removed unneeded !empty() around $instance['settings']['text_processing']
- added missing
'#rows' => $instance['widget']['settings']['rows'],'#rows' => $instance['widget']['settings']['summary_rows'],
- displays summary only for 'text_textarea_with_summary' widget.
That widget might very well be extraneous and get actually merged with 'text_textarea', but removing it doesn't seem to be in the intended scope of sun's patch. That's for a followup patch to remove it properly. This patch just cleans up implementation in a feature-constant way.
Other than that, RTBC after bot comes back green.
#12
Green.
#13
Committed to HEAD. Thanks!
Is this the last of them? :)
#14
Yes, that seemed to be the last of this series! :)
YAY! for SANE FIELD WIDGETS!
Thanks, yched! I'd say that was a very nice peer-review action :)
#15
@webchick: yup, last widgets. Options module has a large followup though: #639466: hook_options_list() + XSS filtering
Er, now let's break formatters :-p ? See #438570: Field API conversion leads to lots of expensive function calls, followups #52, #55, #61.
@sun: 't was indeed ! w00t.
#16
Automatically closed -- issue fixed for 2 weeks with no activity.