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
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
Members fund testing for the Drupal project. Drupal Association Learn more

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

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