Node module does not respect weight for all form items [#296918]

The node.module builds the Author Information, Publishing Options and Submit buttons outside of the normal form building process. This prevents using hook_form_alter() and weights to affect the placement of these items on the page. Further it prevents anyone from adding things such as markup text after the submit buttons should they choose to do so.

theme_node_function() (node.pages.inc @2202) has the following comment.

  // Admin form fields and submit buttons must be rendered first, because
  // they need to go to the bottom of the form, and so should not be part of
  // the catch-all call to drupal_render().

In user testing there does not seem to be a detrimental effect. This patch also re-weights these items to maintain the traditional page flow using weights of 90, 95 and 100 respectively.

Comment	File	Size	Author
#1	296918-1-node.form_.patch	2.26 KB	jbrauer
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jbrauer CreditAttribution: jbrauer commented 18 August 2008 at 20:45

Status:

Active

» Needs review

File	Size
296918-1-node.form_.patch	2.26 KB

Comment #2

catch

he/him

English

CreditAttribution: catch commented 19 August 2008 at 00:13

I have two HEADS held up waiting for fresh dtbng benchmarks, but visually this looks great (yay, less code), and I can't imagine why this was ever there in the first place.

Having said that, do we need the rest of the stuff in theme_node_form() any more, what about just killing it altogether?

Comment #3

Anonymous (not verified) CreditAttribution: Anonymous commented 19 August 2008 at 18:03

bookmark

Comment #4

Dries CreditAttribution: Dries commented 20 August 2008 at 15:53

Applying this patch doesn't seem to alter the form (visually) at all. Is that correct? If so, looks like a good clean-up.

Comment #5

kscheirer

English

Vallejo

CreditAttribution: kscheirer commented 20 August 2008 at 19:07

patch works and applies cleanly.

Visually there are no changes. The HTML structure is altered somewhat though. Doesn't seem like a big change to me, but could this change require some themes to be updated?

Pre-patch most of the node form is in a <div class="standard">, the "Authoring Information" and "Publishing Options" fieldsets are in a <div class="admin"> and the buttons are not in either.

Post-patch the node form, all fieldsets, and the buttons are in <div class="standard">

Comment #6

jbrauer CreditAttribution: jbrauer commented 20 August 2008 at 23:54

@Dries that is correct

@kscheirer is correct in that it does eliminate the class "admin" on the publishing options section. This could be added if it is seen as critical but I couldn't find where it's used in system, default or node css files. In the use cases I've had I've had need to override this anyway if it is being used for theming.

Comment #7

catch

he/him

English

CreditAttribution: catch commented 21 August 2008 at 19:55

I looked again, and I really can't see any reason for theme_node_form() now. No particular need to wrap it in a div given the entire form has an id.

Comment #8

floretan CreditAttribution: floretan commented 21 August 2008 at 21:09

Status:

Needs review

» Reviewed & tested by the community

Tested this too, and I can confirm the above comments.

The code looks good, so I'm marking this as RTBC.

The HTML changes probably won't affect contrib themes, but they might affect custom themes with specific overrides, so we should mention the change in the "theme 6.x to 7.x upgrade guide" once this gets committed.

Comment #9

jbrauer CreditAttribution: jbrauer commented 21 August 2008 at 22:12

I did an alternative that removes the theme_node_form() function.

There is a minor consequence with this (and indeed if we go this route we should take the corresponding .node-form classes out of node.css or find a different approach to utilizing them.

.node-form .form-text {
  display: block;
  width: 95%;
}
.node-form .container-inline .form-text {
  display: inline;
  width: auto;
}
.node-form .standard {
  clear: both;
}
.node-form textarea {
  display: block;
  width: 95%;
}
.node-form .attachments fieldset {
  float: none;
  display: block;
}

The visual effect of this is to have the fields default to 50% wide (approx) and while the other elements don't affect display in Garland they would have more impact on other themes that potentially count on them being there. So with that it seems that perhaps keeping the minimal function is the better path?

Comment #10

catch

he/him

English

CreditAttribution: catch commented 21 August 2008 at 23:13

We could make all those .node-form into #node-form maybe? However I don't think that should hold up this patch, so tested it as well, and leaving at rtbc.