remove $op from hook_nodeapi
justinrandell - September 27, 2008 - 15:58
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | task |
| Priority: | critical |
| Assigned: | recidive |
| Status: | closed |
Description
same motivation as remove $op from hook_user, the time has come for $op in hooks.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| nodeapi.patch | 55.63 KB | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
all tests pass, except for the upload tests that fail on a clean HEAD.
#2
O WOW.
OK, this is better code, which is cool.
This is huge. You are brave.
I don't know what else to say.
#3
Subscribe! Let's kill $op! Kill it dead!
#4
will update this patch once HEAD is fixed.
#5
updated for HEAD, all tests pass.
#6
As with the user de-$op patch, there's lots here, and plenty of additional cleanup we can do after it lands. I confirmed that no tests are harmed by the patch, so let's get this in then open up many smaller follow-up issues to clean this API up more.
#7
+1 and subscribing to this one.
#8
This patch is made of pure awesome.... aka subscribe.
#9
(party) (dance) :D yes yes yes please.
#10
Please don't get out the pitchforks, but would it be unreasonable to supply an old-style nodeapi stub to funnel the requests through to the new functions? I know legacy support is considered evil and all, but this is going to hit a huge number of modules, and I doubt any magic upgrade script will be able to do auto conversions. ... just so we don't fall further into another huge module upgrade lapse like we have with D6? Reduce the pain? The fix would be only a few lines.
Complain in coder.module, trigger "deprecated" notices in watchdog, but help upgraders to make it work first time. :-}
[ .dan. runs and hides ... ]
#11
I believe webchick put it the best when she said "Kill it dead!". Besides, all API changes are documented at Converting 6.x modules to 7.x.
#12
not to mention.... weŕe not in freeze yet... so a big -1 (where did i left the pitchfork!) at backward compability!
This is the only time we can do this this way, and we have the oppertunity to do it good, so let's kill that bastard consistently and completely ($op that is...)
#13
-1 for keeping a "compatibility layer" from me too.
i think the new shiny is worth the pain, and its consistent with The Drupal Way...
#14
Once this goes in I want to see if we can stuff $teaser and $page into the $node object (if they aren't already) and remove them from the function signatures, so we get closer to just passing a single object around the API by reference. So this is hopefully just the first step at fixing this up.
#15
Ok guys, forget I said anything. Just trying to smooth the ride. :-}
#16
-1 on keeping a "compatibility layer".
The good thing about Drupal is we don't care much about backwards compatibility. This is stated somewhere by an authoritative source so don't quote me :)
#17
Reviewed, tested, and committed to CVS HEAD. Marking as 'code needs work' until this has been properly documented in the upgrade instructions.
#18
Added the update module documentation for remove $op.
#19
The 'prepare translation' op was missed. Attached patch changes it to 'prepare_translation'.
#20
Yup.
#21
Thanks. :)
Marking CNW because the docs in core.php for this are now completely and utterly wrong.
#22
*cough* #314870: UNSTABLE-4 blocker: Add api.php for every core module *cough*
#23
Yeah, I already chimed in there. I don't feel that issue is going to get solved within the next 2-3 days. But we need documentation for this change ASAP.
#24
i've started on this, so if anyone else wants to chime in let me know.
#25
here's a work in progress patch core.php.
note the function signatures - i'd appreciate some feedback. in almost all cases, i think we can eliminate the params formerly knows as $a3 and $a4.
i've created a follow-up issue to clean up the code now we're not stuck with a one-size-fits-all API.
#26
changing title and queue.
#27
This looks good to me.
#28
Just as an aside to those in this issue - today my eye was drawn to code in content.module
<?php/**
* Implementation of hook_nodeapi().
*/
function content_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
$callback = 'content_'. str_replace(' ', '_', $op);
if (function_exists($callback)) {
$callback($node, $a3, $a4);
}
}
?>
I just thought I'd like to say that it made me smile. I like nice code. That is all.
#29
Needs update to head.
#30
#31
thanks Rob.
what's the way to go here? as it is, this is worth committing. who has commit access?
#32
looking good, marking this as RTBC
#33
Hi, I've posted a list of where/how nodeapi hooks are called. I think this might help here with documenting the correct function signatures.
#34
Moved to Drupal core since node.api.php is now in.
#35
Updated the docs to match nodeapi signatures from #319356: Update hook_nodeapi_* functions signatures and documentation.
I'm going to add some examples of hooks implementations.
#36
Added some implementation examples. It's missing examples on hook_nodeapi_alter() and hook_nodeapi_prepare_translation() (I couldn't find module implementing those hooks in core. Also hook_nodeapi_print() doesn't exist anymore, so removing it.
#37
Merged documentation to the patch in #319356: Update hook_nodeapi_* functions signatures and documentation.
Changing the title back to "remove $op from hook_nodeapi" for future reference. And closing down the issue.
#38
Could someone in here explain me how it's now possible to have one nodeapi hook doing the same for more than one hook in the same way?
Now this duplicates code n-times... in D6 it was much easier:
<?phpfunction linkchecker_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
switch ($op) {
case 'insert':
case 'update':
// The node is going to be published.
if ($node->status && _linkchecker_scan_nodetype($node->type)) {
_linkchecker_add_node_links($node);
}
break;
...
}
?>
<?php
function hook_node_update($node) {
// The node is going to be published.
if ($node->status && _linkchecker_scan_nodetype($node->type)) {
_linkchecker_add_node_links($node);
}
}
function hook_node_insert($node) {
// The node is going to be published.
if ($node->status && _linkchecker_scan_nodetype($node->type)) {
_linkchecker_add_node_links($node);
}
}
?>
#39
<?phpfunction hook_node_update(&$node) {
hook_node_insert($node);
}
?>
Maybe.