remove raw $_REQUEST from comment.module [#93425]

comment.module sets $form['#attributes']['destination'] = $_REQUEST['destination']. This results in a 'destination' attribute in the HTML form tag (theme_form puts it there). The attribute is not defined for the form tag and therefore results non-compliant XHTML.

Also, AFAIK, the attribute has no effect. Removing it does not prevent the destination query parameter from working, e.g., from admin/content/comment's edit links.

Patch attached.

Comment	File	Size	Author
#7	comment-request-destination-5.x-93425-7.patch	757 bytes	bjaspan
#7
#4	comment-request-destination-93425-4.patch	750 bytes	bjaspan
#4
#3	comment_request.patch	705 bytes	catch
#3
	comment-patch.txt	676 bytes	bjaspan

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 7 November 2006 at 16:52

Status:

Needs review

» Needs work

Good catch... but $_REQUEST['destination'] indicates to where the form should redirect once it's submitted. Therefore, I think this should change to $form['#redirect'] = $_REQUEST['destination']..,.

Comment #2

bjaspan CreditAttribution: bjaspan commented 24 January 2007 at 04:35

Version:	x.y.z	» 6.x-dev
Status:	Needs work	» Needs review

The offending line was added in comment.module:1.379 (http://cvs.drupal.org/viewcvs/drupal/drupal/modules/comment/comment.modu...). It seems to have been part of one of the initial Forms API patches: http://drupal.org/node/29465.

The old code

$destination = $_REQUEST['destination'] ? 'destination='. $_REQUEST['destination'] : '';
return theme('box', $title, form($form, 'post', url('comment/reply/'. $edit['nid'], $destination)));

was replaced with

if ($_REQUEST['destination']) {
  $form[attributes]['destination'] = $_REQUEST['destination'];
}

I never worked with 4.6 but I looked into form.inc from that era. The 4th arg was $attributes and got inserted into the <form> tag. So, the old code generated <form ... destination=some/path> which, it turns out, is exactly what we are getting now: invalid XHTML that has no effect. So the patch accurated preserved the bug. :-)

So, $_REQUEST['destination'] has not had any effect on this form since before 4.6. Putting it in $form['#redirect'] now would be change of semantics. Furthermore, why should this one form set #redirect = $destination? If that is correct behavior, the FAPI should do it, no?

Conclusion: I still think the code should be removed for 6.x-dev, 5.0, and 4.7.