comment_form() structure is inconsistent / $form['#token'] WTF

Sorry, silly typo. Also fixed tests accordingly.

I forgot to mention that this patch also brings consistency into node forms and comment forms. With this patch, both forms use $form['author'] as container for content author fields and (always) $form['author']['name'] as the username field (which is the most important part of this patch).

@Dries: $is_admin and $anonymous_contact denote entirely different things. The latter holds the configuration value of the setting whether anonymous users may post their contact information. (on that note, the constant names are a bit wonky)

This patch fixes the failing tests.

+++ modules/comment/comment.module	27 Nov 2009 20:28:56 -0000
@@ -1891,12 +1839,8 @@ function comment_form($form, &$form_stat
-  $form['#token'] = 'comment' . $comment->nid . (isset($comment->pid) ? $comment->pid : '');

I have no idea what this is or was for.

I need someone to fill in and replace the @todos in this patch.

Although dww just mentioned in IRC that he thinks that no form should define a custom #token, I absolutely don't want to be guilty for introducing a security issue. Hence, changing priority and component accordingly.

Also note that http://api.drupal.org/api/function/contact_site_form/7 equally defines a custom #token. I asked chx and Heine to chime in here.

Until we have background information on Form API's proper usage of #token, let's proceed with this:

Users with "administer comments" should always see the Save button when editing an existing comment.

yay! Information! :)

So now I could write the docs for when to use a custom #token and when to not use one. However, when is it legit to do #token = FALSE?

Additionally, since we now support block caching (e.g. per role) for auth users, and blocks can contain forms, I guess the same rule that applies for anon cached pages applies to such forms in blocks?

chx rtbc'ed the patch in #9, which just always displays the "Save" button on comment forms for users having the "administer comments" permission. There's no need to require them to (p)review their edits.

I will do a follow-up patch to revert the removal of #token in comment_form anyway though.

I started to incorporate the gained knowledge into the code and write docs, but...

WTF? I stepped through the logic for #token all over again, and in the end, I also checked the actual output of current forms in HEAD.

There is no 'token' element in any form for anonymous users.

I thought I would have understood it, but now I'm even more confused than before... :-/

Attached patch contains the same logic for #token as before, but using a more grokable logic and most importantly more comments. Also contains the comment preview button patch from #9.

Of course, the custom tokens assigned in comment.module and contact.module make no sense with that.

+++ includes/form.inc	29 Nov 2009 02:59:46 -0000
@@ -666,14 +666,21 @@ function drupal_prepare_form($form_id, &
+  // @todo isset($user->uid) ?!
   elseif (isset($user->uid) && $user->uid && !$form_state['programmed']) {

Aha! Now I understand it :P

This review is powered by Dreditor.

Alright, instead of assuming it should work, we want to test and ensure that it works. :)

I expect this patch to fail, because I cannot get page caching for anon users to work in HEAD.

If it turns out that it will fail, then this needs to be fixed elsewhere.

More related to the patch: Since we're now removing the last two instances of #token in core, I don't think that there is any use-case anywhere for setting a custom token.

um. That passed. Not sure why I cannot get page caching to work on my local dev site.

This one hardens the caching assertion. But tinkering some more about this, I don't think that the situation from 2005 can happen today, because Form API doesn't allow to post the form with 'op' = 'Post comment' (or whatever) in any case.

So, actually, I don't think that the test makes sense in any way.

Should we drop it?

I'm trying to wrap my head around this suggestion, but I don't see how I could write such a test, because:

  protected function drupalPost($path, $edit, $submit, array $options = array(), array $headers = array()) {
...
        // We post only if we managed to handle every field in edit and the
        // submit button matches.
        if (!$edit && $submit_matches) {

Removed the previously added comment test for now.

Fixed a stale comment.

Further debugging revealed something else, which is going to be tackled on the security list. I think that this patch is fine to commit though.

For D8, we can finally remove the inconsistent form element for the comment author name:
#1866978: Leverage new #input facility of #type 'item' for author name in comment form