Posted by David Lesieur on October 14, 2008 at 5:27am
Jump to:
| Project: | Weight |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Ah well... I know it is always better to split multiple changes in multiple smaller patches. However, I needed this immediately, so here's a patch that includes multiple small improvements:
- In DB Setup queries, only update nodes of appropriate types (instead of updating all nodes). This was previously marked as TODO in the code.
- Implemented proper form submit functions (instead of checking $_POST['op']).
- Removed useless intermediate menu callback functions (now calling drupal_get_form() directly as a callback).
- Renamed the internal functions to be prefixed by _weight to follow common Drupal practice.
- Use db_placeholders() for sanitizing node type names passed to db_query().
- In weight_old_nodes(), use a placeholder in t() instead of prepending the message with the node count.
- Avoid showing the submit buttons in the DB Setup forms when there are no nodes to update anyway.
- The Weight selector is added in the Publishing options fieldset rather than in its own fieldset. Who needs yet another fieldset in that form? Also, in some setups it was appearing in the midst of CCK fields.
- Changed the weight selector's description. The previous description seemed a bit too specific and unlike the descriptions we usually find in core.
- Updated some comments.
Let me know what you think of this. ;-)
| Attachment | Size |
|---|---|
| weight-cleanup.patch | 13.05 KB |
Comments
#1
See also #318625: PHP notices.
#2
What is the rationale for moving the fieldset to the bottom of the Publishing options fieldset?
I specifically called the field "Node Weight" because some modules (e.g. WebLinks and FAQ) implement a "Weight" field of their own.
#3
I have applied this to 6.x only. If someone wants to try to back-port this, please submit a patch.
#4
I have just checked and if someone doesn't have "administer nodes" they don't get the Workflow options, and therefore no weights.
#5
#6
Oops, right, I had overlooked that permission issue.
Yet the idea was that weight is really a publishing detail and, at least to me, does not deserve being given more emphasis than the other publishing options by having its own fieldset.
Also, as I had reported, depending on how your node types are setup, the Weight fieldset could appear in the midst of CCK fields, at an illogical position.
If the Publishing options fieldset is to be the right place for the weight option as I suggest, then perhaps Weight could create that fieldset when it's not already present in the node form. That way people without the Administer nodes permission would still get the weight option.
#7
I put it back like it was for now. I'll have to think about how I'd like to do this. To me, this field is just as important as the "Input Format" so it should be up there, but I can see your point too. Maybe it should be a setting.
#8
Yes, both ways are valid. However, a setting might not be worth the effort since one can already re-arrange the form in a custom module through hook_form_alter(). I guess we'll never cover all the use cases, so it's probably better to keep things simple. Personally, I wouldn't mind you marked the issue as 'fixed'.
#9
It took longer to test than to write. Please test it out when you get a chance.
#10
Automatically closed -- issue fixed for 2 weeks with no activity.