Make drupal_get_form to return unrendered forms

you can get a lot further by using a simple regexp replace preg_replace('/drupal_get_form((?:[^()]++|\((?1)\))+)/U', 'drupal_render(\0)', to wrap the standalone drupal_get_form calls in a drupal_render.

Edit: I mean apply this to Drupal modules themselves.

Thanks for reviving this. Decided to take a look and see how it is going ...

"for that it's necessary to use drupal_render". append parens to drupal_render() so it hyperlinks in api docs.

at first blush, this looks like it can be a regular menu callback that returns an unrendered array: locale_translate_seek_screen

user_admin() does not look right. you mix $page and $output. That function is a throwback to pre_fapi. Not in scope for this patch but a refactor is needed.

With the page array we surely can do a lot with this... I see search.pages.inc converted for that, can we get the rest converted too?

Here is a complete patch which does just as dmitrig01 proposed - remove drupal_render() from FAPI. In Drupal7, menu callbacks are supposed to return arrays so calling drupal_render() on a form is not needed nor desired. Lets leave this content in arrays form and just let it render with the rest of the page.

This patch adds a few drupal_render() calls which should eventually be removed when those menu callbacks refator to return an array. There is no need to do that much refactoring in this patch.

I only have one question. Have you checked that everything you changed is covered by a test?

fixed install. lets let the bot find more stuff.

tabledrag is still funny looking.

Fixed test errors. /me hugs the test suite and test bot. How did we live without thee?

Reminder. devel.module has an option to pretty print the $page array during hook_page_alter(). You might want to use that to browse how a form page looks (e.g. admin/build/block).

subscribe

I find this syntax to be kind of ugly:

   $output = drupal_render(drupal_get_form('update_script_selection_form'));

I understand why we want drupal_get_form to return unrendered forms for the page, but I'm hesitent to do it at the expense of making drupal_get_form complex. drupal_get_form is intentionally meant to be kept very simple, and if you need complexity you move to drupal_build_form() where you have more power.

Maybe we should use two functions. drupal_get_form() and drupal_render_form()? I'm unsure if this is really the right answer but it's something we should give some thought to.

Thanks for the review.

I started with a two function solution where where we have a drupal_get_form that is unrendered and a rendered version as you suggest. I found it to be awkward, and also not backwards compatible. But more importantly ...

The ugly code you mention is temporary. We should *not* be using drupal_render() in module code. Modules need to pass back arrays, not strings. There are very few legitimate uses for drupal_render() beyond drupal_render_page(). Just a few theme functions that need to ccarefully control their output. I added some drupal_render() calls because refactoring spaghetti like comment module is beyond the scope of this patch. I'll do it, but not here.

-  $output = drupal_get_form('locale_translation_filter_form');
+  $output = drupal_render(drupal_get_form('locale_translation_filter_form'));

Unlike Earl, I think that drupal_render(drupal_get_form('locale_translation_filter_form')) is actually quite elegant, and more readable and predictable than what we have right now.

What's missing though, is the motivation for this patch. Is it just clean-up, or does it open certain doors?

The motivation is to get as many pages as possible to play nicely with the new hook_page_alter(). With this patch, all pages whose page callback is drupal_get_form() will now stay in array form when they go through that hook. Without this patch, all these pages are already rendered HTML by this time.

One might use hook_page_alter() to split the form into multiple page regions. Thats not possible with hook_form_alter().

Thanks Moshe, that is what I expected. I think this should go in, but I'm waiting for the test bot. I can run the tests myself, if necessary.

@Dries - the test bot is woefully behind, AFAICT. See http://testing.drupal.org/pifr/status. Also, results are apparently not being reported back to drupal.org.Would be wonderful if you ran those tests.

Oh nice. test bot has marked this patch green again.

Committed to CVS HEAD. Please update the upgrade instructions in the handbook, and mark this 'fixed' after. Thanks all!

upgrade docs authored

It's probably too late, but I would have done it in a different way, to make module migration to D7 easier, and allow drupal_get_form to remain being used in hook_menu:
- have drupal_get_form behave as it used to.
- split the function internally, so it does something like:

<?php
function drupal_get_form($form_id) {
  $form = drupal_get_form_build($form_id);
  return drupal_render($form);
}
?>

Would have been easier, and could even be used in D6. drupal_get_form() could still be used in hook_menu. And one could use the other function "drupal_get_form_build" (or choose a better name) where you need the unrendered form array.

In fact I did this in a custom D6 module for myself, but it feels stupid having to copy core things.
(this is what I wanted to report before I found this thread)