Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Taxonomy vocabularies already have taxonomy_get_vocabularies() but it doesn't call hook_taxonomy_vocabulary_load(), and causes duplicate queries if called twice in one page load or used with taxonomy_load_vocabulary() - so we should just make a new taxonomy_vocabulary_load_multiple() and have that do the grunt work for both functions. No patch yet, just opening this so I don't forget.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | vocabulary_load_multiple.patch | 13.33 KB | catch |
| #8 | vocabulary_load_multiple.patch | 12.5 KB | catch |
| #2 | vocabulary_load_multiple.patch | 10.07 KB | catch |
| #1 | vocabulary_load_multiple.patch | 9.98 KB | catch |
Comments
Comment #1
catchPatch.
taxonomy_vocabulary_load() and taxonomy_get_vocabularies() are both left in as convenient wrappers around taxonomy_vocabulary_load_multiple(), but now both return proper full vocabulary objects and fire hooks appropriately.
Haven't added new tests yet, although both the wrapper functions are reasonably well tested in core already and cover much of the stuff that's not a straight copy/paste from elsewhere, as I found out when I got loads of test failures at various points of writing the patch. I'll probably add some extra tests for the specific multiple stuff later on, but marking needs review for the test bot and a general look. This might be considered overkill for vocabularies, but since we treat them as first class objects, and already have a 'multiple' function there already, it makes sense to give them the proper treatment I think.
Noticed that we don't have any tests for hook_taxonomy_vocabulary_$op yet so will open a new issue for those.
Comment #2
catchRe-rolled for changes to taxonomy table names.
Comment #4
catchRetesting.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedI've reviewed this patch. It functions as described.
Comment #6
Dries CreditAttribution: Dries commentedStill waiting for tests and some performance data.
Comment #7
catchYep. I'll work those up and set this to CNW until that's done.
Comment #8
catchAdded tests which cover the new function and ensure that taxonomy_get_vocabularies() still works.
Generated a bunch of vocabularies with devel, and gave anonymous users administer taxonomy permissions.
admin/content/taxonomy/1
HEAD:
14.07 reqs/sec
Patch:
13.94 reqs/sec
About 1% slower, some ab runs the patch was half a percent faster, so neglible, and we can probably blame db_select() if that 1% is real since it'd be consistent with what happened with node_load().
Devel says:
Patch:
Executed 28 queries in 12.95 milliseconds.
HEAD:
Executed 28 queries in 13.06 milliseconds.
(couple of milliseconds variation each way both with and without the patch so no measurable difference).
So nothing exciting, but this is more about having full vocabulary objects returned by vocabulary_load_multiple() and consistency with terms than performance, since not many pages load vocabularies.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedI've reviewed the patch. This patch is good!
Comment #10
catchCouple of minor changes - fixed a typo in hook documentation phpdoc and added type hinting to vocabulary_load_multiple().
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch still works, and I extra super-duper need it for #412518: Convert taxonomy_node_* related code to use field API + upgrade path. I'd mark it RTBC if I weren't the only one here...
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #13
mlncn CreditAttribution: mlncn commentedAlso tested. Confirming RTBC!
Comment #14
webchickLooks good here. Committed to HEAD. Thanks!
Please update this in the upgrade documentation.
Comment #15
catchThanks, added to http://drupal.org/node/224333