Multiply form alter

webchick: chx: why a global form_alter_all variable though? wouldn't this best be specified per-module?
webchick: or even per-hook_form_alter?
webchick: For backwards compatibility, what you could do is leave hook_form_alter global, and simply go with the hook_form_alter_form_id thing
webchick: that's pretty cool.
webchick: and is $form['#base'] for like "node" ?
chx: webchick: yes

+1 on the concept. For any decently sized form_alter, I'd probably end up doing the same thing in the function itself. Flattening it can only help performance.

Question: What's with the extra variable? Wouldn't that affect every module in the system, even after the module in question has been removed? It seems like it would be cleaner to just automatically fall back to hook_form_alter if hook_form_alter_foo is not found. Or would that kill performance? It feels clunky to have a global "actually call this extra hook on all modules, too" flag.

First, those very few modules that want to alter every form out there, they do a variable_set('form_alter_all', 1) and thus the general hook_form_alter will be fired. By default, it won't.

-1. Ugly hack.

-1. Ugly hack.

Here's another idea, possibly.

When enabling/disabling modules, check to see whether any currently loaded module implements hook_form_alter_all, and then store that result in a variable:

variable_get('hook_form_alter_all, 0)

Probably still an ugly hack, but at least it could be somewhat automated. ;)

I'm kind of uneasy about how much work (not just in this patch, in others as well) is going into changing relatively developer-friendly constructs and replacing them with somewhat arcane and non-intuitive approaches in hopes of securing a cycle or two. Setting secret variable_set flags before a function will operate?

I understand we do a lot of iterating, but do we have any demonstration of actual speed boost from these kinds of things, and does it justify the change? From what profiling that's been done, it seems that the expensive work is done in path lookups, file lookups that hit the database, db hits from lots of variable_get and variable_set thrashing, and things like that. Why this in particular?

I'm with Eaton, there seems to be a push for complexity to gain some unknown number of cycles at the expense of an easy to use interface/API. The complexity also brings with it a greater chance of bugs and makes understand existing modules harder; "now lets see whats the name of the hook and what is the form id" instead of just looking for a fixed hook name. As a developer one of the things I like about Drupal is the fairly clean and mostly straightforward API's.

I kind of disagree.

mymodule_form_alter_node_form() {
 ...
}

is WAY more intuitive than

mymodule_form_alter() {
  if (isset($form['type']) && $form['type']['#value'] .'_node_form' == $form_id) {
  ...
  }
}

Which currently is the only way to form_alter a node form.

this change would also enforce a good developer practice of having separate functions for each separate thing you want to do.

Also, if http://drupal.org/node/79931 goes in (remains to be seen), there'd be no need of the variable... simply do an:

foreach (variable_get('hook_form_alter_all', array()) {
  // do stuff
}

If no modules implement the hook, no cycles wasted, and no need for funky variables.

webchick, I see the bit your posted and I do agree that it's more intuitive. I'd misunderstood some of the earlier code discussions and thought that a separate alter hook would be needed for each *type* of node. That's really one of the only places where we have a multitude of IDs that all need the same kind of altering.

With that in mind, I can see that it *is* a net gain for developers. Thanks for explaining to my somewhat sleep-deprived self. :-)

-1 on secret variables to manipulate how a function works. That simply won't fly.

What exactly is the problem with devel.module?

Devel module isn't a "problem", per se, it's just weird in that it's the only module in pretty much all of core/contrib that actually hook_form_alters each form in Drupal. Every other instance of hook_form_alter is altering a single form, or a group of single forms (all node forms, for example).

i think this is a good change but it only seems like a practical solution if webchick's hook_define_hook gets committed.

- I like the idea.

- I think that calling the form_alter hooks for both #base and $form_id's is a lot of work. The entire #base and $form_id stuff is a bit messy, imo. Do we _really_ need the #base stuff here?

- This needs to be benchmarked.

What did you change in the last patch?

yes, we really need #base here. or else people will have to implement many similar functions like _form_alter_poll_node_form(), _form_alter_form_node_form(), etc.

benchmarks will say for sure, but i think these module_invoke_all() are free from a performance perspective. is all in memory lookups.

there is no more hack for devel.module in latest patch which is fine with me.

I assume this needs to be reworked since the #base element is now gone?

I'm not opposed to this patch, but I must say that the advantage is not immediately clear. From a developer point of view, it seems to bring a small simplification, which is good. Are there other advantages that I might oversee?

Agreed. The advantage isn't necessarily a massive performance/benchmarking improvement; rather it's the ability for a module that's doing some alterations on, say, four different forms... to separate all that code into discrete sensible functions rather than a monster switch statement.

A bit of benchmarking might be nice but the impact of this is really pretty minimal. Supplying a focused and a general version of the alter hooks makes sense.

The theme registry patch split up the _phptemplate_variables() function by what was being overridden, primarily just for readability. This patch is the same concept, but for form_alter() which is also easy to turn into a nasty switch or if/elseif/elseif block. The performance difference should be minimal, and on par with that change in phptemplate, while the readability improvement considerable, also on part with phptemplate. :-)

Part of FAPI 3 in Drupal 6.