Hello.

You may consider this to be a minor bug, but it I decided to tell about it because it breaks Drupal form API idea.

It is incorrect to create a theme_privatemsg_list function and build a form component model in it which is then displayed with help of drupal_get_form('privatemsg_list_form', $form). A part of the form is outputed in theme_privatemsg_list and another part in drupal_get_form() which is inconvinient - we need to override both theme functions to customize output, difficult to create an accurate layout.

drupal_get_form() function already has an ability to call a theme() function depending on form_id. So, you just need to build a form model and call drupal_get_form('privatemsg_list_form', $form). Then you can override theme_privatemsg_list_form() function to customize output of the whole form.

My patch is very simple, it does 2 main things:
1. Change theme("privatemsg_list",...) call to privatemsg_list() call.
2. Remove $out = theme('links', $folder_list); from privatemsg_list() function and change it to:

  $form['folders'] = array(
      '#type' => 'markup',
      '#value' => theme('links', $folder_list)
      );

which is the main fix which gives us a single form with all data in it.

Please review my patch.
Thank you for your good and usefull module.

CommentFileSizeAuthor
privatemsg_list.patch1.31 KBardas
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mindless’s picture

mindless CreditAttribution: mindless commented

#2 makes sense.. can you explain a little more why #1 is needed? since some people may have built a theme to override this function, seems better to leave that part unless there is a strong reason to remove it. thanks.

ardas’s picture

ardas CreditAttribution: ardas commented

#2 goes together with #1.

The idea is the following:

You don't need to have theme("privatemsg_list") function which will call drupal_get_form('privatemsg_list_form') inside. Generally, theme() functions should not call drupal_get_form() method because call drupal_get_form() already has an ability to be themed using its theme function (built upon form's id). So, you will have some kind of "nested" theme functions.

In you situation you use drupal_get_form('privatemsg_list_form') and it means that if someone would like to customize output he can instantiate theme_privatemsg_list_form() function and customize form layout. This theme function will be called by drupal_get_form() automatically.

The usual practice is:
1. Define a usual function which should output a form
2. In this function create a form component ( $form = array (...) )
3. Render a form using drupal_get_form() in the end of the function.

The rendered form will be already theme sensible. That's why we also don't need to call

$out = theme('links', $folder_list);
return $out . drupal_get_form('privatemsg_list_form', $form);

It is better to have everything within one form component.

Each form can be customized by ['theme_' + form id] function. That is why we don't need to call drupal_get_form from within another theme function.

mindless’s picture

mindless CreditAttribution: mindless commented
Assigned: Unassigned » mindless
Status: Active » Fixed

Thanks, makes sense. I committed a change based on this patch to the DRUPAL-5 branch..

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)