Problem/Motivation

Many places in code use the '#theme' => 'token_tree' to render a 'Browse available tokens' link. This immediately just calls 'token_tree_link'. I am not sure why this was done earlier but it doesn't seem necessary.

Proposed resolution

Replace calls to 'token_tree' with 'token_tree_link'. This immediately makes code a bit more understandable and removes some of the confusing bits.

Remaining tasks

User interface changes

None

API changes

You should use 'token_tree_link' to render the link. '#theme' => 'token_tree' with '#dialog' => TRUE will no longer render the link.

Data model changes

None

Comments

hussainweb created an issue. See original summary.

hussainweb’s picture

Status: Active » Needs review
FileSize
2.35 KB

Here is an initial patch for review. I tested this manually across all pages that display the link and it works fine (User Account Settings page, field edit page). The dialog is loaded via AJAX and the tree items expand/collapse properly.

Berdir’s picture

This looks fine. But, while I'm usually a big fan of making small steps, it's a bit tricky here because each step might be an API change that will break other modules.

So before I commit this, I'd like to have a better idea of where to go with #2430823: Token tree rendering uses a lot of theme functions and can not be render cached and the token browser API in general. And then possibly commit a few things together or as a bigger issue, so we only break those other modules once.

hussainweb’s picture

@Berdir: I am not sure about how we would go, but this is what I have in mind:

  1. Isolate different theme functionalities and consolidate them so that there is no inter-dependency. Here, we are removing the ambiguous nature between theme_token_tree_link and theme_token_tree.
  2. Convert the theme_* functions to TWIG files. I have already started working on that and almost converted theme_token_tree_link.
  3. Move most of the functionality from theme_token_tree to a service (which might move some other code as well).
  4. We can then convert the theme_token_tree to a TWIG file as well.

I am not sure what happens after that and how it relates to render caching. I think that once the code is decoupled and simple enough. Render caching will get a lot easier.

Secondly, I am doing it in baby steps to make sure API does not break and if it does, it is clearly documented. In this case, the only change is calling theme('token_tree') with #dialog to TRUE will not work anymore. The patch already changes that in token module itself. For other contrib modules, yes, it might break but it makes sense to use theme('token_tree_link') anyway. After this, I don't see any API breaks in this part to other modules. Furthermore, if you are concerned about the break, we will just convert the theme calls and not break existing functionality. We can do that in a follow-up.

I intend to work on porting this over this week and next and try to close as many issues as possible. I understand your concerns and will respect your decisions on committing them in whatever way you prefer. Of course, if you can commit this, it will make creating further patches easier.

Please let me know your thoughts on the above.

Berdir’s picture

The problem with render caching is that ajax requests are POST, everything that now uses the dialog and therefore ajax uses POST, and in Drupal Core, you don't get render caching on POST requests. So we probably need to do caching of the output ourself, possibly by calling the render cache service directly.

I'd imagine that we want to switch to #type token_tree, then we can easily add #pre_render and similar logic and direct that to a service. So there will be more API changes there.

But you're right about that there will likely not be more changes for link unless we change that to #type too.

However, this doesn't add any new functionality. So instead of splitting this up, what if we simply fix those other modules already and then commit this all together?

Based on a quick search, that includes the following contrib modules that I know about:
* Simplenews (I'm a maintainer, so I can commit patches)
* File Entity (I can merge PR's on github)
* Google Analytics
* Pathauto (I can merge PR)
* Sharemessage (I'm a maintainer)
* Simplenews Scheduler (I'm not a maintainer but we have someone here who is)

If you can provide patches/PR for some or all of those (or just open novice tasks, as it should be trivial), I can merge/commit most of them :)

hussainweb’s picture

Thanks! I have started working in those modules. I submitted an issue for simplenews here: #2640258: Remove #dialog option to show token browser.

hussainweb’s picture

hussainweb’s picture

It looks like you have committed the changes in most modules. Only two to go.

With that, how do you think we should handle the change here? Is the current patch good or should we remove the option for #dialog in a follow-up. We can change the usages to theme('token_tree_link') in this issue.

I am calling it a day now. I will continue working tomorrow.

Berdir’s picture

The patch is fine. I'll give the other two modules a bit of time, but not too long, it's not a big deal if it doesn't work and there are RTBC patches.

I'll commit it soon, probably tomorrow or so.

Not sure how much I'll be around in the next days, with christmas and so on ;)

hussainweb’s picture

Of course, take your time. If you are around tomorrow and can talk on IRC, I would like to get a sense of direction so that I can work more efficiently.

Thank you for all your help and have a merry Christmas and happy holidays!

Berdir’s picture

Ok. I've just created the first alpha release for Drupal 8, with the idea that we can do all the related changes here until the next alpha, so that people have something "stable".

hass’s picture

I'm using it not only in ga. Piwik an realname are also affected.

hussainweb’s picture

@hass, I am happy to help wherever I can. I already submitted #2640532: Remove #dialog option to show token browser for piwik. I can't find token_tree being used in recaptcha 8.x-2.x. I believe 8.x-1.x is not used and this change won't affect 7.x. Am I missing anything?

Berdir’s picture

Status: Needs review » Fixed

Thanks for all the patches. Committed.

  • Berdir committed 1cea5ed on 8.x-1.x authored by hussainweb
    Issue #2640138 by hussainweb: Use token_tree_link instead of token_tree...

Status: Fixed » Closed (fixed)

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