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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Slight 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.

pwolanin’s picture

quick look, I support the idea, but don't like some of the code. For example:

+function taxonomy_invoke_taxonomy_term_load(&$term) {

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:

+      $term = (array) taxonomy_term_load($tid);

Either make it an array all the way (e.g. menu links) or an object all the way through.

catch’s picture

catch’s picture

Trying 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.

catch’s picture

Last patch of the day.

catch’s picture

Status: Needs work » Needs review
FileSize
9.16 KB
5.72 KB
20.39 KB

While 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.

Status: Needs review » Needs work

Patch failed to apply. More information can be found at http://testing.drupal.org/node/14278. If you need help with creating patches please look at http://drupal.org/patch/create

catch’s picture

Status: Needs work » Needs review
FileSize
20.52 KB

Umm, wrong patch, hopefully this one will make DrupalTestBedBot happy.

dman’s picture

Use 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.

catch’s picture

FileSize
6.79 KB

Here'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.

catch’s picture

And 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.

catch’s picture

FileSize
6.04 KB

Don't call hook_taxonomy_term_delete twice.

moshe weitzman’s picture

I 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.

catch’s picture

That 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.

catch’s picture

FileSize
6.04 KB

This patch does that, otherwise unchanged.

Crell’s picture

From 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.

catch’s picture

As 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).

catch’s picture

catch’s picture

CorniI’s picture

I reviewed this together with catch, and it all looks fine, except that in

$terms[$tid] = db_query('SELECT * FROM {term_data} WHERE tid = :tid', array(':tid' => $tid))->fetchObject();

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.

catch’s picture

FileSize
12.38 KB

taxonomytest.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.

Crell’s picture

db_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(...));.

drewish’s picture

subscribing

R.Muilwijk’s picture

sub

catch’s picture

FileSize
11.99 KB

No longer applied after the nodeapi patch, so re-rolled, with minor test cleanup.

catch’s picture

Title: EOL Taxonomy sprint: Add hook_taxonomy_term_load » EOL Taxonomy sprint: add proper taxonomy term hooks

retitled

dman’s picture

Just 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!

catch’s picture

FileSize
11.99 KB

#25 no longer applied following some commits. re-rolled.

bdragon’s picture

subscribing

Anonymous’s picture

FileSize
5.92 KB

drupal 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.

catch’s picture

At 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?

Dries’s picture

- Test modules should be name foo_test.module, not footest.module. See the other _test.module.s :-)

catch’s picture

FileSize
12.02 KB

This 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.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

catch, 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!

Dries’s picture

Status: Reviewed & tested by the community » Needs work

- + 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.

catch’s picture

Status: Needs work » Needs review
FileSize
11.98 KB

Ack. Thanks Dries. Here's a re-roll which should clear those up.

Dries’s picture

Status: Needs review » Needs work

Great 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.

catch’s picture

Status: Needs work » Fixed
FileSize
2.55 KB

Great!

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.

catch’s picture

Status: Fixed » Needs review
catch’s picture

FileSize
2.74 KB

Slight 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.

catch’s picture

FileSize
2.68 KB

Followup to the followup to the followup removing a stray extra line. Thanks the sun!

sun’s picture

Status: Needs review » Reviewed & tested by the community

All Taxonomy tests pass.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

OK, committed! Thanks.

dman’s picture

YAY.
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!

NancyDru’s picture

Status: Fixed » Needs review

Likewise, I am thrilled at this. You can expect Taxonomy Image to be using this real soon.

catch’s picture

Status: Needs review » Fixed

back to fixed ;)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

joshk’s picture

¡subscribamundo!