comment forms conversion to formapi _execute model

Patch applies cleanly. I've tried creating/editing/deleting comments, replying to other comments, and futzing around in the admin options screens and haven't run across any problems.

+1, especially as chx tells me this is critical to getting the RC out.

Changing status.

Bumping priority (I already mailed Dries, but still).

Committed to HEAD. Thanks.

Committed to HEAD. Thanks.

I dislike immensely the fact that editing a comment automatically updates its timestamp. In a flat display forum, this would cause the comments to be displayed out of order if one of them got edited (which would be extraordinarily confusing). I must say that I like the strategy (not necessarily the code) taken in http://drupal.org/node/38864 a lot better -- namely if a field doesn't need touching, don't try to touch it.

I see the advantages of modifying the update as follows:

• Security: There's no code path available for a non-priveliged user to accidentally update a field in the database that they don't have permission to -- if the validate function doesn't allow a particular field through, there's no SQL code that even touches that field in the database.
• Performance/Correctness: In the general case, a selective update in form processing would guarantee that a field wouldn't have to be selected out of the database first in order to preserve an existing value. It would also prevent any "professors and chalkboard"-type race conditions from coming up.
• Code Simplicity: Rather than needing to set defaults for all the database fields that are specified as needing updating in the UPDATE query, from all code paths that might approach the query, a "keep this default" is as simple as not adding that field to an array. Code size shrinks.
• Code Maintenance: With the fixed fields in the query, a database schema change absolutely must be reflected in three places -- the code that maintains the schema, the SQL query itself called by db_query, and code that sets defaults for the fields that are referred in the db_query call. With the approach taken in bug 38864, that list will be reduced to one in some cases: the database schema code. In all other cases, it will be reduced to two: the schema code and changes to the code that sets defaults.

For these reasons, I am reopening bug 38864, as I believe that patch should be applied in preference to the approach taken here.

The code maintenance effect can be seen in the current proposed patch in this bug.

According to http://drupal.org/node/38864#comment-56558:

Out of all the $edit indices referred to in the update statement, 'status', 'timestamp', 'format' and 'name' aren't set by default.

It seems that the current patch possibly hasn't addressed 'format' or 'name'. With the patch in http://drupal.org/node/38864
these would get handled automatically.

Some little nitpicking:

+  $output = "<a id=\"comment-form\"></a>\n";

Looks nicer if we do:

+  $output = '<a id="comment-form"></a>';

Please adhere to the Drupal 4.6 behavior.

At Dries's request, I am going to:

> attempt to fix the other outstanding issues that
> I've found with the comment module and submit a patch
> corresponding to the comment.module that's running on my live site.

I'm attaching the patch that I'm currently running with at the moment, in case anyone's interested.

There's some site-specific hacks in the first hunk of the patch (some things not being themeable in hook_block), but if you take those out, the things which have been fixed are:
1) blank subjects get replaced with the first part of the body of the comment when the comment is posted
2) updates to the comment don't touch fields that aren't set in the form. (fixes http://drupal.org/node/38864)
3) #comment-form anchor added back into the page.

The things that I still plan to add to the patch are:

1) I don't know if this patch does "blank subject replacement" for the preview, since I didn't think about it when I created the patch. I'm going to confirm one way or other and fix if necessary.
2) http://drupal.org/node/38944 -- if it turns out to be a problem in comment.module

It would also be nice if these other comment related issues get fixed:

1) http://drupal.org/node/38845 (comments disabled by default -- includes patch)

please do not include site specific stuff.

Current state of my site. Still need to check on the preview thing.

Preview gets subject properly added as well.

I don't think we want to put in another permission for the current release cycle. Can you please make a patch that only fixes the problem at hand?

Really sorry, I do not like the sqlparts thing. It's hard to read. I think a switch statement would be more apt. I do not think there is another thing like that in Drupal core.

As far as the extra permission, I'll advocate for something along those lines as being necessary for drupal's forums to be used on any forum where the site administrator is not acting as the forum moderator, or where there are several forum moderators.

As far as the switch statement, i don't see how a switch statement could get the same effect, without some kind of exponential combination explosion. As far as other examples in Drupal core, I seem to remember user_load() doing something similar.

A proposal:

1. immediately switch this task to "fixed," since the conversion mentioned in the title is actually complete (but has introduced some regression bugs)
2. split the patch (after upgrading it to apply cleanly against HEAD) into as many mini-patches as possible, such that each mini-patch is conceptually coherent.
3. post the url of each new mini-patch issue here for posterity

This allows the mini-patches that people like to get committed quickly, and the other mini-patches that people don't like to languish in the bug system unobtrusively.

Implementing step 1 of comment #25

http://drupal.org/node/40752

http://drupal.org/node/40753

http://drupal.org/node/40754

http://drupal.org/node/40755

http://drupal.org/node/40758

http://drupal.org/node/40760

http://drupal.org/node/40761

http://drupal.org/node/40762

That's it. I also just confirmed that the split up patches, combined, get me all the fixes.