At the EOL taxonomy sprint we've been discussing the need to add metadata to terms, either adding fields directly, or having information about relationships etc. available. This patch is a start at introducing hook_taxonomy_term_load() - which will allow contrib modules to add information to the term object.
There's a couple of caveats here:
We don't want taxonomy_get_parents_all() taxonomy_get_children() etc. being called for every term name that's displayed on every node view, since those queries are expensive, and just description/name/weight tends to be used. So I've left taxonomy_get_term() in for now. There's no node_load_lite() so I'm not quite sure how to deal with this. node_load() currently grabs the user name/picture etc. direct from the database to avoid a user_load(), which is ugly as hell.
Note that this version of the patch doesn't yet add synonyms/relations/parents to the term object, it just provides the hook. We've also been discussing whether relations/synonyms should be moved to a contrib module in D7 (which having a hook_taxonomy_term_$op would allow for pretty cleanly) so that's up in the air for now.
Comment | File | Size | Author |
---|---|---|---|
#41 | term_hooks_update.patch | 2.68 KB | catch |
#40 | term_hooks_update.patch | 2.74 KB | catch |
#38 | term_hooks_update.patch | 2.55 KB | catch |
#36 | term_hooks.patch | 11.98 KB | catch |
#33 | term_hooks.patch | 12.02 KB | catch |
Comments
Comment #1
catchSlight revision:
Removed the helper function and now call the hooks directly in taxonomy_term_load(). This also now passes the term by reference so that contrib can alter properties as well as add. I can't get module_invoke_all() to pass by reference, so for now I'm assuming this is the only way to do it. Or maybe module_invoke_all() could do that now we require PHP 5.2 in D7.
Comment #2
pwolanin CreditAttribution: pwolanin commentedquick look, I support the idea, but don't like some of the code. For example:
Could be replaced by a call to (e.g.) drupal_alter('taxonomy_term', $term);
Or use module_invoke_all() - if it's an object, it will always be passed by ref in PHP 5+.
Also, we should stop having this kind of code:
Either make it an array all the way (e.g. menu links) or an object all the way through.
Comment #3
catchComment #4
catchTrying to actually use the hook is making this patch a little bigger than I'd planned. Lots of work still to do as well, and I'm doing a few dbtng updates as I go.
term relations are now factored into a separate module. synonyms are next up. These implement hook_taxonomy_term_load to add themselves into the term object (which currently doesn't happen at all).
What I haven't worked on yet is _save - taxonomy_save_term is horrible - it needs completely refactoring to use drupal_write_record + helper functions and then another module_invoke_all _save so that contrib can do things there too. That'll come up later but posting progress for now.
Comment #5
catchLast patch of the day.
Comment #6
catchWhile there's some cleanup to be done in the patch, and much much more to be done in taxonomy.module, I think this is pretty much ready for review.
What this patch does:
* Renames taxonomy_get_term() to taxonomy_get_term_data() - this is functionally identical to taxonomy_get_term, in that it returns the row from the term_data table, but it's use will be limited to where only name/description/weight are required.
* Adds taxonomy_term_load() which returns a full term object, including parent tids and stuff added by other modules (relations and synonyms are factored out into separate modules and implement hook_taxonomy_term_load()).
* Does the same for _save and _delete
* Adds taxonomy_get_parent_tids() and some other lightweight helper functions, so we don't get recursive function calls when doing a full term load.
* Tidies up some other functions and begins the process of updating taxonomy.module to dbtng (losts more work to do, but not necessarily in this patch).
There's some TODOs:
Fix up taxonomy_term_page so it actually loads the term, rather than querying the table itself then doing taxonomy_get_term in theme_taxonomy_term_page (oh yes, that's what it does).
Write tests for synonyms and relations (the old core tests more or less worked with these changes, but need to factor it out and test the hooks etc.)
I'd like reviews on the general approach taken and any performance concerns etc. As it is at the moment, there's no additional database queries on a basic term object, only if you add other stuff to it via contrib - and then you're going to be doing a lot of extra work in 5/6 anyway to make that information available. Factoring out relations/synonyms into a seperate module is completely optional, but since they're very little used, I think it makes sense. I've not yet decided how to deal with the vocabulary table for those two (at the moment, taxonomy.module still owns those fields).
If you want to test this, patch #302440: EOL Taxonomy Sprint: Taxonomy returns PDOException on "list terms" on clean HEAD install as well since HEAD is broken at the moment. synonym and relations are tarballs because I'm having trouble getting cvsdo to add folders to CVS.
Comment #8
catchUmm, wrong patch, hopefully this one will make DrupalTestBedBot happy.
Comment #9
dman CreditAttribution: dman commentedUse cases for taxonomy_term_load include better factoring of existing utilities like taxonomy_image, taxonomy_enhancer, and taxonomy_term_relations. Currently they all use various methods to get their data (or metadata) available, or require additional init calls, whereas we really want the $term object to be able to initialize itself as logically as $node does on load.
What the patch is doing is a lot closer to the way we EXPECT Drupal objects to behave on load, so that's cool.
Comment #10
catchHere's a new patch, which needs some feedback. It's quite a lot smaller than the previous patches, since some cleanup has already gone in now, and I've simplified what it's doing a bit.
What this patch does:
1. Adds hook_taxonomy_term_$op for load/save/delete in the appropriate places so that modules can act on the term object in these various states
2. Adds taxonomy_get_term_data() - which is essentially taxonomy_term_load() without the hooks - this is for functions like taxonomy_term_select where you really just want the term name and not a lot more and it's used internally in taxonomy_term_load(). taxonomy_term_select() and friends could do with cleanup themselves, so we might want to skip this, and just clean them up instead - but there's many prerequisites to that.
3. minimal cleanup of the functions where these hooks are added so it doesn't hurt eyes too much. There's way more cleanup to be done, but not in this patch.
What it doesn't do:
1. It doesn't remove hook_taxonomy() - because that's still used for vocabularies. I'm tempted to do that in this patch as well, but if I can write tests for the term hooks without having to do so, then I'll post a followup for that.
2. It doesn't touch any of the logic around related terms or synonyms - I think we should look at removing them from core entirely in a separate issue, since with these patches they'll be able to do what they're doing just as effectively, or more, in contrib.
No tests are affected.
Comment #11
catchAnd the test module - most of this is copied form taxonomy synonym handling, except it adds stuff to the term object, saves and deletes it via the taxonomy hooks. Can't get cvsdo to add the files for me, so attached directly here.
Comment #12
catchDon't call hook_taxonomy_term_delete twice.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedI would have removed the other delete hook. We usually delete our data *and then* call the hook for other modules to take action. I will try to review more this later.
Comment #14
catchThat makes sense to me - although I'd still keep it in taxonomy_del_term() so it runs when the function is called directly, just move it above the deletes.
Comment #15
catchThis patch does that, otherwise unchanged.
Comment #16
Crell CreditAttribution: Crell commentedFrom a quick read-through of #15:
Conceptually I like the direction this is going. Parallel APIs are a good thing, at least until we can factor them all into the same API. :-)
Why taxonomy_save_term() instead of taxonomy_term_save(), like taxonomy_term_load()? The verb should come last, per convention elsewhere.
Also, why does $form_values appear inside taxonomy_save_term()? That's an API function, so it should know nothing of FAPI. It shouldn't even use the same variable name. It should take a taxonomy object (or whatever), in the exact same form as it comes out of taxonomy_term_load(). Marshalling FAPI structures to the data object is the submit handler's job. Let's not repeat the same mistakes as hook_nodeapi $op insert/update.
Comment #17
catchAs discussed in irc, taxonomy.module has a crufty API that's not as we'd like it to be. This patch adds the hooks, it doesn't try to change every single thing that's bad which is already there. However, passing $form_values to the save hook would be bad in itself, so I fixed that.
As soon as this goes in, I'll post issues for fixing up the vocabulary hooks and removing hook_taxonomy completely, and a parallel issue to make taxonomy_save_term($array) into taxonomy_term_save($object).
Comment #18
catchThere's already an issue for renaming taxonomy_del_term etc. here #295392: DX: taxonomy_del_vocabulary/term -> taxonomy_delete_vocabulary/term. And here's one for #314147: DX: Standardise taxonomy load/save/delete functions on objects
Comment #19
catchShame on me, #10399: New 'view' option in taxonomy hook to add to display just marked as duplicate.
Comment #20
CorniI CreditAttribution: CorniI commentedI reviewed this together with catch, and it all looks fine, except that in
the ->fetchObject(); should be unnecessary, but both catch and I are sure we had already problems with leaving such a fetchObject() out, so he'll recheck with Crell. All Taxonomy tests (including the new ones) are passing fine.
Comment #21
catchtaxonomytest.module now has hidden[] = TRUE, and here it is including the testing module in the patch file after giving up on cvsdo and doing it manually.
Removing fetchObject() completely borks things, so we need clarification from Crell on this in general.
Comment #22
Crell CreditAttribution: Crell commenteddb_query() returns a result set object, just as it always did. If you want a single record from it, you do need to stick ->fetchObject() (or one of its siblings) on the end, just as in D6 you would do
db_fetch_object(db_query(...));
.Comment #23
drewish CreditAttribution: drewish commentedsubscribing
Comment #24
R.Muilwijk CreditAttribution: R.Muilwijk commentedsub
Comment #25
catchNo longer applied after the nodeapi patch, so re-rolled, with minor test cleanup.
Comment #26
catchretitled
Comment #27
dman CreditAttribution: dman commentedJust a +1 of #25 as I've found all tests pass on a new test env against HEAD
This is good stuff, lets get it in so we can start USING it!
Comment #28
catch#25 no longer applied following some commits. re-rolled.
Comment #29
bdragon CreditAttribution: bdragon commentedsubscribing
Comment #30
Anonymous (not verified) CreditAttribution: Anonymous commenteddrupal camp chicago gave me beer.
yeah so what needs to be done here? tests pass. should we also rip out synonym and hierarchy loading into the taxonomy module's implementation of this new hook?
here's a patch that does just that. we can use the new function taxonomy_term_load and get a fully constituted term object with taxonomy module's own extensions.
if you want to move this stuff to a contrib module, it's cool. but for now, here it is.
probably i should sit on this until the hook is committed... but i'm just so eager.
Comment #31
catchAt the moment there's no synonym, hierarchy or related terms loading into the term object - so I'd like to leave that for followup patches. If we start loading that information, I'd also like to allow for loading multiple terms in one query so we can offset the performance hit - see #324313: Load multiple nodes and terms at once which does that for nodes. We've had several +1s here and two people verify that the tests pass - anyone want to RTBC?
Comment #32
Dries CreditAttribution: Dries commented- Test modules should be name foo_test.module, not footest.module. See the other _test.module.s :-)
Comment #33
catchThis changes taxonomytest.module to taxonomy_test.module - otherwise the same as #28.
Just to clarify - we could potentially load synonyms, related terms etc. into the taxonomy term object in followup patches - but many of these features (even hierarchy) are used by a small subset of Drupal sites - so I'd like to consider each on their own merits in followup patches.
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commentedcatch, i follow your lead on this. i'll be ready to submit patches to separate the other attributes of terms when this patch is committed. i say RTBC.
i'm on pins and needles of excitement for this patch!
Comment #35
Dries CreditAttribution: Dries commented-
+ foreach($result AS $antonym) {
should use lowercase 'as'.- There seems to be an 'Hello world' debug statement in the code.
Otherwise looks great. Quick reroll? Thanks.
Comment #36
catchAck. Thanks Dries. Here's a re-roll which should clear those up.
Comment #37
Dries CreditAttribution: Dries commentedGreat work, catch. I committed this patch to CVS HEAD. This will need to be documented in the upgrade documentation. Once it is documented, please mark this issue 'fixed'. Thanks.
Comment #38
catchGreat!
But here's a followup already :(
When writing docs I realised I'd collapsed 'insert' and 'update' into just 'save', this is less than ideal, so attached patch changes that.
Upgrade docs and API are fixed to reflect the desired state already since I'd already committed half of it by the time I realised this, if this followup patch needs alteration for any reason I'll of course go back and change again.
Once this is fully fixed I'm going to work on #329140: Make vocabulary load/insert/update/save like terms so hook_taxonomy() can finally lay to rest too.
Tests are changed here too to reflect the changed hook name and continue to pass. Sorry folks.
Comment #39
catchComment #40
catchSlight update to put the db_delete() on one line since we don't have a nice standard for multiple line indents for these chained methods yet.
Comment #41
catchFollowup to the followup to the followup removing a stray extra line. Thanks the sun!
Comment #42
sunAll Taxonomy tests pass.
Comment #43
Dries CreditAttribution: Dries commentedOK, committed! Thanks.
Comment #44
dman CreditAttribution: dman commentedYAY.
I look forward to some joy with this. I'm already declaring and (manually) invoking the prospective hooks in taxonomy_xml, taxonomy_enhancer and (a rewrite of) Node Auto Term under D6. Porting will be a breeze!
Comment #45
NancyDruLikewise, I am thrilled at this. You can expect Taxonomy Image to be using this real soon.
Comment #46
catchback to fixed ;)
Comment #48
joshk CreditAttribution: joshk commented¡subscribamundo!