Postponed (maintainer needs more info)
Project:
Token
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
10 May 2017 at 15:16 UTC
Updated:
20 May 2021 at 01:05 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
phenaproximaThis patch was extracted from #389 in #2831274: Bring Media entity module to core as Media module. It restores token support to Media.
Comment #3
berdir> This is a "must" followup because it is required for pathauto support.
As maintainer of the token and pathauto modules: Nope, it's not.
token has generic token support for content entities and their fields, including base fields. To avoid conflicts, base field support is only enabled if the module does not define any tokens on its own. So by removing the hook completely, you automatically get those tokens.
I also doubt that even one of 10 sites is interested in actually having aliases, manually or generated, for media entities. Media entities are almost always viewed only as part of some other entity, typically nodes (as field, embedded in text fields, paragraphs, ..). I don't have any numbers to back that claim, though :)
Comment #4
marcoscanoNot postponed anymore
Comment #5
seanbIf token has generic support for content entities and their fields we could probably skip this. Are there any other use-cases (that don't rely on the token module)?
Comment #6
phenaproximaDiscussed briefly on IRC with @Gàbor Hojtsy and it seems that this could use framework manager review.
Comment #7
gábor hojtsy@Berdir: sounds like token integration would help with views of media as well? (Depending on what we have tokens about).
Does token module generate these tokens already? If so are there more? If not so, are there ones missing? Also does it do the same caching metadata on the tokens?
Comment #8
xjmBased on #3, this is at least not a must-have. I'll update the roadmap accordingly. Leaving this open pending further discussion though.
Comment #9
berdirViews doesn't use the token API, it uses its own twig-like token syntax that has nothing to do with token module/API.
As I said, I don't think we need anything media specific here, at least not for the pathauto use case and not sure if there are other uses.
Comment #11
vijaycs85Probably we could update tests with media or add one and close this ticket?
Comment #12
seanbJust looked into this a bit more. Basically, every custom module that depends on the token service could use the tokens provided by the media module. Without this patch, those modules would have to depend on the token module as well. Panels, for example, uses the tokens service in a couple of places. But there could, of course, be a lot of other modules.
This makes me believe this is still something we should add. Even though the token module, pathauto and views don't need this. All other content entities in core provide basic tokens and I don't see any reason why module developers would not want to use token replacement for media.
Looking at the patch, we should probably expand the tests a bit and add tokens for the source field value and thumbnail.
Comment #13
seanbChanged issue title and summary.
Comment #14
seanbChanged issue title and summary.
Comment #15
phenaproximaMy feeling is, if Media Entity had it, core Media should as well, at least in some capacity. We shouldn't regress functionality if we can avoid it. If other content entities in core provide basic tokens, I also see no reason we should not. So +1 what @seanB said in #12. Let's get this ready.
Comment #16
seanbNew patch. Added thumbnail token and more tests.
The source field token might be complex, since source fields could be anything. In case of an entity reference it might not be clear what the token returns. Left it out for now. This could be a followup, or we could say that for source field/field map tokens you need the token contrib module?
Comment #17
phenaproximaWhen in core, let us do as core does. If core's token support doesn't support fields extensively, then we can and should delegate to Token. I'm unclear on the extent of core's token support, though...
Comment #18
seanbNode / taxonomy provide only tokens for the basic fields, not for fields added through the field UI. The source field is kind of in the middle of it and a bit of a grey area.
Comment #19
phenaproximaTrue. However, at the end of the day, the source field is a configurable field, albeit a required one. Therefore, my inclination is to delegate to the Token module.
Comment #20
berdir> All other content entities in core provide basic tokens and I don't see any reason why module developers would not want to use token replacement for media.
That's not 100% correct, block_content, shortcut, menu link, content moderation, aggregator feed/item all don't provide tokens. token.module provides hardcoded tokens for some of those, specifically menu links, but actually not through the content entity api to support all menu links.
I still think we could spend our time on more meaningful things than this, like trying to get the generic token support for fields into core, but if you think it's useful, fine with me ;)
Keep in mind that there is no token UI without token module, so really almost nobody uses tokens without token.module as they wouldn't know which tokens they could actually use..
Comment #21
seanbMarcoscano summed up the plan for this issue real nice in today's media meeting:
1) Review if no token from ME 1.x is left behind.
2a) If there is one, we need to implement it in this issue (or in [another] specific contrib module for the upgrade path?)
2b) If there is none, we can close this as won't fix and just show a message during the upgrade path suggesting to install the token module.
3) Open a new follow-up issue to discuss generic entity tokens in core.
Comment #22
marcoscanoSpent some time comparing the tokens available, for the same site, before and after the upgrade (i.e. ME 1.x versus Media in core with the tokens module enabled).
(Note I also had the tokens module enabled in the first case, to make the comparison easier through the UI. As stated in #20, this is probably the most common scenario for people using tokens anyway due to the only way of getting them visible on the UI.)
Here are some screenshots of the 1st-level tokens available for each scenario:
Media Entity 1.x
Media in core (+ tokens module)
Some new tokens are available after the upgrade, but also some others are missing:
1)
[media:edit-url]is not available anymore2)
[media:bundle-name]is not available anymore (but probably available as well as[media:bundle:entity:name], or something similar?)3)
[media:type]is not available anymore-> This is probably the only that is truly media-related, because the (now called) source plugin being used is something that the token module doesn't expose so explicitly, and in fact it also should now be called
[media:source]instead. This token is also missing from the patch, so if we go the route of including them in core, this needs to be added as well.4) ME 1.x declared
[media:created]and[media:changed]as date types, so formatted-values tokens were available before and are not available anymore:Before:

