PLEASE put form actions in an 'actions' wrapper and give them names. Docs: http://api.drupal.org/api/drupal/developer%21topics%21forms_api_referenc...

Several reasons:

  • The buttons are now combined and weightable together
  • Themes (and the Form system) can position them correctly
  • Extending modules will know what to alter

This is just horrible:

    $form[] = array(
      '#type' => 'submit',
      '#value' => t('Delete'),
      '#validate' => array('nodequeue_edit_queue_form_delete_validate'),
      '#submit' => array('nodequeue_edit_queue_form_delete_submit'),
    );

It doesn't even have a name! How are other modules supposed to hook into it?

Patch attached.

Files: 
CommentFileSizeAuthor
#10 1442928.patch1.53 KBSebCorbin
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#3 form-element-names.patch1.23 KBrudiedirkx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form-element-names.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
form-buttons.patch815 bytesrudiedirkx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form-buttons.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

rudiedirkx’s picture

Almost the same goes for the operations/actions links in nodequeue_view_queues(). If you'd give them names, other modules could do something with them.

    if (user_access('administer nodequeue')) {
      $operations[] = l(t('Edit'), "admin/structure/nodequeue/$queue->qid/edit");
      $operations[] = l(t('Delete'), "admin/structure/nodequeue/$queue->qid/delete");
    }
amateescu’s picture

Status:Active» Needs work

Ha, good catch about the missing names :) Can you provide a patch for operations as well?

rudiedirkx’s picture

StatusFileSize
new1.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form-element-names.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ah man =)

Okay, here it is.

rudiedirkx’s picture

Actually there's another location this would be good...

      $form['nodes'][$node->nid]['edit'] = array('#markup' => l(t('edit'), 'node/' . $node->nid . '/edit', array('attributes' => array('title' => t('Edit this node')))));

You can write that without the markup element so that modules can influence it:

      $form['nodes'][$node->nid]['edit'] = array(
        '#type' => 'link',
        '#title' => t('edit'),
        '#href' => 'node/' . $node->nid . '/edit',
        '#attributes' => array('title' => t('Edit this node')),
      );

So that other modules can add #access = FALSE.

But that's tiny minor tiny and I'm not making a patch for that =)

The main issue is the biggest: form actions in an actions wrapper and named.

rudiedirkx’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, form-element-names.patch, failed testing.

rudiedirkx’s picture

Why doesn't it apply? Works fine for me locally... The GIT commit ID's are nonsense, but that shouldn't matter. The paths are correct, right?

recidive’s picture

@rudiedirkx: Is the patch for 3.x or 2.x ?

rudiedirkx’s picture

I thought 3, but apparently 7.x-2.0-beta1... Crap. Make new patch? =)

SebCorbin’s picture

Version:7.x-3.x-dev» 7.x-2.x-dev
Status:Needs work» Needs review
StatusFileSize
new1.53 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
rudiedirkx’s picture

Thanks SebCorbin, but I think they want a patch for 3.x. Maybe I'm wrong?

SebCorbin’s picture

Status:Needs review» Fixed

Committed to both 7.x-2.x and 7.x-3.X

Status:Fixed» Closed (fixed)

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