Remove access checks from comment_form_submit()

+1. Also breaks drupal_form_submit(), when called from say a cron job as anonymous user.

First solve and discuss in this issue http://drupal.org/node/687586 and then come back here to apply the same solution.

Created patch with solution from issue http://drupal.org/node/687586

What exactly would we be testing for here? If it's removing the access controls checks, how would the result to the user appear differently based on the patch? Or would we check via some log file?

Thanks for the patch! Here's a patch review with some code style pointers:

+++ b/modules/comment/comment.moduleundefined
@@ -1852,6 +1852,17 @@ function comment_form($form, &$form_state, $comment) {
+  $form['administer_comments'] = array(
+     '#type' => 'value',
+     '#value' => $is_admin,
+  );
+  $form['post_comments'] = array(
+       '#type' => 'value',
+       '#value' => user_access('post comments'),

Is it better to use $form_state or $form here?

+++ b/modules/comment/comment.moduleundefined
@@ -2201,8 +2212,13 @@ function comment_form_submit_build_comment($form, &$form_state) {
+  ¶

Trailing whitespace.

+++ b/modules/comment/comment.moduleundefined
@@ -2201,8 +2212,13 @@ function comment_form_submit_build_comment($form, &$form_state) {
+  // Get the premissions for the user from the passed values in the form builder

Misspelling and a missing period.

+++ b/modules/comment/comment.moduleundefined
@@ -2201,8 +2212,13 @@ function comment_form_submit_build_comment($form, &$form_state) {
+  ¶

Trailing whitespace.

+++ b/modules/comment/comment.moduleundefined
@@ -2216,7 +2232,8 @@ function comment_form_submit($form, &$form_state) {
+    	// check if the user can administer comments ¶

Indentation is off; trailing whitespace; needs capitalization and a period. I'd suggest: "Check whether the user can administer comments."

7 days to next Drupal core point release.

Minor changes based on feedback from #9. All tests passed.

Minor: Whitespaces.

+  ¶
+  // Get the permissions for the user from the passed values in the form builder.¶
+  $admin = $form_state['values']['administer_comments'];¶
+  $post = $form_state['values']['post_comments'];¶
+  ¶

Reroll without whitespaces.

The patch looks fine to me, it solves the problem. Some minor things though:

+++ b/modules/comment/comment.module
@@ -1852,6 +1852,17 @@ function comment_form($form, &$form_state, $comment) {
+  $form['administer_comments'] = array(
+     '#type' => 'value',
+     '#value' => $is_admin,
+  );
+  $form['post_comments'] = array(
+       '#type' => 'value',
+       '#value' => user_access('post comments'),
+  );

Too much indentation.

+++ b/modules/comment/comment.module
@@ -2201,8 +2212,13 @@ function comment_form_submit_build_comment($form, &$form_state) {
+  // Get the permissions for the user from the passed values in the form builder.
+  $admin = $form_state['values']['administer_comments'];
+  $post = $form_state['values']['post_comments'];

I'd like to have a little bit more descriptive variable names here. $is_admin is used in other places in core. So $can_post should be fine too, yes?

0 days to next Drupal core point release.

We can continue the work here:

Review this patch with the same approach: #687586: Remove access check from user_register_submit()
Roll a similar patch: #687588: Remove access check from submit() in UserCancelForm

If this breaks hook_form_alter() or drupal_form_submit(), it should be possible to write a test for that.

Also if this is something we need to apply lots of places (and presumably contrib is doing the same thing wrong), is there somewhere central that could be documented?

I guess the way that this "breaks" hook_form_alter() is if a contrib module alters in another submit handler and doesn't do the equivalent checks (which might be a valid use case)?

This patch will only make it easier to do those checks by adding the result of the permission checks to values in $form_state. It doesn't fix anything else, does it?

So, does it really make sense to add tests for that -- the fact that it'll be easier to do a form alter the right way?

So, my conclusion is that we don't need tests for this, no? It doesn't need tests because it's generally just a cleanup.

Hm. Maybe if we had Object FAPI we could test it properly. So no tests then?

Tagging as novice for the task of rerolling the Drupal 8.x patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Reroll

Thanks!

#24: topic-687584-24.patch queued for re-testing.

Should be fixed in #2227503: Apply formatters and widgets to Comment base fields