After:

Based on all that, I am not sure the easiest path forward, handling well the upgrade paths of sites that were potentially using them.
The options I see are:
A) Add the Media tokens to core in the media module (this patch), probably including the missing
[media:source]. Also add a note in the documentation indicating that they need to update their sites, replacing[media:type]by[media:source], once the former means a different thing in core.B) Don't include anything. Explain the differences in the documentation, and probably also write some snippets showing how the missing tokens can be added in a custom module so users can use that as a starting point.
C) Create yet another contrib module to provide the missing tokens, and instruct users on the documentation that, if they used any of the missing tokens, after the upgrade they can install that module and get them back.
I would personally prefer option A), it seems the most hassle-free for users who are upgrading.
Opinions?
Comment #23
chr.fritschThanks @marcoscano for investing in this.
For me, the most logical option would be to move the token discovery from token (contrib) into core and extend the discovered tokens then with the missing ones from media_entity.
But i think this could be a follow-up.
For now we should go with option A) and when my suggested follow-up lands, we could reduce the number of provided tokens in media.
Comment #24
marcoscanoYes, I agree that we need a short-term solution for the upgrade paths of sites using Media Entity 1.x, and this patch is probably the best one.
I have opened #2929255: Support generic content entity tokens to discuss moving generic entity tokens from the token module into core, and removing all specific tokens that already exist.
Was going to complete the patch in #16 with the missing
[media:source], token, but just discovered that the current[media:type]from ME 1.x is broken! so there is definitely no existing sites using it, and I personally don't think we should create a new one in its place. (IMHO it's too technical for a practical use in a token context)Also, I have manually tested the patch in #16 and it works fine:
With that, I'm RTBC'ing #16, in the expectation to hear other's opinions still.
Comment #25
seanbIt looks like the easiest way forward would be to get this patch in and fix #2929255: Support generic content entity tokens after that. Thanks marcoscano!
Comment #26
wim leersSome nits.
Unless this is necessary for BC, it should be a relative URL, and be wrapped in
file_url_transform_relative().I realize this is what e.g.
Nodetokens also do, but we can do better here now:That'd then ensure nothing is able to slip by.
Comment #27
marcoscanoThis should fix #26.
Comment #28
gábor hojtsyI guess the hard part about this issue is some tokens are provided by token module but not all the tokens we need, however we want an upgrade path soon, so fixing it more generically is a headache (AKA not feasible in this short term). Was the option to let the token module define some tokens and add on top of that considered or is that not technically possible? (With a quick google, hook_token_info_alter() is maybe what I have in mind, but have little experience around defining tokens myself).
Comment #29
marcoscanoRe #28, I believe it could indeed be a viable option, thanks for bringing it up @Gábor Hojtsy!
Please correct me if I'm wrong, but I believe the attached patch would be what that solution would look-like, more or less.
In my opinion however, I'm not convinced it would be a good solution, because:
- It won't eliminate the need for
media.tokens.inc- We would have hooks there that only partly implement / tweak tokens, being inconsistent to what other core files do, such as
node.tokens.inc,comment.tokens.inc, etc- This part:
feels weird, we are doing something in core if a contrib module is enabled, that doesn't seem to happen often.
It's true it's less code we would be adding to core, but IMHO in a less consistent way, so I'm not convinced the pros are bigger than the "cons".
Another viable approach, on the other hand, would be to have all media tokens in a (yet another :)) new contrib module, just for the interim / upgrade path. It's the same approach we have used for actions (with Media Entity Actions) and the "Generic" source plugin (with Media Entity Generic), so it's a quick solution we could use as a last resource. However, the more contrib modules we throw to the mix, the more complex the upgrade path becomes, so I'm not really a fan of this option either...
Comment #30
gábor hojtsyI somehow mixed up the location of said token module. It is absolutely not ok to put conditional code on contributed modules in core modules, so scratch my idea. Sorry for derailing the conversation.
Comment #34
catchDo these really get used? The Token UI can get cluttered at best, and I think we should only add tokens we expect people to use, it's easy enough to add additional tokens via contrib.
Comment #35
marcoscanoRe: #34:
They were available in Media Entity contrib, and we want to ensure people upgrading existing sites to Media in core won't find anything missing. It's true it's very unlikely sites out there were using those, but we can never be 100% sure.
The alternative would be to have a message during the upgrade path saying "Hey if you used tokens X and Y you should install the contrib module Z to keep using them.", but if we were in that scenario, maybe it would be worth having all media tokens in a contrib module then?
Comment #36
catchI don't think this is an upgrade path issue, small changes from the contrib to core module are fine, we just want to ensure data is preserved and there are feature equivalents - an individual token isn't a feature as such.
Comment #37
xjmYeah @catch and I are on the same page on this; I don't think we should add these tokens to core just to support the upgrade path. However, I also understand the concern about adding yet-another-contrib-module and making the upgrade process more complex for contrib media_entity users.
So, how about we:
@phenaproxima and I discussed this and thought that might be the best approach. If the needed tokens can be added to Token, then that meets the upgrade path's needs without adding features that don't necessarily belong in core.
Comment #38
phenaproxima@xjm and I spoke to @Berdir on Slack and he agreed to accept a patch against Token for this functionality. So this is going to move into Token and be refactored.
Comment #39
marcoscanoStill a very WIP, only worked on the
edit-urlone, which I do think could be a good addition, generically for all entities.I'm not sure how far we should go with this though. Some media-specific tokens are either broken in Media Entity 1.x (see #24) or available as a different token, so there isn't much else left to match here. For example, the timestamp basefields: should we bother changing all of them? or alternatively, hacking around something specific only for media? I'm not sure we should do either of those.
Comment #40
kumkum29 commentedHello,
do you have news about the implementation of tokens media in drupal 8? (without patch...)
Thanks.
Comment #41
chris matthews commentedHello all, can anyone provide an update re: the status of this issue? Currently, the file directory path for core media images is: [date:custom:Y]-[date:custom:m]. I would like to change the file directory path to: media/images/[media:field_media_image_category]/[date:custom:Y]-[date:custom:m], but when I try I get: File directory is using the following invalid tokens: [media:field_media_image_category].
Comment #42
chris matthews commentedCurious if anyone has made any progress with this or knows a workaround. Basically, instead of using the default '[date:custom:Y]-[date:custom:m]' subdirectory for each file based media type I'd like my media directory paths to be:
media/audio/[date:custom:Y]-[date:custom:m]
media/document/[date:custom:Y]-[date:custom:m]
media/image/[date:custom:Y]-[date:custom:m]
media/video/[date:custom:Y]-[date:custom:m]
Comment #43
rwilson0429 commented@chris_matthews. Have you tried Filefield Paths module.
From the project page:
Comment #44
chris matthews commentedYeah, just didn't want to install another contrib module.
Comment #45
andrew answer commentedHello all,
I adapt patch from #27 for Token module. Please check.
Comment #46
andrew answer commentedFixed paths.
Comment #47
damienmckennaAwesome, thank you Andrew!
What test coverage would be useful for this?
Comment #48
andrew answer commented@DamienMcKenna, you can take test from patch #27 too, I don't adapt tests for contrib modules previously.
Comment #49
berdirtoken should provide tokens automatically for media, just like any other entity that has no tokens defined on its own. The only reason for that not to work would be something interfering with it and provide some tokens.
Comment #50
tiburi commentedOh, nothing changed after the patching :(
This is the media token what i want to use [media:field_media_image:my_img_style:url]. I found in the tokens. But only this option is not enough because there is not reference to the node. I tried this [node:field_my_content_type_media_image:entity:field_media_image:my_img_style:url] pattern using one of D7 solution but nothing changed.
Comment #51
berdirSee #49.
Comment #52
lucasantunes commentedRe-rolled #46 + added an initial implementation of the source field token. Feel free to improve it! Not sure if we need to expose nested token info for it, though, since it can be of multiple types.
It served us well, since we had an issue where we needed a common token for media fields that reference multiple media types with divergent source field definitions.
Usage examples:
[node:field_media:entity:source-field:entity:url][node:field_media:entity:source-field:alt][node:field_media:entity:source-field:width][node:field_media:entity:source-field:thumbnail:url]