support for comment_driven

2010-03-20_WYSIWYG_support_cdriven_format[unix].patch

explanation:
comment_driven has one 'format' already in use for the node's body, and also 'signature_format' if they are enabled
therefore, for the comment's body it uses 'cdriven-format' instead,
which contains everything that WYSIWYG might need to handle a proper format

without the patch: http://drupal.org/files/issues/cdriven1-working-but-just-node-body.jpg
with the patch: http://drupal.org/files/issues/cdriven2-body-AND-comment.jpg

EDIT: link to patch

My first reaction to this is "why can't comment_driven namespace the fields they use by wrapping them in another FAPI layer?". That way they should not need to alter the keys used by the fields and it would make Wysiwyg work out of the box. I don't know the inner workings of comment_driven (hadn't even heard of it before today) so I don't know how much work a change like that would require though.

If we 'give in' to module-specific changes like this, we'd have to start tracking every module out there which uses a key other than "format".
In D6, the only reliable way we've got so far for determining what is a "format selector" is the FAPI key, following Core's example. signature_format is a Core exception we won't need in D7 because FAPI now explicitly holds the information we need.

@#2 comment_driven module attempts to display a node_form within a comment_form without altering the node_form, and since both forms need to co-exist and both have a 'format' key it chooses to change the one in the comment_form

it seems to me a pretty fair case to deserve an exception the same way signature did
the long story can be read at #747616: Compatibility with WYSIWYG

as you can check over there, that module made the best it could to be closer to what WYSIWYG sets as requirements
but the parent key which wraps the format can't be renamed

you can recognize there is an strict limitation of WYSIWYG to demand how the parent key of the FAPI elements it needs should be named

I did my best, and the structure within the parent key complies with your requirements,
but can't change the parent key, otherwise it wouldn't be possible to merge node_form+comment_form

what if there is a need to have several formats on the same level of a form?
(which is precisely this case)
I think it wouldn't be so hard to have at least a variable (even without admin UI) to hold the list of allowed keys
then you wouldn't even have a request for each module using distinct format keys or multiple formats at the same level
instead it would be responsability of those modules to tell their users how to set up the module or write automated functions to do it for them

If one follows http://drupal.org/node/358316 as many format enabled fields as you need can be used in the same form.

I'm waiting for Sun to chime in on this, he's got more FAPI experience than me.

UPDATE: Fixed the link.

If one follows http://drupal.org/node/358296 as many format enabled fields as you need can be used in the same form.

