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.

CommentFileSizeAuthor
#10 1442928.patch1.53 KBSebCorbin
#3 form-element-names.patch1.23 KBrudiedirkx
form-buttons.patch815 bytesrudiedirkx

Comments

rudiedirkx’s picture

rudiedirkx commented

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

amateescu commented
Status: Active » Needs work

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

rudiedirkx’s picture

rudiedirkx commented
StatusFileSize
new form-element-names.patch1.23 KB

Ah man =)

Okay, here it is.

rudiedirkx’s picture

rudiedirkx commented

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

rudiedirkx commented
Status: Needs work » Needs review

Status: Needs review » Needs work

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

rudiedirkx’s picture

rudiedirkx commented

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

recidive commented

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

rudiedirkx’s picture

rudiedirkx commented

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

SebCorbin’s picture

SebCorbin commented
Version: 7.x-3.x-dev » 7.x-2.x-dev
Status: Needs work » Needs review
StatusFileSize
new 1442928.patch1.53 KB
rudiedirkx’s picture

rudiedirkx commented

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

SebCorbin’s picture

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