Lots of unnecessary querying for custom tokens

This is still a problem for the 8.x-1.x versions (for Drupal 9/10) too. Patch attached!

I'm a bit amazed to see this issue never got noticed in the Drupal 7 days; maybe because it wasn't ever set to 'Needs review', sorry about that. I could probably help maintain this module if help is needed?

+++ b/token_custom.module
@@ -43,17 +44,60 @@ function token_custom_token_info() {
+    drupal_static_reset(__FUNCTION__);

Is this a bit of debugging code that's been left in?

Why do you need to reset the static variable otherwise?

Is this a bit of debugging code that's been left in?

Ohh no, just a plain mistake. TokenCustomType::postSave() does it where it needed to.

Seems to work nicely, and saved the queries for me :)

Here's a graph of the minor performance improvement this change brought on our production systems.

The average call time for token_custom dropped from 1.1ms to 0.08ms

I'll prepare a MR and fix this tomorrow. Thank you all!

MR from #4 is ready!

https://git.drupalcode.org/issue/token_custom-2731425/-/merge_requests/4

anybody → changed the visibility of the branch 2731425-lots-of-unnecessary to hidden.

@steven jones there's still a @todo in code:

@TODO Only return types with tokens that actually exist

How should we proceed here?

@anybody It's up to you really :)

Does the TODO make sense?

To unpack it a little, there's a performance hit in the following edge case:

• A site has lots of custom token types.
• Those custom token types are used as tokens that should be replaced in lots of strings all over the site.
• There aren't actually any custom token replacement values for those types.

I feel like this is a pretty narrow edge case, but certainly might be wrong about that. People do weird things eh?
But given that the performance gains are pretty marginal here, it might be fine to leave it too.

I would suggest that you could leave the TODO in the code, and that would be fine, or decide that you'd never resolve the TODO, given the extra code complexity it would introduce and thus remove the TODO.

@steven jones thanks for the quick reply. I am very undecided. Code complexity vs. potential edge-case improvement. I'm fine with the code in general.

We don't have this case on any site, but I don't know how many users might run into this. I'd like to leave the final decision to you.

In that case, I'll mark it as RTBC as it works nicely in 99.9% (probably) of use-cases :)

Thanks, ok then let's keep it like this.

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