form id="node-form" declared twice
alpritt - October 27, 2008 - 04:44
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
<form action="/node/add/article" accept-charset="UTF-8" method="post" id="node-form"> appears twice in node edit forms, producing invalid markup. When previewing a node, the error does not exist.
#2
I confirm, node-form appear twice (id & class) :
<form action="/drupal/7.x/node/add/article" accept-charset="UTF-8" method="post" id="node-form"><div>
<div class="node-form">
<div class="standard">
#3
Attached patch, rename form
id="node-form"toid="node-form-article".#4
The last submitted patch failed testing.
#5
Similar issue : #276653: Duplicate element ID renders invalid HTML
#6
Well, it seems like on my copy, the form-tag is opened twice. See attached screenshot.
#7
The form-tag is opened twice actually. This also happens in many (maybe all) forms rendered with a theming function defined in $form['#theme'].
See for example admin/content/taxonomy and admin/user/user (two duplicate forms here).
This doesn't happen on D6.8
The difference to HEAD is in drupal_render() :
On D6 the variables #type, #prefix and #suffix are saved and unset before calling the custom #theme function - which ends calling drupal_render() again - and restored afterwards.
drupal_render() itself takes care of those items after all other have been processed.
Well, the properties are saved and restored on HEAD as well, but #type is no longer unset in the meanwhile.
In drupal_render() - D6.8 we have:
<?php
if (isset($elements['#theme']) && empty($elements['#theme_used'])) {
$elements['#theme_used'] = TRUE;
$previous = array();
foreach (array('#value', '#type', '#prefix', '#suffix') as $key) {
$previous[$key] = isset($elements[$key]) ? $elements[$key] : NULL;
}
// If we rendered a single element, then we will skip the renderer.
if (empty($children)) {
$elements['#printed'] = TRUE;
}
else {
$elements['#value'] = '';
}
$elements['#type'] = 'markup'; // NOTE : this is functionally equivalent to unset($elements['#type'])
unset($elements['#prefix'], $elements['#suffix']);
$content = theme($elements['#theme'], $elements);
foreach (array('#value', '#type', '#prefix', '#suffix') as $key) {
$elements[$key] = isset($previous[$key]) ? $previous[$key] : NULL;
}
}
?>
while in D7-HEAD we have :
<?php
if (isset($elements['#theme']) && empty($elements['#theme_used'])) {
$elements['#theme_used'] = TRUE;
$previous = array();
foreach (array('#type', '#prefix', '#suffix') as $key) {
$previous[$key] = isset($elements[$key]) ? $elements[$key] : NULL;
}
// If we rendered a single element, then we will skip the renderer.
if (empty($children)) {
$elements['#printed'] = TRUE;
}
else {
$elements['#markup'] = '';
}
// NOTE : #type is not unset
unset($elements['#prefix'], $elements['#suffix']);
$content = theme($elements['#theme'], $elements);
foreach (array('#type', '#prefix', '#suffix') as $key) {
$elements[$key] = isset($previous[$key]) ? $previous[$key] : NULL;
}
}
?>
This was changed here: http://drupal.org/node/252013 (removed save, unset, restore of all three variables)
and partially restored here: http://drupal.org/node/295506 (except the unset of the #type variable, probably because the problem was less noticeable)
IMO it makes sense to unset the #type property as it is already saved and restored, and it is rendered correctly at the end of drupal_render() as in D6.
<?php
unset($elements['#type'], $elements['#prefix'], $elements['#suffix']);
$content = theme($elements['#theme'], $elements);
foreach (array('#type', '#prefix', '#suffix') as $key) {
$elements[$key] = isset($previous[$key]) ? $previous[$key] : NULL;
}
?>
The attached patch fixes all duplicate form-tags I was able to find and test, and apparently it has no side effects.
#8
No sure what the implications of unsetting #type are, but the form is opened just once and the page validates with the patch.
#9
I'm wondering if we could add a XHTML validation step to D7's test framework so that every page that is tested automatically gets validated ...
Either way, I'm running all the tests with this patch, to see if they succeed.
#10
All tests pass. Committed to CVS HEAD. Thanks.
#11
XHTML validation is an interesting idea - started a new feature request against simpletest here #355185: Add XHTML validation to simpletest
#12
Automatically closed -- issue fixed for two weeks with no activity.