many formats in the same form, yes.
many formats in the same level of a form, no. (unless I'm missing something)
$form_state['values']['format'] is taken by a node's body
therefore, can't use the same $form_state['values']['format'] to hold another format's value,
that would require having that value more nested or named differently
(for instance it could be nested within a #tree=TRUE)

but in this case, I don't build/create the involved forms,
I just merge a couple of existing forms and make them work as well as possible.
to avoid breaking the forms, I can't change the nesting levels of their elements
therefore, some elements are renamed to avoid having name clashes at the same level, and guide both forms through their respective FAPI workflow to make them work (i.e. the renamed elements get to their validation/submission handlers with their original names)
but WYSIWYG needs to recognize the format structure in advance (i.e. for presentation, before processing the form)

if this is unclear or not enough and can provide further details
but I hope you'll find not so hurting to provide an alternative mechanism to let other modules work around this issue
the simplest way could be considering punctual support requests (if backed up by a reasonable limitation like signatures), but a more generic mechanism could also be implemented

From my point of view (again, I've not yet had time to go through the code of the module in question so I might have missed something) I don't see why it's not possible to wrap the entire form which you wish to merge into another form within a new FAPI "level". That way, there should never be any risk of any form element keys colliding, for the format or otherwise. Then when processing the form, you extract everything from within the wrapper and process it as a separate form.

Please don't take this as a "We won't do anything at all about this or don't care about integration with other modules". I just need some time to go through the code to get a better overview of things before making a decision. And Sun also has a say about this of course, given he created Wysiwyg to begin with. ;)

why it's not possible to wrap the entire form [...] within a new FAPI "level".

because 3rd party modules expect to operate on those elements by means of form_alter, after_build, process, validation, submission, etc
the first time I get those elements is in form_alter, and then I can't touch them again until validation,
therefore, after_build should keep working for third parties (for instance)
really there are several kinds of tweaks that I rather not follow on per case basis

even comment_driven wants to wrap the whole node_form within a collapsible fieldset and it is done by JS/jQuery on the client side, because 3rd party won't find the elements being beneath another level
(without going too far, take for example core's taxonomy, elements are expected to be under $form['taxonomy']/$form_state['values']['taxonomy'] whether they are tags or not, and every 3rd party module will go to look for them over there, for instance by means of a form_set_value)

moreover AHAH elements place/replace partial structures into the form,
for instance, CCK's add_more button reads the cached form, changes it, and then re-stores it, without passing the form through its whole processing (i.e. no more traditional form_alter/after_build/etc once it is asynchronous)

once the validation/submission stage is reached, I can take over control again and conduct elements submission separately, i.e. giving them back their original names

Please don't take this as [...]

there is no problem, questioning the flow of comment_driven is definitely ON the table

I see, thanks for the explanation, that will be very helpful when I have time to dig into the code.
EDIT: Noticed I posted the wrong link earlier, updated my previous post.

I posted the wrong link earlier

I was already aware of "Developer info", which is referenced at #747616: Compatibility with WYSIWYG

according to #5 this is waiting for sun's review

I have the feeling that you are mistaking the form structure in $form with the submitted form values that end up in $form_state['values']

wysiwyg.module contains the special hack for user signatures, since we cannot change/fix User module in core. Any other module should be able to use 'format' as internal key in the form structure; optionally passing alternative parents to use for the submitted form values. See the last @param of http://api.drupal.org/api/function/filter_form/6

Most likely this means that this issue won't fix, and comment_driven needs to be changed instead.

I have the feeling that you are mistaking the form structure in $form with the submitted form values that end up in $form_state['values']

there is only one $form_state['values']['format']
if two $form's elements are named 'format' and neither has #tree above them both will be submitted as $form_state['values']['format'] (unless I'm missing something)

wysiwyg.module contains the special hack for user signatures, since we cannot change/fix User module in core.

understandable

Any other module should be able to use 'format' as internal key in the form structure; optionally passing alternative parents

alternative parents is what I don't have here, since I don't create the forms, they already exists, this module only makes them coexist, but at submission time both formats end up at their right $form_state['values']['format'] but at different times
to achieve this, at some point they are named differently, WYSIWYG eagerly expect them to be 'format' when they are not, and later they are submitted separately and they are switched back (but the rich editor didn't show up)

Most likely this means that this issue won't fix, and comment_driven needs to be changed instead.

anyway, I'll have to review it again, since it was three month ago the last time I checked
but at the time I convinced myself that it wouldn't be possible to do otherwise
(and really, it is not too much to ask for a special case consideration, IMO)

I don't create the forms, they already exists, this module only makes them coexist, but at submission time both formats end up at their right $form_state['values']['format'] but at different times to achieve this, at some point they are named differently, WYSIWYG eagerly expect them to be 'format' when they are not, and later they are submitted separately and they are switched back (but the rich editor didn't show up)

I have no idea what comment_driven is doing, but regardless of how it works: if you are merging two forms and properly processing them, then you need to have different #parents for each form, so as to not clutter form values of one form with the other. However, neither #parents and #tree are of any importance for Wysiwyg, as we are testing the internal form array keys. Or in other words: the key name in #array_parents is what matters for Wysiwyg's form processing.

Especially if you are merging pre-existing forms, then both forms likely use $form['format'] = filter_form(...);. It is this 'format' array key name that matters, not the #tree/#parents property or the optional argument to filter_form().

Also note that Wysiwyg's form processing is much simplified in D7 and no longer relies on any special key names.

lets see:
- from node_body_field I'm getting $form['format'] = filter_form($node->format); (which will be submitted as $form_state['values']['format'])
- and from comment_form I'm getting $form['comment_filter']['format'] = filter_form($edit['format']); (which also would be submitted as $form_state['values']['format'], since $form['comment_filter'] is not #tree)

then, to avoid name clashes (in $form_state['values']) the comment form's elements are renamed, that's why the appearance of $form_state['values']['cdriven-format'] since it is the equivalent of $form[...]['cdriven-format'] = filter_form(..., ..., 'cdriven-format');

therefore WYSIWYG stops working because it assumes that the key will be always 'format' which is only the default value of filter_form for its third parameter, not a requirement,
then functionality is constrained by wysiwyg_process_form

      // filter_form() always uses the key 'format'. We need a type-agnostic
      // match to prevent false positives. Also, there must have been at least
      // one element on this level.
      if (($item === 'format' || $item === 'signature_format') && $index > 0) {

From http://api.drupal.org/api/function/node_body_field/6, you are getting:

$form['format']

From http://api.drupal.org/api/function/comment_form/6, you are getting:

$form['comment_filter']['format']

Changing the internal array keys makes no sense and has no effect, since #tree is FALSE for both forms. Therefore, you need to adjust the #parents in

$form['comment_filter']['format']['#parents']

respectively, the actual form elements below that key, so as to change the key of the submitted form value into 'cdriven-format'. The internal form structure and array keys have nothing to do with this; the internal array key can be kept as 'format', as you only need to change the resulting key of the submitted form value.

when I said it was "equivalent" to $form[...]['cdriven-format'] = filter_form(..., ..., 'cdriven-format'); I meant it
#parents are being properly adjusted to the "renamed" elements

The internal form structure and array keys have nothing to do with this; the internal array key can be kept as 'format', as you only need to change the resulting key of the submitted form value.

... hourglass ....

I hope this can be resolved soon, not being able to use the editor is not fun.

I haven't found the time to review this
I think that sun's proposal should work, but I don't feel comfortable with violating #parents/#array_parents conventions, because other contrib modules might rely on them to navigate the forms
thus, I would like to review this without rush

@arhak: Do you believe that I would recommend an implementation that violates something?

Alternating #parents are common, a built-in Form API feature since Drupal 4.7.x. That is also why http://api.drupal.org/api/function/filter_form/6 makes it so easy to pass custom #parents via the $parents argument.

Also, for the sake of completeness: #array_parents cannot be altered. Form API declares this property and it is not supposed to be altered by anyone.

Do you believe that I would recommend an implementation that violates something?

no, I wouldn't think that at all

Alternating #parents are common ...

indeed, but it is done to point to an ancestor element (at a higher level)
I don't recall seeing #parents pointing to non-existent elements

I might be missing something, but as I read it, it would end up being something equivalent to
$form[...]['format'] = filter_form(..., ..., 'cdriven-format');
so the element's key would be actually 'format' but the #parents array would point to something else ('cdriven-format' in this case) which will yield a $form_state['values']['cdriven-format'] and everyone should be happy then
but, isn't there any trouble with having #parents pointing to a non-existent element key?

Custom #parents are cool. You might want to consider to key your special values hierarchically under a top-level key "cdriven" for extra clarity and cleanliness.

well, then I'll have to refactor the forms merging process

just the last questions
- instead of renaming form elements should I preserve (all) their original keys and tweak #parents only?
- tweaking the #parents should be done after_build? (if it is intended to resemble their original #parents)

Getting a bit too detailed now - I'm not familiar with cdriven module. Only know how Subform module did it in the past. And of course, how Multiform module will do it in the future. ;)

You likely won't be able to retain both $form structures, because they are overlapping (uid, status, etc.). So the merged-in form likely needs both form structure keys and their parents to be manipulated. The form structure keys likely just need a new dedicated top-level parent only. More or less the same applies to #parents, but of course, you can put them anywhere you want - as long as you put the submitted form values back into the locations where form validation and submit handlers are expecting them, prior to invoking them.

Yeah, that's the process I had in mind in #7, and how I thought comment driven did it first.
Somehting like:

comment-form
 |-title
 |-name
 |-email
 |-body
 L-format

+

node-form (to be driven)
 |-title
 |-taxonomy
 |-body
 |-format
 |-author
 L-workflow

=

comment-form
 |-driven-form-wrapper // All #parents below here are prepended with 'cdriven' or similar.
 | |-[escaped form-level #validate and #submit handlers etc]
 | L-node-form
 |   |-node-form
 |   |-title
 |   |-taxonomy
 |   |-body
 |   |-format
 |   |-author
 |   L-workflow
 |
 |-title
 |-name
 |-email
 |-body
 L-format

The comment's #validate and/or #submit handlers rip out everything below $form->comment-form->driven-form-wrapper to a new $form, unescape and restore #parents/handlers, extract values from $form_state['values']['cdriven'], and run just those parts through the form processing functions, or maybe invoke handlers manually.

Makes sense?

The form structure keys likely just need a new dedicated top-level parent only. More or less the same applies to #parents, but of course, you can put them anywhere you want

almost what is currently done, one of the forms get its elements under a dedicated top-level wrapper
but #parents are being moved as the result of the elements being renamed (to avoid making the top-level a #tree, which would be recursively inherited by nested elements)

with your proposal, they don't need to be renamed at all,
just move their #parents under a dedicated top-level

as long as you put the submitted form values back into the locations where form validation and submit handlers are expecting them, prior to invoking them.

yes, the unmasking functions are taking place in their right time

the renaming approach was taken to leave FAPI build up #parents which wouldn't collide
that's why I was asking if the insertion of the new top-level for #parents should take place #after_build
because it is form_builder the one populating #parents

@#27 actually it is the other way around: the node_form is the one that preserves its original structure and the comment_form the one being masked (since it is the lighter/simpler form)

yes, #after_build sounds like the right place then.

thank you very much sun & TwoD