The trigger module defines hook_hook_info(). I think the name of this hook is confusing. It looks like it should provide info on Drupal hooks in general, from the name, but it is actually just providing information on triggers for actions to the trigger module. I just think we should not have things called 'hooks' in Drupal other than actual Drupal hooks.
So I would propose changing the hook's name to hook_trigger_info(), or something similar.
If we do this, we would also want to update some other documentation in the trigger module that mentions 'hooks', and make sure it only says 'hooks' when it is talking about a Drupal hook or its implementation, rather than for a trigger name. For example, hook_action_info() returns an array with component 'hooks', which should be called 'triggers', I think. The documentation of that component is very confusing to me as it is.
If there is agreement on the principle, I could probably attempt a patch...
Refs:
http://api.drupal.org/api/function/hook_hook_info/7
http://api.drupal.org/api/function/hook_action_info/7
Comment | File | Size | Author |
---|---|---|---|
#92 | drupal.actions.92.patch | 116.99 KB | sun |
#90 | drupal.actions.90.patch | 116.96 KB | sun |
#88 | 525540actions12jv.patch | 116.96 KB | jvandyk |
#86 | 525540actions11jv.patch | 99 KB | jvandyk |
#83 | drupal.trigger-revamp.patch | 116.82 KB | sun |
Comments
Comment #1
brianV CreditAttribution: brianV commentedAgreed. hook_hook_info() is a bad and confusing name.
I like hook_trigger_info() *much* better.
Comment #2
jhodgdonI was working on a patch for this, and I still think that hook_trigger_info() would be better than hook_hook_info() as a name for the base hook of the trigger module, and that we should avoid using the word "hook" in Drupal documentation and functions, unless it refers to a hook in the normal Drupal sense of the word.
However, there's still some terminology that needs reworking, because a "trigger" is a combination of a "hook" (bad current term) and an "operation". For instance, there is a node_update trigger, which is hook=node, operation=update. So we need a replacement for the word "hook" in that context. It's generally a module, but not necessarily the same as the name of the module, as the module can define these in hook_hook_info (or the renamed hook_trigger_info). It's an operand, but I don't think that's a good name for it.
Thoughts?
Comment #3
webchickNot sure about this one. That data is there for more than just Trigger module. Trigger module is just an example in core of how to put action and hook data into practice, but other contributed modules also invoke these to determine what system events they can attach actions to.
Also, the hook_info hook *does* describe information about hooks exposed by a module. http://api.drupal.org/api/function/node_hook_info/6 contains hook_nodeapi and all of its various ops.
What about this hook's name/contents did you find confusing? Could it be addressed with better documentation?
Comment #4
jhodgdonThat logic was OK in Drupal 6, but it doesn't work any more in Drupal 7.
Only one of the many "hooks" given in Drupal 7 core hook_hook_info implementations is actually a Drupal hook (in the normal Drupal sense of the word "hook") -- that is hook_cron() named in system_hook_info(). All of the rest of the hook_hook_info() return values for core implementations are not keyed off the actual Drupal hook names any more. For instance, your example of node_hook_info(): in Drupal 7, the inner array has key "node", but there is no hook_node() in Drupal 7 -- the actual hook names are node_insert, node_delete, etc., not hook_node( $op = 'insert'). Which is to say, the Drupal hook name is a combination of the trigger/action "hook" name ("node" in this case) and the trigger/action "operation" ("insert", "delete", etc.).
I don't see any way to clean up the documentation of hook_hook_info(), assuming we keep the word "hook" all over the Trigger and Action module documentation, to avoid confusion between the trigger/action meaning of "hook" and the standard Drupal "hook" concept. The example mentioned over and over in http://api.drupal.org/api/function/hook_hook_info/7 is hook_node(), which doesn't exist at all.
Besides which, really in any module that defines triggers/actions, you could use anything for the name of the trigger/action "hook". You can name your trigger/action "hook" whatever you want, and cause it to be fired whenever you want by calling actions_do() in your module in any function of your module. Actually -- and this is bad -- the only way for your module to find out what actions the user has assigned to your trigger/action "hook" is to call _trigger_get_hook_aids(), which is not even an official API function (starting with an underscore as it does).
Anyway, it may not make sense to tie this to a Drupal hook at all -- you just want some actions (that the user chooses) to be fired off when a particular "trigger" happens in your module, and that may not be related to a Drupal hook that your module is defining -- why would it? The Trigger module and the Actions API don't care whether these triggers/actions are related to a Drupal hook, and Trigger is only going to be putting up the UI screen for you, not causing those actions to be fired (you have to do that yourself in your module). The hook_hook_info() hook documentation should mention this.
So I stand by my original premise: Especially in Drupal 7, calling the array keys returned by hook_hook_info() "hooks" is innacurate and confusing, since they need not be actual Drupal hooks (most of the core ones aren't). And calling the hook hook_hook_info() is also confusing, since implementations of this don't begin to cover all Drupal hooks, and they need not even be related to core Drupal hooks.
Additional issues with the Trigger module and Trigger API:
- There is no official Trigger API function for finding out which actions the Trigger module's UI has assigned to your trigger/action "hook" -- the closest thing is function _trigger_get_hook_aids() which is internal-use only.
- Taxonomy action triggering in trigger.module is broken (actions would never fire, as they are in an implementation of hook_taxonomy(), which no longer exists in Drupal 7). I'll make sure this is filed as a separate issue.
- The actual action firing-off for core modules in Trigger isn't really tied directly to what is in the core modules' hook_hook_info() implementations. It is hard-wired into the Trigger module (which has to implement those core modules' hooks in order to fire the actions). So in my opinion, the current hook_hook_info() implementations really should be stored in the Trigger module, not the individual core modules, since the Trigger module is what is actually making the actions fire off.
- hook_hook_info() documentation should make clear that your module needs to call actions_do() in order to cause anything to happen, because the Trigger module isn't going to do it for you.
Thoughts?
Comment #5
jhodgdonI filed #538058: Trigger module is not triggering taxonomy correctly on the taxonomy triggering issue.
I think I like the name "event" for a combination of a trigger/action "hook" and an operation, and the name "group" for what is now called a trigger/action "hook".
Comment #6
jhodgdonI also filed #538064: No official Trigger API function to discover assigned actions on the issue of not having an API function for discovering assigned actions.
Comment #7
webchickWow, nice retort! :)
I'd love to get some opinions on this from someone like jvandyk or eaton, since they are more familiar with actions/trigger than I am. But what it sounds like to me at a glance is simply that hook_hook_info() was never updated when we renamed the node, taxonomy, comment, etc. hooks. This is not particularly shocking given that I can count the number of people who really understand this stuff on about 4 fingers. :P
So while we might have a problem with the name of the hook as well, what it more sounds like to me atm is you've uncovered an actual oversight/bug (not to mention a gaping hole in our test coverage...)
Comment #8
jhodgdonYes, definitely a gaping hole in testing. I mentioned that in the first comment on #538058: Trigger module is not triggering taxonomy correctly.
Comment #9
jhodgdonI solicited opinions from jvandyk and eaton. Haven't heard from eaton, but jvandyk wrote back and said that he didn't have time to look into it, but "...if ops have truly gone away than a revisit to how triggers happen is clearly needed."
Comment #10
jhodgdonI have a big patch to fix this and the following other issues:
#538064: No official Trigger API function to discover assigned actions
#539716: actions_get_all_actions - wrong documentation
#538058: Trigger module is not triggering taxonomy correctly
Sorry for having to roll them all together, as they are all intertwined.
This patch makes the following changes, some of which will need to be documented on the module update 6/7 guide, if this patch is committed:
- Hook names, function names, and parameters now make (hopefully) more sense, in that they don't refer to "hooks" unless they are really talking about Drupal hooks (see extensive discussion above in this issue).
- In particular, hook_hook_info() is now hook_trigger_info(), and the 'hooks' component of the hook_action_info() return value is now 'triggers'.
- All of the hook_trigger_info() implementations that were previously in node, system, comment, taxonomy, and user modules have been moved into trigger.module, because trigger.module is the place they are actually triggered.
- In general, the API doc and help text has been cleaned up.
- There's a new API function in Trigger that tells you what the user did in the UI - trigger_get_assigned_actions()
- Descriptions of actions have been updated to UI text guidelines (mostly that they should refer to "content" not "post" as in "publish content" vs. "publish post")
- Paths for trigger pages are now consistently admin/structure/trigger/module -- so the path admin/structure/trigger/cron is now admin/structure/trigger/system
- Fixed taxonomy triggering so it would actually work with current taxonomy hook API
- Added the ability to attach system actions to user logout
- Added tests for previously untested sections of the trigger module: user, taxonomy, and comment
Comment #11
jhodgdonComment #12
jhodgdonI am setting this to critical because it fixes another critical issue #538058: Trigger module is not triggering taxonomy correctly.
Comment #13
jhodgdonComment #15
webchickOh, testing bot...
Comment #17
jhodgdonSome changes in system.module made the previous patch not apply. Here's a re-roll.
Also tagging this issue...
Comment #18
jhodgdonJust another note that if this patch is committed, there will be some documentation needed (module update guide), and also the trigger example module will need a rework (which it could use anyway -- it hardly makes any sense to me).
Comment #19
jhodgdonBojhan just wrote to me and suggested that maybe includes/actions.inc should be made into a module (currently its menu router item and page-generating function are in the system module). I'm going to look into whether that makes sense.
I also am thinking that the actions API should not have hook_action_info() returning trigger information -- the actions API should be independent of the trigger module.
Also, I realized that the actions setup screen is displaying non-translatable text (the Type column is not translatable as it stands). Another problem that should be fixed in this overhaul.
And I should probably look through the issue queue and see if anything else relevant has been reported against actions and trigger.
This may get pushed off to Drupal 8... I'm not sure I can get the whole thing fixed up in the next week, not to mention getting it reviewed and committed, but we'll see. I should have considerable time to work on it next Monday and Tuesday.
Anyway, for the moment, I'm marking this as "needs work", and I don't recommend committing this patch.
Comment #20
jhodgdonComment #21
webchickNote that I just committed #296322: Tests for abort of actions firing in a loop + trigger module API is completely broken + fix which might have some overlap with this; not sure.
Comment #22
sun.core CreditAttribution: sun.core commentedComment #23
Dave ReidAlso see #344772: Rename hook_hook_info() to hook_trigger_info()
Comment #25
jhodgdonThanks for the links. Sigh.
Comment #26
jhodgdonI took a look at other actions/trigger related issues that have been reported and/or fixed/committed recently...
Given that code freeze is in a week, and that core is a moving target due to these other issues, and the difficulty of getting patches reviewed at all, I don't think I can get a comprehensive overhaul patch written, approved, and committed before then.
Some of the problems discussed above are on other issues, too. Anyway, I think I'll just unassign myself and see what happens with the other issues.
Comment #27
jvandyk CreditAttribution: jvandyk commentedSince hooks and ops are no longer separate in Drupal 7, it doesn't make sense to artificially support ops in actions/triggers anymore. This simplifies things. Hooks could just be supported. The hook and op columns in the trigger_assignments table get combined. We just need a way to still group things together in a nice UI. jhodgdon suggests 'groups' as a term.
The patch in #17 is quite massive. I wonder if we can break out some of the changes being made.
Comment #28
jhodgdonYeah, I was going for a comprehensive "let's make this module actually make sense" approach in #17's patch, because it didn't make sense to me to just tweak the API a little if the whole approach was outmoded. And I didn't want to patch those 3 issues I was working on in 3 separate patches that wouldn't play well together.
So probably the terminology and hook changes could be pulled out and discussed/patched on #344772: Rename hook_hook_info() to hook_trigger_info(). That would need to go in before code freeze.
The part that fixes the taxonomy issues could be done on #538058: Trigger module is not triggering taxonomy correctly after the code freeze, as it's a bug fix and doesn't involve API changes. I plan to do that, but will wait for after code freeze.
Ditto with the doc fixes -- those can wait until after code freeze, and I will do what I can after code freeze with whatever the state of the API is. Doc patches I've made in the past have had good success at getting reviewed and committed.
But I am not going to try to get in any API changes before code freeze, having had little luck with my other API patches that are just as important (to me anyway) and have been sitting in "needs review" for weeks on end. If someone else wants to have a go at it, the patch is there. If someone wants to commit to working this through review in the next 3 days, I might reconsider, but it's pointless and discouraging to put in hours on making a patch and then have it sit there for weeks with no review.
Comment #29
jvandyk CreditAttribution: jvandyk commentedI am actively working on this to get rid of ops. New opless hook_action_info():
New opless hook_trigger_info():
Comment #30
jhodgdonI actually do not think that hook_action_info() should specify the triggers each action supports. I would favor putting this into hook_trigger_info instead. Things in actions.inc should not be trigger-specific.
Or maybe there is a better way to connect actions to triggers? Obviously, some action-trigger pairs do not make sense, but maybe this decision should just be left to the user, or specified in some other way?
Comment #31
jvandyk CreditAttribution: jvandyk commentedAn alternative way of connecting actions and triggers was suggested by cwgordon7 at Drupalcon DC. He suggested (/me squints at notes) that hook_trigger_info() could have a 'provides' key and that hook_action_info() could have a 'requires' key. The values of these keys would match up. For example, hook_comment_insert() would have 'provides' => 'comment' and hook_action_info() for the Unpublish Node action would have 'requires' => 'node'. Of course, you can derive some things from others. E.g., if you have a comment you can get the user and the node with a little extra work; trigger.module does that now.
Realistically, are we able to put that together before the code freeze? I dunno.
The current way prevents people from doing silly things with triggers. Of course, the triggerunlock module lets you shoot yourself in the foot if you want to.
I suppose we use the current 'triggers' key as a suggestion, and have a UI toggle that puts trigger in regular vs. expert mode.
Comment #32
jhodgdonI like the requires/provides idea. Comment triggers could also "provide" nodes (e.g. someone adds a comment, and that node gets marked somehow by a custom action someone writes). The generic actions like "send email" and "display a message" might not require anything (though maybe "display a message" could somehow require "interactive" or "logged in", because it wouldn't work too well with cron as a trigger?).
The regular vs. expert mode on the trigger UI is another interesting idea. I think I like the requires/provides logic a bit better though.
Comment #33
jhodgdonIf you went with the requires/provides idea, there would be some upgrade/migration issues to deal with as well, for people moving from D6 to D7. It would be fairly complex... but would probably be a better approach in the long run.
If you get a patch in today or early tomorrow, I could potentially review it, but I do not have time to attempt writing one before the code freeze (too much paying work to do, never a terrible situation).
I should probably also read through all the above notes and make a single, coherent list (and/or file separate issues).
Comment #34
jvandyk CreditAttribution: jvandyk commentedHere's where I'm at. For some reason simpletest is not running for me in general. Changes in this patch (based on patch above):
- ops are removed from everywhere
- trigger_assignments table loses op column (update function translates old ops to hooks; hook node op insert becomes hook node_insert)
- hook_hook_info() becomes hook_trigger_info(). I am lukewarm on this, as I think the hook describes hooks, not just triggers, but several people have said it is more understandable.
- hook_trigger_info() moved into trigger.module. Moderate memory savings if triggers are turned off.
- _trigger_get_aids() becomes trigger_get_assigned_actions()
- special menu case for cron is gone; we call that group System now
Comment #35
jhodgdonI had problems with simpletest last week -- see #554214: Simpletest has fatal error for all tests when profile.module is enabled - might not be your problem, but maybe? Anyway, it looks like the tests passed in the testbot.
Anyway, here are a few minor comments on this patch, which I generally like:
1) Minor doc typo:
Should be "functions that perform" not "functions that performs".
2) I'm not sure why the system_goto_action cannot be an 'any' action, as are send email and display message?
3) Inside function trigger_assign(), there are some commented-out code lines -- why? either put them in or take them out...
4) Doc for hook_action_info() says:
I think it should not be mentioning groups any more? I think it's a flat array of trigger functions?
5) Also in doc for hook_action_info():
This was an error in my patch above -- besides moving node save actions to later in the list, I believe this behavior also adds a node save action if there wasn't one before?
6) Example code for hook_action_info() doesn't match current expectations, as in node_action_info().
7) I think the trigger_update_7000() table row update could be done in an SQL query directly, rather than a loop, something like this:
8) "Get the aids of actions to be executed for a hook." as description for function trigger_get_assigned_actions($hook) --
a) Should be "gets".
b) I think we use IDs rather than aids to describe getting ID numbers in function doc.
b) hook_trigger_info() doc doesn't mention "hooks" but "operations", so this function should also use the same terminology. This applies to the $hook parameter, which should probably be $operation or something like that?
9) hook_trigger_info() example code should match trigger_trigger_info() or some subset thereof.
10)
should be _trigger_user('user_view' ...) I think?
11) Maybe the taxonomy triggers should be taxonomy_term_delete etc. rather than taxonomy_delete, to match the hook names? Just in case someone wanted to do taxonomy_vocabulary_xyz hooks as well?
Comment #36
jvandyk CreditAttribution: jvandyk commentedThanks for the review!
1) Fixed.
2) I'm not sure why the system_goto_action cannot be an 'any' action, as are send email and display message?
Well, you wouldn't want to assign that to cron...
3) Removed commented-out code.
4) Updated to:
5) Updated to:
6) Fixed.
7) I wondered about that too, but was concerned over how cross-database-compatible CONCAT is. Could not get an answer in #drupal and my online forays suggested that CONCAT can get weird, so I elected for the more PHP-based approach. I agree that the direct SQL query is nicer.
8) I think we use IDs rather than aids to describe getting ID numbers in function doc.
b) Not quite sure what you mean here. Changed to "action IDs" to match @return description.
hook_trigger_info() doc doesn't mention "hooks" but "operations", so this function should also use the same terminology. This applies to the $hook parameter, which should probably be $operation or something like that?
I updated the hook_trigger_info() doc to remove mention of operations, since the point of this patch is to get rid of them.
The structure of this hook is...worthy of some serious thought. It currently has three levels:
Level one: the name of the module providing the trigger (used to build tabs on trigger assignment page in trigger_menu()).
Level two: the group (used to build the list of triggers the assignment page in trigger_assign()).
Level three: the hooks and descriptions.
cwgordon7, in particular, has noted the relative absurdity of this structure.
Does it make sense to eliminate the top level and use the group for both menu building and lists-of-triggers?
10) Fixed.
11) Fixed.
Comment #37
jhodgdonLooks good!
Regarding my (7) - PHP loop vs. direct query, I have no idea about CONCAT cross-compatibility, and as the DB table is likely to be small (<100 items) a PHP loop won't be too slow. So, your point is well taken.
Regarding your question about the structure of hook_trigger_info()...
- We were only storing the hook/op combination in the database (now you're storing just hook, which includes op) when assigning actions, so that doesn't point to a need for either module or group.
- As far as I can tell, module/group are only used for grouping on the UI pages. I agree there is no need for both of them to be there.
- Whichever is used, there should be a way to translate the name that is given. So I think using module is a good idea. In the doc, we would want to make it clear that a contrib module could define a trigger for a core module, such as node, and that the module's machine name is what should go in that field. The logic from the old trigger_menu() function can then be used to grab the module's display name (which I believe allows for translation):
Comment #38
jvandyk CreditAttribution: jvandyk commentedWhew. hook_trigger_info() has been flattened by one level. New opless, simpler-to-understand, hook:
Added a test hook_trigger_info() implementation to trigger_test.module that adds a trigger to node tab and a trigger to Trigger Test tab. Revised doc to show new flattened hook_trigger_info().
Comment #39
jhodgdonI think that patch got cut off at the bottom? I don't see any tests in there, and the hook_user() etc. are cut off...
Comment #40
jvandyk CreditAttribution: jvandyk commentedReattempt.
Comment #41
Dave ReidSince we're using MySQL in strict mode w/ D7, you can use "'something' || ' anotherthing'" for concatenation inside SQL.
Comment #42
TheRec CreditAttribution: TheRec commentedOk, the following is a code review... a long one and it should be taken positively and also as suggestions for some cases :) I hope it helps.
, passing this function's input arguments should be , passing the input arguments of this function, I think genitive cannot be used with "function".
Same goes, The documentation of this function
End of line character over the 80 characters limit.
Each element of this array should be put on a new line and indented as it goes past the 80 characters limit. (If you want to be consistent you could also wrap the other 'triggers' even when they do not exceed the limit.)
Just like the previous comment, every items should be on one line according to the coding standards.
Missing a comma at the end of the line.
Maybe we should put emphasis on Save content" as it is refering to a particular type of action ?
These should start with a third person verb form according to the coding standards.
While reading this I was expecting a (3) so maybe we could change it to : , and (3) and optional form definition
Should rather be Adds operators to the hook names and drops the "op" field. I think
This contradicts the coding standards, but currently the de facto "standard" for implementations of hooks is "Implement hook_" (without s). Do not ask me why, but this is this way for the moment, there is an issue to "mass" update this but I am not sure it will ever land ;). There are many occurrences of this I let you find them.See EDIT 2.This review is powered by Dreditor.
**EDIT** Ouch.. cross-post ... well long delay cross post..sorry it took me some time to build the review... I'll try to see if there is something important in the part I did not review.
**EDIT 2** Seems like documentation about this was changed since the last time I have read it. My bad.
Comment #43
jhodgdonDave Reid: RE #41, Will that work in all the DBs that Drupal 7 now supports? The current approach is definitely safe and as it runs once and shouldn't take long, I would advocate keeping it.
So I gave the patch in #40 a thorough read-through (haven't tried running it or installing the module). There are a couple of remaining problems... (sorry I didn't notice the pre-existing ones before):
a)
'op' shouldn't be in there any more, and does the update function need to do something to update that index? I am not sure what happens to an index when one of its fields is dropped. Probably OK, but who knows?
b)
I think the calls to trigger_get_assigned_actions should pass in $hook instead of $op? It worries me that the tests passed with these errors in there. Maybe the tests are no good?
c) As long as you'll be making changes, a doc issue:
@param doesn't match the actual parameter name, and description doesn't quite match either.
Comment #44
TheRec CreditAttribution: TheRec commentedAfter a quick review there was only those two other things in the part I did not review :
I do not really get the meaning of this description. Would it be possible to improve it ?
Missing a comma at the end of the line.
This review is powered by Dreditor.
Comment #45
jhodgdonMinor detail on review in #42 --
You are incorrect about the current standard for hook implementations -- the current standard short doc header is:
Implements hook_foo()
See http://drupal.org/node/1354
Comment #46
jhodgdonAlso, regarding #42, I do not see why we cannot say "this function's arguments" and should have to write "the arguments of this function". Isn't the first one more compact and just as clear?
Comment #47
jhodgdonForgot to set status after substantive problem found in #43
Comment #48
TheRec CreditAttribution: TheRec commentedjhodgdon: Agreed about Implements.. i'll remove it from my previous review (or strike it with correct reference)... wasn't in that state last time I had to create an hook implementation :) My bad.
About using the genitive (possessive case) function's, I was taught that it should not be used for "objects", only for persons (John's pants) or a group of persons (like "men's room") ... but that is only grammar... I think everyone will get that use of genitive but it I was just pointing out what I think is a grammar error. I've seen few occurences of functions's in the core so if you don't correct this I don't think it will prevent it to get commited ;D
Comment #49
jhodgdonReally? You mean you have to say "the muffler of the car" and not "the car's muffler"?
Sorry to say I don't own a copy of Strunk and White, but I usually refer to my well-used copy of "Index to English" that I've had since college (i.e. it's old-school)... It doesn't mention anything about 's being forbidden for objects, and has this example on using "x of y" vs "y's x" for genitive:
And later on, it has more examples like:
the wind's force
the bill's defeat
I just love having grammar discussions in the issue queue. :)
Comment #50
jvandyk CreditAttribution: jvandyk commentedAll issues since #42 addressed. About #43b, yes, we can always use better and more specific tests.
Comment #51
jhodgdonThis could be a US vs. British difference, by the way... I'm not sure where you grew up and/or learned English... But Drupal doc standard (for better or for worse) is to use our sometimes-sloppy "standards" of common modern American usage. To me (a native speaker of American English and a programmer geek for decades) saying "the function's return value" sounds just fine. :)
Comment #52
jhodgdonI am happy with the patch in #50 and I think it should be committed.
The status change in #51 was an artifact of me submitting my comment after #50 was submitted.
I am setting back to needs review so the test bot can test it. Assuming tests passed, I think it is RTBC.
Comment #54
jhodgdonSo much for the tests passing...
Comment #55
jhodgdonSo much for just looking at the code...
I tried to run the tests on a new fresh install of the latest CVS update of D7 (with the above patch applied and no other changes). The Simpletest startup is completely broken. It's getting a database error when it tries to set up the menu router table.
(this error message goes on for many many many lines)
That said, the module itself appears to be somewhat but not totally working. I created a system message configured action, and it successfully triggered on:
- create new user
- create content
- update content
- add new comment
-cron
It did not trigger on
- create taxonomy term
- edit taxonomy term
- delete taxonomy term
Comment #56
jhodgdonSo I have to go now, and I won't be back in my office again before the code freeze (not returning until Sept 8th)...
My review of the code (from looking at it) is that it is a VAST improvement over the previous trigger/actions API.
Obviously (from the tests and from trying it out), it isn't quite working, but if it can be gotten into a state where the existing automated tests will pass, and a simple usage test like what I did in #55 shows that it works OK, then I would mark it RTBC and hope it gets committed before the code freeze. We can then write some more tests at our leisure, and fix any issues that come up (assuming someone fixes the SimpleTest module so that we can actually run tests in our own workspaces!!!!).
Good luck, and sorry I can't help get this pushed through any more!
Comment #57
jhodgdonJust out of curiosity, I submitted the patch on #538058: Trigger module is not triggering taxonomy correctly for retesting as well. Possibly someone committed something else in actions/triggers that broke the tests...
Comment #58
jvandyk CreditAttribution: jvandyk commentedFixed simpletest trouble, which was nondefensive coding in trigger_menu(). Fixed tests. And fixed taxonomy actions.
Comment #60
jvandyk CreditAttribution: jvandyk commentedUpdated actions tests in simpletest/tests.
Comment #61
webchickMoving this into the RTBC queue to look at sometime in the next couple days. Great work, folks!
Comment #62
sunWonderful.
First of all, this patch needs a CHANGELOG.txt entry. :)
Can we remove the "parameter" stuff, please?
The argument is named $object, and what is being called "typically" here, is the expected standard. API functions should always take and expect a certain data format for arguments. (even when another data type may be supported, which I did not look up yet)
Same here. I can only guess that *all* actions expect $context to be an array containing additional objects (?) from the environment that triggered the action.
Nitpick: Please leave blank lines between @param and @return. (possibly elsewhere)
Hm. It would be nice to keep (update) those example array structures.
If that example of *disabled* modules is really true, then we need to create a separate bug report, because no data should ever be removed for *disabled* modules. Disabled modules are just - perhaps temporarily - disabled. Only uninstalled modules are really gone.
huh? ;)
OK. What I absolutely do not understand is why hook_trigger_info() has been removed from some core modules, but not from system.module, and why the API docs still explain this hook - and more confusingly, it contains a copy of the removed node implementation...
Is it gone or not? The rest of the patch made me assume it would have gone... but, uhm. (?)
I didn't read the entire issue, but I read the reasoning for this by coincidence.
However, we should use 'any' here. It's up to the site admin to configure it properly.
Why not simply $module ?
$group argument has been added here without PHPDoc... Fixing the other @params would be neat, too, but not strictly required (though at least the new @param should have a proper description).
It seems like $group was documented here instead.... erm, and also instead of $module ;)
Nitpick: That can go.
The Wrong Indentation police will catch you! :-D
huh? Now I'm totally confused. :)
I'm on crack. Are you, too?
Comment #63
jvandyk CreditAttribution: jvandyk commentedThank you for your review!
Nitpick: Please leave blank lines between @param and @return. (possibly elsewhere)
The rest of core does not do this, and it's not in Documenting functions. Or maybe I've misunderstood you.
Hm. It would be nice to keep (update) those example array structures.
They are now in modules/trigger/trigger.api.php. We could copy them here, I suppose (since actions.inc can be used without trigger.module) or put a @see trigger.api.php. We could put @see hook_action_info()? I choose the latter.
If that example of *disabled* modules is really true, then we need to create a separate bug report, because no data should ever be removed for *disabled* modules.
I agree that we can do this separately.
$foo = 'bar';
Curses, my attempts to get this into core are foiled again!
OK. What I absolutely do not understand is why hook_trigger_info() has been removed from some core modules, but not from system.module, and why the API docs still explain this hook - and more confusingly, it contains a copy of the removed node implementation...
It's easy. hook_hook_info() has been renamed hook_trigger_info() and had its array structure made simpler. All core hook_hook_info() implementations have been moved into trigger.module, primarily to conserve memory since ostensibly they are unused unless trigger.module is enabled anyway. So system_hook_info() is gone and the information is now provided by trigger_trigger_info(), i.e., the trigger.module implementation of hook_trigger_info().
However, we should use 'any' here. It's up to the site admin to configure it properly.
OK. Changed to 'any'.
Why not simply $module?
Because $module is used within the function. Any naming change here is fine with me.
$group argument has been added here without PHPDoc
Now using $module throughout for consistency.
Comment #64
fagoThe patch contains like:
+<<<<<<< node.module
+=======
+>>>>>>> 1.1116
I wonder that it was able to pass the tests!?
Regardless from that what about renaming 'runs when' in hook_trigger_info() and 'description' in hook_action_info to what they really are 'label'. That would 1. make them more consistent and 2. make them consistent with rules.
Rules currently re-introduce similar hooks, hook_rules_action_info() and hook_rules_event_info(). However with the currently proposed design the core hooks come closer to the rules hooks, which I think is good so it's good as it's easier for people to deal with those similar APIs.. Rules uses 'label' in all hooks, so that would be consistent over all then.
It would be cool, if we event could start using the same hook. For hook_trigger_info() I see no problem as trigger.module manually invokes all triggers. For hook_action_info() it's probably better to keep them separated as rules only actions sitting in hook_action_info() would probably cause troubles as well as the other way round.
Anyway, what about using hook_event_info() instead of hook_trigger_info() as name? As I think the hook really describes *events* which in turn are used to *trigger* something.
Comment #65
fagoedit: double-submitted comment :(
Comment #66
sunI agree that a better key would make sense -- however, we usually call this "title" in core.
Renaming the hook names is a bit too late and would require more discussion, I think...
Comment #67
fagook, I had a look at it and replace 'description' of hook_action_info and 'runs when' of hook_trigger_info with label. The description of action appeared quite often, but I think I got them all. Also I added an update to rename the description column to label. While doing this change I found some other issues I corrected too:
* hook_action_info() and co is described in trigger.api.php - I corrected this to be in system.api.php. Then action configuration form lives in system.module although the menu items already stated system.admin.inc - where it belongs too. Fixed that too.
Also I fixed the problem in node.module mentioned above and removed the old node_hook_info() which also was left over.
Updated patch attached containing all described changes.
Comment #68
fago@sun:
sry I was already working on the patch, so it implements 'label' now. However the whole field API uses already 'label' so it's already in use too.
Comment #69
jvandyk CreditAttribution: jvandyk commentedChecking in on the run, but +1 for 'label' and the move of hook_action_info() to system.api.php. I also think hook_event_info() is a better name than hook_trigger_info() as a purist, but as a pragmatist hook_trigger_info() is probably more intuitive. Either is fine with me.
Comment #70
jvandyk CreditAttribution: jvandyk commentedApplied the patch in #67. Installed D7. Ran tests, 0 errors. Assigned nonconfigurable and configurable actions to triggers. They fired. Not sure if I should be the one to RTBC this, but I'm not physically at Drupalcon so I can't get my hand slapped except virtually.
Comment #71
fago@jvandyk: Great, it's working fine for me too!
@sun: "Renaming the hook names is a bit too late and would require more discussion, I think..."
hook_trigger_info() gets introduced with that patch, so I don't think it's too late to change it. So for rules, I care much about the terminology, as it's important for people to get it right. So with the keys changed, I think it would be fine to build upon the same hook for events, so I'd prefer hook_event_info() if that's fine with others.
Comment #72
fagoI took a closer look and noticed that the hook_trigger_info array is also keyed by modules. I wonder whether this shouldn't be something like 'group' ? Consider a module providing more node triggers, shouldn't they appear under 'node' too?
Anyway, in rules I don't have that hierarchy, 'module' or 'group' is an info array property there. Well, with rules2 I'm moving to 'group', which is just about grouping in the UI. Consequently it should be run through t() and so be a property. Do you think we should try to streamline that too? Well, we could also do so in a follow-up and let this patch fly.
Comment #73
jvandyk CreditAttribution: jvandyk commentedGroup is what we had before. We had a module - group - hook - 'runs when' hierarchy. Now we have a module - hook - label hierarchy. Other modules can still add hooks to modules. For example, I can have a nodefoo module that provides a trigger for the nodefoo_bar hook that it provides. But if that's node-related, it could define it under 'node' - 'nodefoo_bar' - 'After a bar has been assigned to this node'. That puts the new trigger in among the node triggers that core provides. So we have a simpler system, and a one-to-one correspondence with the menu system.
Comment #74
fagoI see, also the hook documentation explains that well. So let's go with that!
Comment #76
webchickThis is probably a quick re-roll just to catch the path change of Actions. Sorry. :(
Comment #77
jvandyk CreditAttribution: jvandyk commentedRerolled.
Comment #79
jvandyk CreditAttribution: jvandyk commentedComment #80
cweagansSame code as before, so re-RTBC
Comment #82
sunIntroducing a new tag for feature freeze: API clean-up.
Comment #83
sunStraight re-roll.
Well, while being there, I also fixed plenty of PHPDoc, coding-style issues, and removed all unnecessary hunks not belonging to this monster patch.
Comment #84
sunGreen, so reverting status.
Comment #85
Dries CreditAttribution: Dries commentedI spent 30 minutes with the latest version of this patch. Not enough for an in-depth review, but here is some feedback. I haven't found anything major (yet), which is a really good sign. Based on what I saw the last 30 minutes, I think this is in really good shape.
Inconsistent spacing. Really minor.
The text wraps too quick now.
This is still here?
By the way, I don't like how we started to capitalize the letter after : or ;. It is not how it was teached in my school, and we always had an unwritten rule not to. I'd like to switch back to lower case letters after : or ;. Thanks. Not going to hold up this patch for it though.
A TODO? :)
We're still missing a CHANGELOG.txt entry.
Comment #86
jvandyk CreditAttribution: jvandyk commentedFixed spacing, text wrapping, removed foo (though foo still appears twice in menu.inc, hee hee). Left the menu callback comment alone (I agree with Dries, this is incorrect and should be corrected throughout core in a separate patch). Removed the TODO pending expanded test coverage later. Wrote CHANGELOG.txt entry.
Comment #88
jvandyk CreditAttribution: jvandyk commentedComment #90
sunSorry, my monster FAPI clean-up patch introducing $form as first argument to all form builder functions broke this one.
It seems that all remarks have been resolved.
Comment #92
sunFixed 3-4 old form builder signatures of this patch.
Comment #93
Dries CreditAttribution: Dries commentedI looked at this again, and I believe this is an important improvement. Much simpler and better documented.
I committed this to CVS HEAD. Thanks all!
I'm marking it 'code needs work' because we'll want to update the upgrade instructions.
Comment #94
neclimdulforgotten status change.
Comment #95
sunTo be continued over in #585868: Triggers/actions need overhaul (part II) - Presave
Comment #96
jhodgdonI've updated the module 6/7 upgrade guide in accordance with this (huge) patch. Would be good if someone would review what I wrote, since the patch that went in was quite different from my original proposal, and I may not have noticed all the changes or understood them fully.
http://drupal.org/update/modules/6/7#trigger_overhaul
Comment #97
sunLooks accurate to me.
Comment #98
mattyoung CreditAttribution: mattyoung commented.
Comment #100
sunI seriously have problems to find people who understand Trigger/actions.
It would be awesome if all of you could quickly jump over to #585868: Triggers/actions need overhaul (part II) - Presave, so we can at least push one more required clean-up through to make actions not totally suck in D7.
To accomplish that, I need peer-reviews, now.