The token system has "type"ing as part of the array definition of tokens, but they don't conform to our typed data system or data definition. We should rectify this so that we can reliably get tokens for typed data objects w/o the need to convert from typed data syntax to token syntax.
Eclipse
Comment | File | Size | Author |
---|---|---|---|
#22 | drupal_2163027_22.patch | 21.48 KB | Berdir |
#18 | drupal_2163027_18.patch | 21.81 KB | Xano |
Issue fork drupal-2163027
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
EclipseGc CreditAttribution: EclipseGc commentedComment #2
jibrantest-bot:ping
pickup the patch.
Comment #3
jibranTrying status change.
Comment #4
jibranHmmm issue was posted NR. Let's see if test bot can pick it now.
Comment #5
jibranre-uploading the patch
Comment #8
jibran5: d8-token-returns-2163027-5.patch queued for re-testing.
Comment #9
EclipseGc CreditAttribution: EclipseGc commentedas I understand that particular test has issues right now unrelated to this patch. Anyone have thoughts on what I'm attempting to do here?
Eclipse
Comment #11
andypost5: d8-token-returns-2163027-5.patch queued for re-testing.
Comment #13
EclipseGc CreditAttribution: EclipseGc commentedI thought those errors were because of an issue in Views Area stuff I'd heard about. I was wrong, I think this should fix it.
Eclipse
Comment #14
XanoI went through the patch and all looks good. This will help with #2164635: Automatically expose typed data to token API.
Comment #15
dawehnerI wonder whether we can use that somewhere in core.
Comment #16
fagogreat to see that - it makes a lot of sense!
@dawehner: Not sure, but e.g. if views fields would declare the data they provide using (typed) data definitions, it could be used to generate tokens while serving as possible input for VBO actions at the same time.
Comment #17
Dave ReidI like the idea of this in principle but I don't think the implications are fully considered.
Doesn't this affect the actual token names? Or is this going to be causing a disconnect in the 'types' in hook_token_info() and the $type values checked in hook_tokens() (because I don't see any changes to hook_tokens() implementations)? If so, then that is an unacceptable DX regression.
Comment #18
XanoRe-roll.
It may. Is it a problem if it does? We can always add aliases if we don't want to break BC, can't we?
Comment #19
klausi18: drupal_2163027_18.patch queued for re-testing.
Comment #20
Berdir18: drupal_2163027_18.patch queued for re-testing.
Comment #22
BerdirRe-roll. #2287733: Configure page title per display is a first example of integrating the token and context/typed-data system and this would make it much nicer...
Comment #23
BerdirComment #24
BerdirComment #25
BerdirAnd yeah, hook_tokens() is a mess now, because that is all still using the old token names, as are the tests.
The type right now maps directly to what is used in tokens and specified as available token types for e.g. the token tree UI.
Comment #26
Berdir#1920688: Support multiple instances of the same token type in token replacement might help?
Comment #27
tim.plunkettBumping.
Comment #29
fagoAs far as I can see it does not change the keys of $types, so the connection is still the same. But then it's weird that 'type' points to data type and not a token type. The best thing wold be align them, would means renaming some token types.
The patch misses documentation for the 'type' key being added there.
Great improvement, but a BC break now. :(
Not sure whether adding primitive types brings us anything though.
Comment #31
EclipseGc CreditAttribution: EclipseGc commentedAdding primitive types lets you say "Plugin requires context of ContextDefinition('some:primitive')" and have it actually stick. You could generate a list of contexts from the primitives available from your tokens and actually figure out which ones are viable to use for context.
Eclipse
Comment #32
tim.plunkettIs there a way for us to do this without breaking BC?
Comment #33
Dave ReidNo
Comment #48
andypostLooks the issue still makes sence but needs careful re-roll
Comment #49
andypostComment #52
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #53
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
This needs an issue summary update.
What's the proposed solution, remaining tasks.
Was tagged as an API change what was that? It will need a change record.