This is a reopening of #125315: Textarea with input format attached, but that issue has already seen a commit.
Let me start by saying: richtext is NOT HTML and is NOT plaintext. The contents of a richtext field make NO sense whatsoever without the associated format. Please read the previous sentence once more and let it sink in.
What we have now is that you can add #text_format => FILTER_FORMAT_DEFAULT to a textarea or textfield and you get a magic new key [elementname]_format in your form values.
The arguments for the current implementation with having an additional property, not an additional #type and implementing it this way were:
- it is easy to go from filtered to non-filtered (by adding another property)
- it does not change the form structure, so that you would look to ['item']['value'] and ['item']['format'] instead of ['item'] itself (tied to 1)
- it allows for single line input fields to have format (eg. short quotes)
My objections to the current implementation are as follows:
If you define a format for ElementX, a magic key appears on the same level as ElementX. WTF! Not only does this require additional hacks #412016: Custom key name for #text_format., it also flies in the face of how we normally create more advanced FAPI widgets and process them into simpler elements.
The argument 1+2 "easy to go from filtered to non-filtered" and vv. is flawed. There's no easy way to do that anyway: you've got to adapt lots of code to properly handle this change: add or remove check_markup, add or remove saving of db fields.
Solution:
A possible solution is to implement the #text_format property so you get an array('text' => ..., 'format' => ...) or a RichText object as value in your submit handler. This is undesirable as it introduces complex behaviour for the same element type depending on a property (a bit similar to selects #multiple, but undesirable nonetheless).
IMO the best solution is to create the richtextfield and richtextarea element types that deliver an array('text' => ..., 'format' => ...) or RichText object as form value.
Comment | File | Size | Author |
---|---|---|---|
#131 | drupal.filter-format.131.patch | 5.02 KB | sun |
#128 | drupal.filter-format.127.patch | 2.81 KB | sun |
#126 | drupal.filter-format.124.patch | 3.69 KB | sun |
#123 | drupal.filter-format.120.patch | 38.33 KB | sun |
#118 | drupal.filter-format.118.patch | 38.39 KB | sun |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedI agree line-by-line with Heine concerns. Moving to a separate FAPI element would:
(1) be cleaner,
(2) remove the ugly belly-dancing that was introduced by #125315: Textarea with input format attached: when you add a #type => 'textarea' with a #text_form, form_process_text_format(): copies the textarea as a child of itself ($element['value']), and add the filter selection widget as a child element ($element['filter']), but mess up parenting so that the $element['value'] will be attributed to $form_state['values'][''], and $element['filter'] to $form_state['values']['_format'],
(3) will allow us to simplify some aspects of #304330: Text format widget.
Comment #2
webchickSubscribing. I've also pinged sun on IRC to take a look at this issue.
Comment #3
catchsubscribing.
Comment #4
sunI agree with the reasoning, but not with the ranting. In fact, #125315: Textarea with input format attached was a corner-stone for considering and using two (thus far separate) elements in a combined fashion. It laid the ground for #304330: Text format widget (which wouldn't have been possible without this initial step) and overall better Wysiwyg support in Drupal. We are able to advance on it now.
Once #304330: Text format widget and #412016: Custom key name for #text_format. are in, let's do the following:
- Introduce a new FAPI #type 'text_format' (or similar)
- Use the #rows property to determine whether we render a textfield or textarea as widget (thus, 1 FAPI element type)
- Use further FAPI properties specific to this #type to allow to customize the arguments for filter_form() and widget display (such as #weight-s).
- Use a #theme_wrapper function to render the text widget, format widget, and form element description (see process function in #304330: Text format widget)
- Make Field API's text field use this widget.
- Cleanup: Move all of that stuff out of form.inc and put it into filter.module. Move core functions from filter.module into common.inc. filter.module is needlessly loaded on all pages currently, but almost never required, because we are caching filtered content.
Comment #5
sunComment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis one is critical.
Comment #7
sunSo here is the battle plan:
1) #304330: Text format widget, required by
2) an option widget validation issue yched mentioned recently in IRC, but also #412016: Custom key name for #text_format., which is required by
3) #375907: Make text field use D7 '#text_format' selector
Work on this issue can start at any time, but I will personally focus on the aforementioned issues first.
After further discussion with Damien in IRC, I think we can't move everything into the theme function of this new #type, because we will still have to ensure proper access checks for the text format using a #process function (see #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use.).
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedSubscribe. This certainly seems like the right way to go.
Thinking aloud, another advantage of going this route is that the current #text_format approach requires code like this to specify the format:
which is not only ugly but also prone to error, especially when it's changed via hook_form_alter (there are security risks to assigning the wrong one).
Presumably, if this were its own form API type, you could instead replace the above code with something like:
There would also be a #default_format property available, although most people would not need to use it. The processing code that decided which format to preselect on the form would then work like this:
1. If #existing_format is set and the user has access to it, preselect that.
2. If #existing_format is set but the user doesn't have access to it, deny access to editing the field, as per #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use..
3. If #existing_format is empty, preselect #default_format (if it's set and the user has access to it), or FILTER_FORMAT_DEFAULT otherwise.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, minor bikeshedding, but I think the name of the new #type would be much better off as something like 'rich text' or 'formatted text' (rather than 'text_format').
Comment #11
Dries CreditAttribution: Dries commentedI've read #412016: Custom key name for #text_format. and #375907: Make text field use D7 '#text_format' selector and a couple of related issues and I agree that this is the right way to go.
I don't think we should go from filtered to non-filtered text -- it seems like it is up to the _developer_ should decide up-front. I'm wondering what the use case is to change the type in mid-air ... given that I can't think of any, this seems like the right way to go.
I'd love to see us prioritize this, but in absence of code, we can go ahead and hack the hack. :-)
Comment #12
Dries CreditAttribution: Dries commentedI'd prefer to see us focus on this patch instead of #375907: Make text field use D7 '#text_format' selector but I'm comfortable committing #375907 because it comes with tests.
Parts of the tests in #375907 are re-usable for the work in this patch, and the parts that fail when this patch lands will tell us what code to undo/redo.
Ultimately, it comes down to timing. If someone volunteers to start work on this patch in the next day or two, I might hold back #375907. If not, I'll proceed with #375907.
Comment #13
sunIt's not about hacking the hack, but about consistency. Working on a patch for this issue is much easier if all of core uses the current implementation consistently. Whether #412016: Custom key name for #text_format. is required or should come first, is certainly dependent on how many other issues are blocked due to it.
Comment #14
cburschkaI support this, and I also agree with David in #10. "richtext" would make more sense than "text_format". Assuming that "richtext" actually means what we think it means, and is not just a Microsoft document format.
Aside from bikeshedding, a bigger design consideration: Can the element receive an array of selectable filters? Will this array intersect with user access control?
Before we get to work on a patch, there should be a code example of setting this element in the form builder, and saving its submitted contents.
Say,
Please adjust to how you think the element should work.
Comment #15
sun#14 goes way too far for D7.
However, we need to clean up the new API feature we introduced.
In a quick IRC discussion, Heine had the frickin' awesome idea to ditch filter_form() and turn it into a #process for this new #type.
A proper #type name is not really clear, but since the UI refers to "text format", the API documentation refers to "text format", the most natural form element #type seems to be 'text_format'.
Comment #16
sunSo this is what I think would make sense.
The patch will fail, because there's a certain amount of direct filter_form() invocations that fatal out. Let's focus on the architectural stuff first.
Comment #17
sunok, seems we have a conflict here. #412016: Custom key name for #text_format. was all about adding a consistent #text_format_parents (or similar) property that Field API could use to assign different #parents for the text format, but NOT for the textarea/textfield.
So it seems we indeed have a name clash here. #format_parents to the rescue?
I'm on crack. Are you, too?
Comment #19
sunImplements #format_parents, defaulting to KEY_format if not defined.
Comment #21
sunoh, wow, Field API makes such patches much smaller than I initially thought. :)
Comment #23
sunComment #25
Heine CreditAttribution: Heine commented- use hook_elements to define the types.
I'd rather have $form['foo'] of such a type result in $form_state['values']['foo'] which is array('text' => 'foobarbaz', 'format' => 1). Yes that would also mean $node->body['text'] and $node->body['format'] (and do check_markup($node->body)?)
Maybe that part is better left for D8? I cannot estimate the amount of change in the current codebase to support this.
Comment #26
sunTried to revamp the patch, but I'm not entirely sure what to do with #process + inheritance of the properties. We can't reset everything to element_info($type_minus_format), because hook_form_alter() wouldn't have an effect anymore.
So it seems that we need inheritance of properties in either case. Next best question is whether 'filter_process_form' should come first or last in #process. My impression was that it should come first, because other #process functions want to work on the respective, injected element instead.
Comment #27
sunIntroducing a new tag for feature freeze: API clean-up.
Comment #28
sunI'm trying to port Wysiwyg module to D7. As of now, I have no idea how to determine/access all elements in a form that may or may not have a #text_format property, so I'm basically back to recursing into the entire form to find all candidates. Not good.
Hence, this is a release blocker.
Comment #29
sunTagging absolutely critical clean-ups for D7. Do not touch this tag.
Comment #30
sun1) This is a release blocker, as stated by the security team lead, and I agree.
2) I'll work on a patch today.
3) I can't promise to get it to fully work today, but I'll try hard.
Comment #31
sunFirst of all, re-rolled that approach against HEAD. CNW, 'cause it will fail equally.
Learned some new rendering stuff in the past days, so I'll try to come up with a sexy solution.
Comment #32
sunBack into business.
Comment #34
sunHEAD was broken.
Comment #36
sunok. I badly need help here.
This patch could be considered complete, if there wasn't this total mess of a text.module + Field API... I debug_backtrace()'d to figure out what exactly is guilty in the past hours, but I still have no clue at all.
Here's what I can provide:
Additionally:
text_field_widget_formatted_text_value()
.text_textarea_with_summary_process()
already gets the malformed values in #default_value, but not in #valuetext_field_widget()
already gets the messed up values in$items[$delta]
, this whole thing must be caused earlier in the process.node_form()
as well asfield_attach_form()
, the values are still correct (IIRC).#array_parents
in filter_process_format() does not help.<rant>
And, btw, we can't release with this mess of a text.module. For one, I have no clue why it doesn't simply use the already existing #types. Secondly, everything happens in #process, so no one except Form API experts will be able to alter those widgets, since there is *nothing* in hook_form_alter() that you could alter. However, that is probably a different issue...
</rant>
Comment #38
bjaspan CreditAttribution: bjaspan commentedsubscribe
Comment #39
sunHrm. Sorry, that error was rather a Form API hickup.
To make any sense of text.module during debugging, I *had to* remove this fake Form API #type along with the strange #process functions.
So this should actually work -- and pass the tests.
I can't think of a cleaner solution. And this really removes a bloat of WTFs.
Comment #41
sunok, this is only failing, because all tests need to be updated to post and check the form values for blocks, comments, nodes, and their respective text formats in the proper locations... that will take some time.
So, before doing that, I need feedback/thumbs-up/confirmation/approval for the taken approach in this patch.
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedI took a look at this patch.
Overall it makes sense and looks very nice, especially with all the code that it removes!
12 files changed, 254 insertions(+), 392 deletions(-)
This isn't a super in-depth review (especially not of the Field API parts), but...
At first glance, I liked your previous suggestion (from comment #4) of using #rows to avoid having two separate elements (or maybe if not that, using some other property to indicate the type) - any reason for not doing that? There are definitely places in the patch that seem pretty odd due to the "_format" convention used here, e.g.:
+ $element['value']['#type'] = strtr($element['#type'], array('_format' => ''));
....Also, the names still sound awkward to me - I think the problem may be that they make 'format' sound like the object, but 'textarea' is the actual object. The best I can come up with is 'formatted textarea' (or 'formatted text' if using one element). It's still not great.
The code comment should explain why we need all submitted form values on one level, because I don't understand why :) I assume it's because the Field UI submit handler expects it that way?
Ideally, the Field UI would take care of pulling array elements out of $form_state['values'] itself, but I have no idea if that's simple or not.
I didn't really understand this comment. Other process functions (for example, checkboxes) set #tree... why can't this one?
I realize the previous code did this too, but I don't understand why it should have to... again, other process functions don't do anything like this, right? As far as I have seen, they just leave the #type alone and expand out the children and everything works OK.
Similarly, couldn't this have been defined in hook_element_info?
With only a few more lines of code here, I think it would be easy to implement what I suggested in #9 (separate keys for #existing_format and #default_format).
I think we should do it, because module developers are going to try to hook_form_alter() this to change the default format, and keeping them separate makes it simple for them to do that without risking security issues.
Similarly, wouldn't it only take a couple more lines of code here to do something like what @Arancaytar suggested in #14? For example:
This is maybe less critical, but it's also something module developers are going to want to do and it also has some security implications if they get it wrong. I think as long as we are changing the API here anyway, it's best to get the API right.
Using array_diff() might be cleaner here?
This function renaming (as well as a few other cleanups in the patch) seems unrelated to the issue. I think it would be best to remove them, especially since we are in code freeze and the patch is already somewhat complex...
Also, if we rename filter_default_format(), I think we would need to similarly rename filter_fallback_format() for consistency?
Comment #43
yched CreditAttribution: yched commentedAbout 'widgets as FAPI #type expanded on process' (re sun #36 - #39) :
That approach was introduced in CCK D6 in an attempt to make field widgets reusable outside CCK (as in webform.module components, custom forms etc...), or to have widgets reuse one another (for instance noderef autocomplete widget reused text.module's textfield widget).
I don't think it really lived to the expectations so far. And the trend from D7 up is now 'make more stuff fieldable' + 'make powerful, Field-agnostic FAPI #types' (e.g this 'text_format', quicksketch's file uploader) rather than 'be able to use powerful Field-specific widgets on non Field stuff'.
I personally wouldn't mind dropping this #type / #process approach and switch back to the simple approach where hook_field_widget() uses existing FAPI types.
As this (CCK D6) article by Karen explains, we're supposed to support both (see her example_widget() function and its PHPdoc at the end of the code) - except that we found in ParisDC that there were bugs in the simple approach; see #567064: Widgets done "the easy way" have too many limitations.
Also, most current core widgets (text, number, list, but not file and image) use that 'expand on #process' pattern; so they would need to be converted too...
Comment #44
sun@David_Rothstein:
On the new #type names: I think the names are good for consistency, because the difference between the "textarea" and "textarea_format" Form API widgets is very clear at first sight. This convention would basically also allow to text-format-enable other Form API #types in the very same way as being done for textfield and textarea. (Not sure whether there are any possible use-cases, but at least, it would be possible.)
I tried several other approaches before the totally simple solution of this patch crossed my mind. First, I tried to revamp the #value_callback function that existed before, which didn't work out, because Form API's input value handling could not be stopped from recursing further into the form (and messing up the already set #values for the widget). Second, I tried to assign hard-coded #parents in the text field widget itself, so filter_process_format() would just add children to the element's parent #parents level, but that really required a lot more explanation.
However, I first tried with the #tree property itself, which worked fine for Field API widgets, but not for implementations like custom blocks. That is, because the custom block body Form API widget does not specify #tree at all - and Form API's form_builder() recurses into all elements in a form and inherits the #tree property for all children from the parent element, if not explicitly defined. Hence, since #tree is not defined, it is inherited as FALSE by default - which means that regular implementations would get 'value' and 'format' at the top level of the submitted form values, instead of the expected location further down the tree.
Therefore, I went with a property #format_tree that is custom for filter_process_format() and specifically tells whether to enable/disable the extra-parenting for the expanded element.
A ugly workaround for that would be to force developers to properly assign #tree => TRUE/FALSE for *_format #types.
Now that you mention it, it may be possible to define #tree = TRUE as default in filter_element_info(). We can try that. :)
Now that you mention it, #type = '*_format' does not output anything, so we might be able...
Developers cannot use hook_form_alter() to change this behavior, because #process runs after form alterion. However, while developing this, I specifically had contributed modules like Better Formats in mind - which would just add another #process function after filter_process_format() via hook_entity_info_alter() to do their customizations. However, it is the responsibility of such a module to not break security (and I'm not even sure they are able to do so, because everything is still validated by Form API and Filter API).
So the approach taken here is KISS.
@yched:
Comment #45
yched CreditAttribution: yched commentedI wouldn't put it as in "@yched bullet 1", because CCK D6 also got rid of 'CCK specific' bits and tricks for existing FAPI stuff, but I basically agree with the rest ;-).
field.form.inc has to massage some of the $field / $instance properties because for instance, for a 'required, multiple' field where we call hook_field_widget() several times, only the first widget should be '#required', and the title (label) and description are displayed by field.module above and below the table and should be left empty by the widgets themselves.
I don't think we want to fake these into the $field and $instance params we provide to hook_field_widget(), this probably needs a separate $options parameter or whetever.
No biggie in itself, other than updating all existing widgets, and the bumps we'll find on the road. Widgets are funky stuff :-(
Comment #46
yched CreditAttribution: yched commentedHere's a proof of concept refactor of field.form.inc and hook_field_widget(), based on patch 39.
We provide hook_field_widget() with a $base array, containing the values to be used for #title, #description, #required.
This applies fine for text widgets as sun refactored them, but will choke on any other field type for now. I'm afraid this same patch will have to convert all existing widgets...
To check that it works, it should be tested with a required, multiple text field.
(Also, note that I couldn't get formatted widgets to actually display with patch #39, so be sure to use an unformatted text)
Comment #48
yched CreditAttribution: yched commentedI took my proposal into a separate patch in #567064-4: Widgets done "the easy way" have too many limitations, which is thus now a prerequisite for this issue in its current (non-#process) approach for the text widgets.
So forget the patch in #46, current state is #39. Sorry for the hijacking.
Comment #49
sun#567064: Widgets done "the easy way" have too many limitations is in. Re-rolled against HEAD.
Due to that, text.module turns into a really nice and clean implementation with this patch.
Working on the other stuff from #44 now.
Comment #50
yched CreditAttribution: yched commentedFYI: #626354: Remove #process pattern from number field starts with a patch to 'simplify' number widgets.
I think we can also ditch the abstraction on 'columns' inside the text widgets. They will be used on text fields, let's not pretend we don't know that
$element['#columns'][1]
is 'value'.Comment #51
sun- Clarified why #tree does not work in an inline comment. (wrong assumption)
- Implemented the array_diff() proposal by David. Nice one.
- Cleaned up quite a lot of the processing in filter_process_form(). Much nicer now.
- Reverted renaming of filter_default_format().
- Moved JavaScript for the text format widget into filter.js.
- Fixed #array_parents processing in form_builder().
- Fixed theme_container() to respect existing #attributes.
I will now proceed to fixing all the tests, which will probably take some hours.
Comment #53
sunSo let's see where we are...
Comment #55
sunAAAAAAAAAH!
Comment #57
sunSo who spots where we did go wrong in this entire #text_format thingy here?
Comment #58
sunSo.
Let me provide some hints for you. :P
So what could be the issue?
I'm on crack. Are you, too?
Comment #59
sunSo where did we go wrong?
I'm a bit embarrased about myself, because I didn't realize this earlier. It made us waste the entire time between #10 and #57, plus countless of hours of trying to make this totally optional egde-case work. :(
Yes, it basically means to go back to D6. But no entirely.
This patch is almost the same as the previous, but without the conditional support for expanding the original element into separate 'value' and 'format' sub-keys in the submitted form values.
So what's about this patch?
Comment #60
yched CreditAttribution: yched commentedWe don't want to do that. For #title, #required and #description, we want to use the properties that were set in the incoming $element passed by Field API to hook_field_widget.
This the reason why in #567064-14: Widgets done "the easy way" have too many limitations I was promoting a $base argument, which you freely use in the $element you build, instead of a 'pre-cooked' $element that you add to. Because more often than not, those properties wlll not be used directly on the $element you build. Most of the time, you'll want to do
Just a variable name stuff, really, but providing those values to hook_field_widget() in a variable named like the form part it's going to return (usually, by convention, '$element') is really misleading - as we see here ;-)
Comment #61
sunRight. Thanks for reminding! So we can remove even more code here :)
The approach we took allows each widget to decide whether $element is $element or $base on its own. Nifty! ;)
Comment #62
yched CreditAttribution: yched commentedHm. Not untrue. I didn't think of that. number widget should do the same, I guess. Might warrant a comment, though ?
My last remark on the patch is that I think we should ditch the
$field_key = $base['#columns'][n]
abstraction, and use the actual column names for text field ('value', 'format', 'summary'). Those widgets will be used for text fields, period. The code is now simple enough that a different field type with different columns that wants similar widgets can just reimplement them.This abstraction IMO only makes sense to keep in options.module widgets (select, radio, checkboxes), since they're more tricky, and are already used by list and taxo fields (and will be used by noderef / userref in contrib). But text.module is the #1 module people look at to build their own widgets, it's best to keep it simple.
This can also be done in a separate task, if needed.
Comment #63
David_Rothstein CreditAttribution: David_Rothstein commentedIt's very, very neat that it's possible to keep 'format' on the same level without hurting the Field API, since that's certainly the 95% use case for a form that would have more than one format-enabled textarea. I would never have thought that was possible :) However, I'm still quite a bit worried about the other 5%. With this patch, if you build a form with two format-enabled elements on the same level, the format of the second will completely clobber the first. It seems like it will be totally not obvious to a module developer how, why or even if it is doing that, or what the method is for fixing it. This clobbering can lead to weird bugs and even security issues (see below).
In addition, we need to consider what happens with hook_form_alter(). Basically, with this patch the recommended procedure would need to be that if you are adding a format-enabled element via hook_form_alter(), you always must put that element inside a structure with #tree = TRUE, because otherwise the auto-generated 'format' key might conflict with that of someone else's, and you can't predict beforehand whether that will happen. Again, this seems quite different from anything else in form API.
I realize tons of work went into trying the previous solution and it caused big difficulties with Field API, but overall it really seems to me that having a form submit handler receive $form_state['values']['something']['value'] and $form_state['values']['something']['format'] is natural because those two things are inextricably linked together as part of 'something' -- even if it does lead to a bit more code in some places for processing $form_state['values'] before putting it in the database. And if we really can't do that, then I think we need some serious documentation somewhere (I'm not sure where?) explaining to module authors exactly how they should be adding format-enabled elements to forms and how to deal with the case of more than one of them.
While I do like the power of being able to format-enable any element type, the part where we express that relationship by magically appending '_format' still makes me uneasy. Since they have to register these things in hook_element_info() anyway, maybe we can make the relationship more explicit rather than relying on magic naming? For example:
Seems to me like it would make the code cleaner and easier to understand if we used an explicit property like '#base_type' to specify this information.
I'm still not a big fan of this name, but I also still don't have an amazing suggestion for improving it :) However, perhaps 'textarea_with_format' would be better? I think this is easier to read, plus it fits the pattern that is being used elsewhere (e.g., 'text_textarea_with_summary').
If I read the code example correctly here, 'some' would not actually appear in the $form_state, because the code above this never set #tree to TRUE...
This is indeed a great change, but it seems the new function is very large and trying to do two totally different things at once:
I think it would be better to split this up into two separate process functions, since modules might reasonably want to totally replace the second one (but probably don't ever want to touch the first).
I still think we should expand this to include the #default_format and #format_options properties discussed in previous comments. Given that this patch is totally replacing the existing API anyway, I don't see it as out of scope for this issue - it seems worth it to make sure we get the new API right.
Really, lots of modules want to introduce this kind of functionality of dynamically changing the list of formats and the default based on a variety of properties (content type, whatever). You said above that they can do it by adding their own #process functions and taking care of security on their own, but hook_form_alter() is generally a lot simpler, and wouldn't we want to provide a clear way to do it that takes care of security for them? (BTW, the security hardening part of this is a bit subtle - it is not that modules would be able to expose formats that the current user doesn't have permission to use, but rather that they would accidentally switch an existing format. For example, someone tries to type in an XSS attack on my site, which does not work because they are only able to save the content as Filtered HTML. Later, I as the site admin go to edit this content, and due to a bug the format selector accidentally defaults to Full HTML. So I go to save the node, without ever having touched the selector and therefore completely not noticing that it has switched on its own, and as soon as the node is saved the XSS attack succeeds.)
Again, it is really only a tiny amount of code we are talking about adding, and I mostly wrote it all above :) Rewriting it a bit for the current code and putting it all together, all we'd need to do is replace the second hunk of the above snippet with something like this:
This tiny bit of code (which would be enhanced via #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use. to make sure to deny access whenever the current user doesn't have permission to use $element['#format']) would mean that modules could feel free to easily set the #default_format and #format_options completely however they want, and easily change them via hook_form_alter() based on whatever criteria they want, with all the security aspects taken care of for them behind the scenes.
Comment #64
sunI really badly need to get some client work done, so my reply will be short.
Please let me know what needs to be done to get this in.
Comment #65
Dries CreditAttribution: Dries commentedThe 5% case does not exist in core. And based on my relatively large knowledge of contributed modules, the case also does not exist elsewhere.
It might not exist because it was either impossible, or a royal pain to date. I can certainly imagine it being possible, especially with CCK in core. Going forward, we'll see bigger and more complex applications being built on Drupal. Maybe I'm missing something, but it doesn't sound crazy to have a form with two textareas that each have input formats. Do you disagree with that sun -- or am I missing something?
Comment #66
sunThe case certainly exists. But not in the way we are foreseeing it here. It was already possible before, and you had to assign special parents for filter_form() to not overwrite the 'format' of another element.
Use-case 1) Multiple fields next to each other. No issue here, because fields use a more complex form structure anyway.
Use-case 2) A field widget containing multiple text format-enabled form elements next to each other. Due to field storage, those need to be in a hierarchy or use special #parents anyway. Sidenote: We'll rather see the rise of a "combofield" or "multigroup" field that allows to add multiple fields as one field to an entity.
Use-case 3) A custom form wanting to use multiple text format-enabled form elements next to each other. As mentioned before, I do not know of any module that needs that, and with the rise of fields in D7, more and more stuff will use text fields. Even if the use-case really exists, it can be done by using a hierarchical form structure or just by assigning custom #parents.
Use-case 4) A form_alter that wants to add a text format-enabled form element to a form structure on a level that already contains a text format-enabled form element. In almost all cases, this very form_alter is required to set custom #parents anyway, so the submitted form values do not end up within the form values that will be processed by the module that "owns" the form and the form values can be properly extracted for custom storage.
As already mentioned in #59, I think that already the current implementation of the #text_format property suffered from trying to make it "also work" for the almost non-existing edge-cases and thereby required us to introduce plenty of workarounds and hacks to make it work for all known cases.
If anyone in here is actually able to point me to a module that would suffer from this, then I'd perhaps reconsider my position. But as of now, I see lots of ugly code that's just a workaround for a problem we introduced earlier.
Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedRegarding points in #64:
2. Yes, documentation of these cases seems like a must, even though they are rare (since they'll be totally unexpected if someone encounters them and it's not documented.) Otherwise, I'll stay out of the decision :)
3. Hm, the patch may not touch the #tree behavior, but #tree is still required in the way I'd expect it to be. I can't decide from your description if you think this is the way it's supposed to work or not, but here's what I tried. First, I put the following code at the bottom of the form builder for custom blocks:
As I expected, the text format selected for this element clobbered the one selected for the block body.
Then I added this line of code:
After that, the block body text format selector worked correctly.
So regardless of what is going on internally with array parents, it seems to me like from a module developer's perspective, #tree is still what you need. (I think this also makes sense, by the way.) @sun, is that the behavior you expected from this code or not?
5. Yes, that is great! A single element that defaults to a #base_type of 'textarea' makes total sense.
6. OK, given that we are now back to finding a name for a single element rather than a bunch of them, that puts us back to where we were at the beginning of this issue, where type names like 'richtext' and 'formatted_text' were floated around. The problem I see with a name like 'text_format' is that a "text format" in Drupal refers to the format itself. But the element you're adding to your form isn't just a text format - it's a text area with a format attached to it. So I think 'text' needs to be the noun here.
Anyway, this can be the last thing we fiddle with before the patch goes in. But my vote right now is '#type' => 'formatted_text'.
8. I actually wasn't necessarily thinking of two separate elements with two separate #types but rather a single element with two separate process functions. So in hook_element_info():
A dedicated theme function does sound like it might work also (and might actually work better). Either way, this would be good, but doesn't seem totally critical to getting the patch committed.
9. The proposed #default_format and #format_options properties are totally optional - no form builder function has to care about them unless it wants to. You can see from the code I wrote in #63 that in the case where an element doesn't specify either of these properties, it reduces to behaving exactly like the code that you already have in the patch.
As far as I can see, without adding these options, modules like Better Formats (and that's not the only one) that want to do this kind of thing actually would need to do it in two separate functions - they need to initially modify #format to set a default before the filter module process function acts (because as soon as the process function moves it away from being NULL they lose context of when it is safe to modify or not), and then they need a separate process function to modify the selector #options after the filter module acts (because the options don't exist until then). And while doing that, they more or less need to duplicate the code I wrote above anyway, if they want to get it right. So why not move these ~10 lines of code into core where they are centralized and let modules safely use and manipulate these as optional element properties just like they normally would anything else?
In most cases I wouldn't continue to push this point (because in theory it could be a followup issue), but for this patch we are going to be breaking the code freeze anyway, so it's important to break it only once and make sure it is right the first time.
Comment #68
sunStraight re-roll against HEAD for now. I'll come back to + answer the last follow-ups, but I'll extract the rather unrelated changes to text.module and form.inc into separate issues + patches first to decrease the size of this patch.
Comment #69
sun@yched: See you in #639428: Remove #process pattern from text field :)
Comment #70
sunAll extracted changes have been committed.
Re-rolled against HEAD.
Comment #73
cburschkaThree conflicts were introduced within two weeks after the patch was uploaded. Two definitely trivial in php.test and filter.module (too much fuzz), and one in common.inc that probably shouldn't be a problem. Here is a resolution - let's see if it passes.
Comment #74
cburschkaWhoops, forgot to diff a new file.
Comment #75
cburschkaWhat is it with me?
Now I somehow managed to drop the changeset in text.module.
Comment #76
sunok. I already lost hope in trying to solve this, but finally was able to come up with a solution (and fix for form.inc).
This is the result of a discussion with Damien in IRC some time ago. While my arguments raised earlier in this issue still hold true, I can see the general sense and reasoning for expanding a form element into two proper child elements 'value' and 'format', and to submit those values accordingly in separate 'value' and 'format' sub-keys.
Now, this patch is the compromise of both, the theoretical and security-wise correct case of having separate 'value' and 'format' keys, and the practical reality of not having (and knowing) any implementation that would want those form values to appear as sub-keys of the original element.
Therefore, this patch sets a #value_callback for both child elements by default. Those default #value_callbacks make the submitted form values appear next to each other on the same level, as mentioned earlier in this issue. They solely work off from the assigned #parents of the original element. So if your module needs multiple text format-enabled elements on the same level in the form structure, then you can simply assign different #parents. Alternatively, you could remove the #value_callbacks and do your own, custom processing (or nothing at all). All of that is also explained in PHPDoc.
This means that values in $form_state['input'] are not touched. Only values in $form_state['values'] are altered, so that form submit handlers will get them. Example from the PHPDoc:
This patch also incorporates the #base_type suggestion we discussed earlier.
I hope we can agree on this one. It simplifies a lot, while still keeping sanity.
Comment #77
chx CreditAttribution: chx commentedWhile a proper review is not in my time frame right now I am OK with the form.inc changes. You might call that a bugfix actually.
Comment #79
sunQuickly discussed the form API change in form_builder() with chx. We investigated whether this code shouldn't live directly in form_set_value(), but no, it seems to be right location, because we only want to conditionally skip the form_set_value() during automated $form_state['values'] processing in form_builder(), resp. input handling. Manual invocations (overrides) of form_set_value() should still be able to reset values in $form_state['values'].
Comment #80
sunoh my, requires update of all tests that happen to post a value to text format-enabled form elements somewhere. :-(
First pass.
Comment #82
sunThis is a major challenge. Submitting a node form with an empty body doesn't work yet, and I don't really understand why. It works properly if body is non-empty, but not if not.
Comment #84
JacobSingh CreditAttribution: JacobSingh commentedI didn't find the error submitting an empty node body, but the patch needs a re-roll due to comment changes.
Comment #85
sunRe-rolled against HEAD + comment body as field.
Comment #87
RobLoachI like it!
Comment #88
cburschkaCheck the quotes and the syntax highlighting. You'll get an unexpected T_STRING error.
You need to lose the quotes around the first 'value'.
Comment #89
cburschkaLet's try this one.
Comment #90
sunComment #92
cburschkaI forgot to diff the new filter.js, but apparently that wasn't the only problem...
Comment #93
sunRe-rolled against HEAD (node title field changes).
Comment #95
sunThis is required to fix #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use.
Comment #96
chx CreditAttribution: chx commentedPoor issue. [value][value]? We can live without that. Accidentally, it allows removing some code and changes too.
Comment #98
chx CreditAttribution: chx commentedComment #100
chx CreditAttribution: chx commentedI have tweaked the after build function.
Comment #102
yched CreditAttribution: yched commenteddebug :-)
We should put the previously defined $main_widget['#type'] as $element['#base_type'].
Text Fields can use a 'textfield' widget and still hold formatted text.
Powered by Dreditor.
Comment #103
chx CreditAttribution: chx commentedIncorporated yched's changes and added some fixes so some of the tests failed now pass for me.
Comment #105
chx CreditAttribution: chx commentedHm, it seems vocabularies have textfields as descriptions they are not textareas... Edit: they are not without format, that's what matters.
Comment #106
chx CreditAttribution: chx commentedWrong patch, sorry.
Comment #107
chx CreditAttribution: chx commentedIf I can make this pass then I guess it's time to assign to myself.
Comment #108
chx CreditAttribution: chx commentedRemoved debug from index.php, removed commented out code from filter.module and tried to modernize a bit the comments on filter_process_form.
Comment #109
sun.core CreditAttribution: sun.core commented#108: drupal-text-format.patch queued for re-testing.
Comment #111
sunRe-rolled against HEAD. This works great. Thank you very much for stepping in and finding the proper way to implement this, chx!
Comment #112
webchickCan someone please summarize (or point me to the summary) on why this extremely, extremely late API change is a release-blocker and doesn't belong with D8?
Comment #114
sunI'll look into that exception now.
Comment #115
sunThat exception was caused by some improper testing code, which must have been introduced elsewhere since chx's last patch. Fixed that test, back to RTBC.
Comment #116
webchickExcellent. Thanks for the great summary. I agree with the case for this being a D7 patch.
Upon closer inspection, it looks like the majority of this is just moving code from form.inc to filter.module, and doing some name swapping. The one that has me the most eeked is the fact that body is now not a text element, but an array of 'value' and 'format' (as opposed to body and body_format). I think that's going to be a god-send for WYSIWYG editors, but will throw a wrench at people who have already started porting their modules and writing SimpleTests. But the extra security/predictability this brings seems worth the trade-off.
The one thing I'm not clear on is why "filter_format" was the name of the text element chosen. "text_format" is much clearer that this will be a *text* field with a *format* underneath it. I would never guess from reading the code that "filter_format" gave me what I was looking for. I guess we're trying to name-space it or something, but we call text fields 'text' fields, not 'system_text' fields, so this feels unnecessary.
Can we swap the name? Then this is good to go for me. I also asked Damien to take one last look.
Comment #117
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is a really nice - and required - cleanup.
Frankly, I'm still not sure why we are moving the values higher up in the $form_state['values'] array (ie. why the form structure is different from the values structure), but if both chx and tha_sun are confortable with it, I can live with that.
Comment #118
sunIn-patch rename of #type 'filter_format' to 'text_format' (also: theme_text_format_wrapper())
Comment #119
sunTests passed, back to RTBC.
Comment #120
webchickAwesome. Thanks for the quick turn-around.
Committed to HEAD. Thanks!
This change needs documenting in the module upgrade guide. Please do so ASAP, so that the docs are in place in time for alpha 3/beta 1.
Comment #121
David_Rothstein CreditAttribution: David_Rothstein commentedThis looks mostly good - although I don't claim to understand every bit of the Form API stuff in here - but I definitely (still) share Damien's concern. In particular, it looks like in order to make things slightly more convenient for certain form submit handlers, i.e., in order to make it more likely that someone can do this:
without having to massage $form_state['values'] beforehand, we are:
And if they don't figure out what that means or how to do that correctly, the result will be some nasty, hard to track down critical bugs, not to mention security issues.
So essentially, I know it's been discussed a lot above, but it still really seems unclear why that proposed tradeoff is a good one?
****
Minor things:
This should use element_properties() rather than explicitly checking for '#'.
Maybe I missed it, but is this used anywhere?
I found this sentence a little confusing... when it says "no custom processing", it really means no custom processing including the custom #after_build that the current function adds, right?
Comment #122
David_Rothstein CreditAttribution: David_Rothstein commentedCrosspost with the commit, but above comments still stand.
Comment #123
sunFinal tweaks: Removed trailing-whitespace and stale #value_callback.
Comment #124
webchickDoes this address David's comments in #121?
Comment #125
webchickHm. Doesn't look like it.
Comment #126
sun1) Re-rolled against HEAD.
2) We forgot to add filter.js before commit.
3) Incorporated David's feedback.
--
@David:
# Adding a somewhat complicated-looking #after_build function to the filter module.
That #after_build function is quite trivial. Very much simpler than the previous form_process_format() munging to get the value into $key_format.
# Requiring that anyone who potentially wants to hook_form_alter() a text format onto an existing form either always use a #tree structure (I think?) or else read and completely understand this.
No, #tree is applied automatically by filter_process_format(). By default, the (now) current default behavior is identical to D6. For your special use-case, you had to pass an additional $parents array in D6. In D7, you can either define custom #parents (same as passing $parents in D6), or alternatively, remove the #after_build.
Comment #127
webchickD'oh! My bad. filter.js is committed now. Can we get a quick re-roll of #126?
Comment #128
sunfilter.js was added.
Comment #129
sunRemoved old #text_format docs, added new #type 'text_format' docs: http://drupal.org/update/modules/6/7#text_format
Comment #130
David_Rothstein CreditAttribution: David_Rothstein commented#128 looks good.
***
D6 had issues too but I think the difference is that there your code would break entirely if you didn't use $parents, but here it appears to work for your module, but breaks others. (Plus we have a chance to correct it.)
For example, how many people can spot what is wrong with this D7 code, and why it causes the block module to break in a critical way (and possibly introduces a security issue)?
Also, by #tree I meant using that on a wrapper, not on the element itself. For example, if the above code is put in a 'mymodule_wrapper' element with #tree set to TRUE, then it no longer causes problems. Maybe at the very least we should be explicitly recommending that people use that when altering forms to add these kinds of elements.
Comment #131
sunPlus JS behavior fixes I found and fixed in #91663-96: Permission of text format is not checked when editing an entity and instead reset to something a user can use.
Comment #132
webchickCommitted #131 to HEAD. Since this is documented (yay! thank you!!), I think I can mark this fixed now.
David, I asked sun about your concerns, which sound quite valid to me. He said that the behaviour D7 is currently doing is the same as D6. So could we move your concerns to a separate issue about further hardening this code, and cross-reference here?
Comment #133
David_Rothstein CreditAttribution: David_Rothstein commentedI created #735662: hook_form_alter() can easily clobber a text format
Comment #134
effulgentsia CreditAttribution: effulgentsia commentedPlease see #335035-89: drupalPost() incorrectly submits input for disabled elements for a fix needed as a result of this change.
Comment #136
Gábor HojtsyThe upgrade docs at http://drupal.org/update/modules/6/7#default-text-formats are still seem to be outdated.
Comment #137
catchComment #138
catch.
Comment #139
hass CreditAttribution: hass commentedThere seems to be a bug in this new form element implementation, see #820816: #type text_format not compatible with system_settings_form($automatic_defaults = TRUE), please.
Comment #140
sunUpdated http://drupal.org/update/modules/6/7#text_format