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

CommentFileSizeAuthor
#7 drupal_render.patch676 bytesriccardoR
#6 form-tag.jpg81.49 KBdries
#3 issue-326527.patch706 byteslilou

Comments

lilou’s picture

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">
lilou’s picture

Status: Active » Needs review
StatusFileSize
new706 bytes

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

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Needs review
dries’s picture

StatusFileSize
new81.49 KB

Well, it seems like on my copy, the form-tag is opened twice. See attached screenshot.

riccardoR’s picture

Title: form id="node-form" declared twice » form id="node-form" declared twice
StatusFileSize
new676 bytes

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:

    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 :

    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.

      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.

wrwrwr’s picture

No sure what the implications of unsetting #type are, but the form is opened just once and the page validates with the patch.

dries’s picture

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.

dries’s picture

Status: Needs review » Fixed

All tests pass. Committed to CVS HEAD. Thanks.

catch’s picture

XHTML validation is an interesting idea - started a new feature request against simpletest here #355185: Add HTML validation to functional tests

Status: Fixed » Closed (fixed)

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