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.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
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | use_token_tree_link-2640138-2.patch | 2.35 KB | hussainweb |
| |||
Comments
Comment #2
hussainwebHere 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.
Comment #3
BerdirThis 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.
Comment #4
hussainweb@Berdir: I am not sure about how we would go, but this is what I have in mind:
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.
Comment #5
BerdirThe 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 :)
Comment #6
hussainwebThanks! I have started working in those modules. I submitted an issue for simplenews here: #2640258: Remove #dialog option to show token browser.
Comment #7
hussainwebI see you merged the two pull requests. I am just putting the links here for reference:
Simplenews: #2640258: Remove #dialog option to show token browser
File Entity: https://github.com/drupal-media/file_entity/pull/11
Google Analytics: #2640288: Remove #dialog option to show token browser
Pathauto: https://github.com/md-systems/pathauto/pull/101
Sharemessage: #2640274: Remove #dialog option to show token browser
Simplenews Scheduler: #2640284: Remove #dialog option to show token browser
Comment #8
hussainwebIt 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.
Comment #9
BerdirThe 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 ;)
Comment #10
hussainwebOf 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!
Comment #11
BerdirOk. 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".
Comment #12
hass CreditAttribution: hass commentedI'm using it not only in ga. Piwik an realname are also affected.
Comment #13
hussainweb@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?
Comment #14
BerdirThanks for all the patches. Committed.