Download & Extend

form id="node-form" declared twice

Project:Drupal core
Version:7.x-dev
Component:node system
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed (fixed)

Issue Summary

<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.

Comments

#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

Status:active» needs review

Attached patch, rename form id="node-form" to id="node-form-article".

AttachmentSizeStatusTest resultOperations
issue-326527.patch706 bytesIdleFailed on MySQL 5.0 ISAM, with: 14,745 pass(es), 51 fail(s), and 19 exception(es).View details

#4

Status:needs review» needs work

The last submitted patch failed testing.

#5

Status:needs work» needs review

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.

AttachmentSizeStatusTest resultOperations
form-tag.jpg81.49 KBIgnored: Check issue status.NoneNone

#7

Title:form id="node-form" declared twice» form id="node-form" declared twice

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.

AttachmentSizeStatusTest resultOperations
drupal_render.patch676 bytesIdleUnable to apply patch drupal_render_19.patchView details

#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

Status:needs review» fixed

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

Status:fixed» closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.