Currently, Drupal has three structures stored in our standard keyed array format. Forms, Node content, and links. In addition, there are patches in the queue to convert other data like user profiles and block content to the same structured array format. Passing all of these structures into drupal_render() outputs fully themable HTML in a nice, slick fashion.
In every case, we provide a hook_foo_alter() system to make modification of the structures easy for every module. This means duplicated code, subtly different implementations from one data type to another, and all sorts of ugliness. Having individual hooks is good (we don't want modules to be tied to a single gigantic hook), but the burden of doing it 'right' is spread over many spots in the code.
This patch solves the problem by introducing the central drupal_alter() function.
How does it work? Whenever you have a piece of structured array content, and you want to allow other modules to alter it as needed, you call drupal_alter().
$node_content_array = drupal_alter('node_content', $node_content_array, $node, $teaser, $page);
$links_array = drupal_alter('links', $links_array, 'comment');
$thingie_array = drupal_alter('thingie', $thingie_array, $extra_param);
And so on.
Modules wanting to implement alteration hooks do the following:
mymodule_node_alter($node_content_array, $node, $teaser, $page) {}
mymodule_links_alter($links, 'context') {}
mymodule_node_alter($thingie_array, $extra_param) {}
Like drupal_render(), it will help standardize and simplify the process of working with complex, pluggable data structures. The current version of the patch adds the drupal_alter() function, and converts all core implementations of hook_form_alter() to work with it correctly. Node content altering, link array altering, and other similar operations should be converted in subsequent patches if this is committed. They each have their own details to iron out in order to bring them into sync, and can be tackled in a more granular fashion.
What are the drawbacks? hook_form_alter()'s signature changes slightly: $form has to come BEFORE $form_id, as the structured array always needs to be the first param in an alter function. Also, the nature of the function means that we need alter functions to always RETURN their altered array rather than working on a byref copy.
Comments and feedback are more than welcome. Please chime in. Thanks!
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | alter_corr.patch | 1.5 KB | heine |
| #17 | alter_3.patch | 12.85 KB | eaton |
| #15 | alter_2.patch | 12.85 KB | eaton |
| #13 | alter_1.patch | 14.33 KB | eaton |
| #11 | alter_0.patch | 14.33 KB | eaton |
Comments
Comment #1
eaton commentedKilles just noted on IRC that we also have a hook_mail_alter function -- another place for us to centralize this work. We're moving towards a very flexible and consistent data storage model, we need to make sure that the method of working with it is just as easy to exploit.
Comment #2
webchickI'm all for this. Anything that makes the Drupal API more consistent and thus more easy to learn for new developers gets a huge +1 from me.
This is actually a cheap ploy to subscribe to this issue; I'll review more thoroughly later. :)
Comment #3
eaton commentedOne of the additional benefits of this shift (even though it's not part of the patch at present) is the ability to use systems like fQuery to manipulate the standard arrays, rather than re-writing out own array walkers for each module. The #keyed_array structure is robust, and it works, and I think standardizing on it as well as the alter/render paradigm will work well.
Comment #4
webchickA few comments:
1. On the PHPDoc, this is weird:
"+ * This dispatch function hands off structured drupal arrays to
+ * type-specific hook_form_alter implementations. "
we shouldn't say "type-specific hook_form_alter implementations" because henceforth, hook_form_alter is being altered to use this as its implementation (did that make any sense? ;)). We don't want head-nods to legacy stuff in the docs. Also, Drupal should be capitalized </nitpick> ;)
2. Some more comments in drupal_alter to explain why data wasn't passed by reference would be good.
Otherwise, tested uploads, color module, and node module with forum/comment/taxonomy/menu additions. Everything looks good!
The API change will be a little awkward at first with $form_id and $form swapping places, but I think the standardization we will gain from this is worth it.
Comment #5
eaton commentedThanks. Attached is a version that beefs up the comments a bit and clarifies the only funky bit of the function. In addition, it defaults $data to an array, so passing-of-empty-stuff to the function won't explode in flames.
Comment #6
merlinofchaos commentedI think this is generally a very good idea. It makes it easy to implement from the side that needs to be altered, and the hook_*_alter functions will have a consistent argument start.
Comment #7
dmitrig01 commented+1
I might start writing my own hook_*_alter functions
Comment #8
eaton commentedHere's an updated version of the patch. It migrates hook_link_alter, hook_mail_alter, and hook_profile_alter to the new drupal_alter() syntax. I've avoided converting three things: hook_menu_alter, since the menu system is still changing and I don't want to collide with it, hook_nodeapi (op = alter), since it needs to change as a part of a comprehensive node rendering refactoring, and hook_comment op view, for the same reasons as nodeapi.
So. Again.
hook_whateveralter($thing_to_alter, $any_other_params, ...); would be the new standard way to alter stuff in Drupal.
$my_think = drupal_alter('thing', $my_thing); would be the way to expose alterable objects. Consistency is good, and happy. Feedback appreciated!
Comment #9
eaton commentedOops. Here's the file.
Comment #10
webchicksubscribing!
Comment #11
eaton commentedHeine pointed out a code style issue, and chx gave the thumbs up on a change to menu.inc -- now hook_menu_alter() is part of the refactoring as well.
Support clean APIs! ;-)
Comment #12
robertdouglass commentedtracking
Comment #13
eaton commentedExcellent news! This version of the patch restores the ability to use references rather than returning the altered array. it makes converting to the new structure just a bit easier, and a bit harder to screw up. Try it! You'll like it!
Comment #14
Amazon commentedI applied this patch and tested it. I enabled all modules and created several content types and added files using the upload module. I also posted comments on all the content types.
I am eager to see more consistency in the API so that we can programatically configure Drupal for install profiles.
Comment #15
eaton commentedDoh. This is a better update to the patch; older version snuck into the earlier post.
Comment #16
dries commentedCommitted to CVS HEAD. Thanks!
Comment #17
eaton commentedHere's a version with two typos in drupal_alter() fixed. No code changes. ;-)
Comment #18
heine commentedSmall spelling correction of comments and one stray $form, return $form apparantly left in comment_form_alter.
Comment #19
RobRoy commentedIsn't it $form_id, then &form, not the reverse?
Comment #20
RobRoy commentedI haven't really been following, I just wanted to jot that before I forgot..but it's probably unfounded and the args have gotten switched in HEAD for this.
Comment #21
eaton commentedThe args for alter operations have, indeed, been switched. I'm not super happy with that, but I think it's the best solution to one aspect of the consistency problem. Now, the first element is always the alterable one and any additional params (like an id or an additional noe in the case of hook_link_alter) come after that...
Comment #22
dries commentedCommitted #18 to CVS HEAD. Not sure we still need #17?
Comment #23
eaton commentedNope, Steven's fixes and Heine's comment patch fixed the remaining issues that I'm aware of. Thanks!
Comment #24
webchickSounds like 'fixed' to me. :)
Comment #25
(not verified) commented