The attached patch replaces the custom function to display tokens (which is very limited) with the token_tree theme function. The disadvantage is that this only works if token.module exists but it allows to recursively browse the tokens and insert them with a click on the token.

I think most sites will have token.module installed anyway, but we could also keep the current function as a fallback...

CommentFileSizeAuthor
token_tree.patch4.15 KBberdir

Comments

simon georges’s picture

simon georges commented

I'm in favor of keeping the current function as a fall-back, even if I suspect that Token is installed quite everywhere. We don't want to introduce a new dependency, and depriving people of such functionality would be bad ;)

simon georges’s picture

simon georges commented
Status: Needs review » Needs work

Changing to needs work to keep the current function as a fallback, unless Miro thinks otherwise.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Well..
Tokens are used on many places. Other modules have the same problem: They need to provide some help even if token ui is not available.
How do they implement this?

Honestly our current code is very ugly. I just want to avoid to loose time in fixing our custom browser. If we stay with the fallback, we clearly should note that this is a low quality fallback in the code...

I think i'm fine with both situations.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Core itself has in the only place where it uses tokens (user mail templates) a hardcoded list of tokens, which is replaced by token.module by it's browser.

I know that rules.module currently has it's own systems for displaying tokens, and Privatemsg has no replacement at all.

The problem with custom replacements is that they usually don't support things like nested tokens, which makes it *a lot* less useful. I think useless enough that it can as well be left out. And implementing something that does support nested tokens would be a lot of work and a pointless thing to repeat in every module.

IMHO, it's ok to require the token.module to have a token UI, that's what it exists for. Currently, ~30% of all D7 sites have it installed (60k of 170k D7 sites), it's also a requirement for pathauto.module for example.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs work » Needs review

Well argued, berdir. So i vote for remove now.

Let's focus on extensibility, reuse and cleanness.
Review of functionality needed again / only.

simon georges’s picture

simon georges commented

Agreed. What's more, we don't definitely add a dependance on Token since you prefixed everything by if module_exists() (I think).

simon georges’s picture

simon georges commented
Status: Needs review » Fixed

Committed. Thanks!

Status: Fixed » Closed (fixed)

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