This patch cache the form_controller_info to avoid the module_invoke_all execution with every form in the screen because the form_alter.
In the case of form_controller_menu, it is unworthy to cache a global that is used only once, and it can't use the existing global because the
form_alter is executed after the form_controller_form.
This patch avoid multiple module_invoke_all and increase performance.

CommentFileSizeAuthor
#10 653226-form-cache-implementations-D6.patch3.05 KBdave reid
#8 653226-form-cache-implementations-D6.patch2.45 KBdave reid
#4 653226-form-cache-implementations-D6.patch2.39 KBdave reid
form_controller_cache_alterations.patch1.42 KBjcmarco

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Cache form_controller_info » Cache form_info
Project: Form controller » Form
Version: 6.x-2.x-dev » 6.x-1.x-dev
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Closed (won't fix) 
+++ form_controller.module	2009-12-07 12:47:51.928125000 +0100
@@ -32,11 +32,12 @@ function form_controller_menu() {
-  static $implementations;
-  if (!isset($implementations)) {
-    $implementations = (bool) module_implements('form_controller_info');

As you can see, it is already cached.

This review is powered by Dreditor.

jcmarco’s picture

jcmarco commented
Status: Closed (won't fix) » Needs review

The problem is not in this line,
what I was caching was

@@ -169,8 +170,9 @@ function form_controller_process_form(&$
);
$form_state['form_controller'] = $context;

- // @todo Cache this.
- foreach (module_invoke_all('form_controller_info') as $name => $info) {

This module_invoke_all is executed with any form in the screen because that form module form_alter,
the question is that if the first time, in the alter_form, that you get all the form_controller_info's then you can use it there as well.

It has no meaning cache once and later you have to run the same invoke in other line again (other function always call)

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 653226-form-cache-implementations-D6.patch2.39 KB

I think this is what jcmarco meant. It makes sense to convert this to using cache_set/get afterwards as well.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ form.module	9 Dec 2009 17:46:40 -0000
@@ -184,6 +185,30 @@ function form_process_form(&$element, $e
+function form_get_implementations() {

Let's rename this to form_info()

This review is powered by Dreditor.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

How about form_get_info()? Feels empty without a verb.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

My reason is http://api.drupal.org/api/function/element_info/7

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 653226-form-cache-implementations-D6.patch2.45 KB

Revised as form_info(). To be fair it's about 50% thing_get_info() and 50% thing_info() in D7 core, but I don't favor one over another.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ form.module	10 Dec 2009 00:22:15 -0000
@@ -184,6 +185,32 @@ function form_process_form(&$element, $e
+ * @todo Actually cache this info using cache_set()?
...
+      if (!isset($info['process callback']) || !function_exists($info['process callback'])) {
+        $info['process callback'] = FALSE;
+      }
+      if (!isset($info['form callback']) || !function_exists($info['form callback'])) {
+        $info['form callback'] = FALSE;
+      }

If we want to consider caching in a later step, then we can't embed this function_exists() into the registry.

That's quite uncommon, and I feel a bit uneasy about embedding it.

+++ form.module	10 Dec 2009 00:22:15 -0000
@@ -184,6 +185,32 @@ function form_process_form(&$element, $e
+function form_info() {

In general, we can fork 99% of the new mollom_form_get_info(), including PHPDoc and function signature.

+++ form.module	10 Dec 2009 00:22:15 -0000
@@ -184,6 +185,32 @@ function form_process_form(&$element, $e
+    foreach (array_keys($form_info) as $name) {
+      // PHP4 does not support foreach and references, so manually reference
+      // the current element.
+      $info = &$form_info[$name];
+      if (!isset($info['process callback']) || !function_exists($info['process callback'])) {

errrr. WTF?

Why don't you use a regular foreach $form_id => $info and access the array keys properly with $form_info[$form_id]?

This review is powered by Dreditor.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 653226-form-cache-implementations-D6.patch3.05 KB

Revised patch that:
- Uses function name form_get_info() for consistency with majority of core API info functions (and it's even mollom_get_form_info)
- Removed the function_exists() check since it doesn't make sense if its cached
- Added the PHP docs from mollom_get_form_info, but removed references to form_id (since that does not apply here)
- Modify by reference using $form_info[$key] instead of my wackiness.