system_token_info and system_tokens() only provide the 'short', 'medium' or 'long' type tokens, but do not use system_get_date_types() to define what tokens are available. Therefore any module-defined or custom date types do not get tokens. I'm working on adding this support in Token.module for Drupal 7 in the meantime, but we should fix this is core as well.
Comment | File | Size | Author |
---|---|---|---|
#78 | 1173706-78.interdiff.txt | 1.66 KB | franceslui |
#78 | 1173706-78.patch | 7 KB | franceslui |
#76 | 1173706-76.patch | 7.13 KB | larowlan |
| |||
#76 | interdiff-e398d5.txt | 958 bytes | larowlan |
#75 | 1173706.patch | 7.02 KB | larowlan |
Comments
Comment #1
Dave ReidToken.module patch for D7 for review/commit.
Comment #2
Dave ReidCommitted #1 to Git: http://drupalcode.org/project/token.git/commit/cbe724f
Moving back to the Drupal core project to fix in D8 (and consider for D7 backport).
Comment #3
Dave ReidClassifying as a bug as core only supports the three default date types.
Comment #4
lyricnz CreditAttribution: lyricnz commentedThis is somewhat related to the (D6) issue at #593840: Include tokens for custom date formats for date fields which created new tokens for each of the custom date formats and date format types.
Comment #5
Dave ReidComment #6
Dave ReidComment #7
Xano#2: 1173706-date-custom-formats.patch queued for re-testing.
Comment #9
Dave Reidsystem_get_date_types() is gone in D8. This no longer applies and needs reroll.
Comment #10
Dave ReidRelevant change notice: http://drupal.org/node/1876852
Comment #11
klonos...tagging ;)
Comment #12
pwieck CreditAttribution: pwieck commentedWill this work for D8? Used this issue as guide: https://drupal.org/node/1876852
Replaced: D7 system_get_date_types()
With: D8 system_get_date_formats()
Warning: I am a novice
Comment #13
pwieck CreditAttribution: pwieck commented#12 passed and ready for review, testing, rework
Comment #14
XanoComment #15
klonosCan someone please update the issue summary with steps to reproduce this bug so I can test the patch provided and report back.
The truth is that I really want to see this be backported to date 7.x ASAP and the way to do that is fix it in D8 first.
Comment #16
Dave Reid@klonos: This has already been provided by the token module in D7.
Comment #17
klonosOk, I just checked and feel really stupid about the fact I never noticed that before. Thanx Dave (not for making me feel stupid - for pointing the thing).
Still I wish to help with testing so we can get this in D8, so STR in the issue summary would help.
Comment #18
filijonka CreditAttribution: filijonka commentedComment #19
filijonka CreditAttribution: filijonka commentedComment #20
filijonka CreditAttribution: filijonka commentedComment #21
jhedstromComment #22
Dave ReidComment #23
filijonka CreditAttribution: filijonka commentedsorry @Dave Reid thought your response to #15 meant we already had it in D7 and didn't need backport.
Comment #24
areke CreditAttribution: areke commentedComment #26
filijonka CreditAttribution: filijonka commentednot sure what needs to be tested on this.
Comment #28
jhedstromPatch needs rerolling. For adding a test, hook_token doesn't appear to be tested at all at this point. There are integration tests for token replacement though, so the
system_tokens()
function could potentially be tested from there.Comment #29
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedRerolled the patch as per the latest D8 release.
Comment #30
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedRemoving Need reroll tag.
Comment #32
BerdirYes, we need to create and test an additional date format.
We also need to make sure that adding a new one clears the token cache, see token_form_alter() in token.module. So create one in the UI and then ensure that the token info for it exists.
I think we also need to add cacheability metadata for the replaced tokens based on the used date format.
Comment #35
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedAll,
any updates on this issue please?
Thanks,
Sukanya
Comment #39
joelpittetHere's a re-roll + I replaced the
t('@name'
because the string is not translatable like that, the label would need to be translated already.Also changed the logic a bit in
system_tokens
and adds the cachable metadata that @berdir mentioned.Comment #40
BerdirThis is also needed, but it's only about caching the results of a token replacement.
It does not affect what is cached in the token info (which is based on system_token_info()). That doesn't support cache tags, you need to explicitly call \Drupal::token()->resetInfo();. See token_date_format_insert()/token_date_format_delete()
What we should do is extend \Drupal\Tests\system\Kernel\Token\TokenReplaceKernelTest::testSystemDateTokenReplacement(), with token replacement calls for configurable date formats, but also test \Drupal::token()->getInfo(), make sure it contains the date formats, then create a new one, then call getInfo() again and it must return the new one. additionally, we could also test the cacheable metadata that is returned, similar to to testSystemSiteTokenReplacement().
Comment #42
joelpittetI've extended
\Drupal\Tests\system\Kernel\Token\TokenReplaceKernelTest::testSystemDateTokenReplacement
to test all date format tokens and a small assert to make sure we are testing "something" with a check for the 11 date formats.I added a couple system_date_format_insert/delete() with the
\Drupal::token()->resetInfo()
call. And a test forHope I got all the things, thanks for the review @Berdir.
Comment #44
Berdirunlike the token module, you can actually implement that directly in the date format entity class instead of adding hooks.
those seem to be wrong/unrelated includes.
that doesn't seem to be the right exception class ;)
Comment #45
joelpittetWhoops, that's like leaving `dpm()` in the code... got a bit alt-enter happy in PHPStorm. Oh nice I don't have to use the hooks!
Re 1. Is it on DateFormat entity that I add the hooks or do I need to extend ConfigEntityStorage?
Comment #46
BerdirDateFormat
Comment #47
joelpittetTook a stab at it, not sure if I should use dependency injection here and stuck the code on the
save()
method instead ofinsert()
which doesn't exist on that class.Comment #49
BerdirYou want postSave() (with if (!$update)) and postDelete(). save() is just a wrapper for $storage->save($entity) and isn't guaranteed to be called.
DI is not possible in entity classes.
Comment #50
joelpittetpostDelete
is static andpostSave()
is not 😖Comment #52
joelpittetComment #53
jhodgdonThere was a slack request to review this, and I had a few minutes to spare, so I took a quick look at this patch. A few comments -- just noting that I didn't test the patch, I only looked at the code.:
a) The latest test found a coding standards violation:
b) I think this should most likely be on 8.8.x. It may be backported on commit, but my understanding is all issues are 8.8 now.
c) Nitpick -- in the system_token_info() function:
Why use double quotes here? I think we usually want to use single quotes unless double are really needed, and I don't see that they're needed here. I guess it is just left over from the copy/paste of existing lines from that function, such as
d) In the system_tokens() function: It looks like there could be a problem in the unlikely event that a date format ID is either 'since' or 'raw', because it would load the format instead of getting to the switch statement cases that are still in the code. Is it possible for either of those to be a date format ID?
e) Also, what happens if you call ->load($id) on an ID that doesn't exist? Because 'since' and 'raw' would be passed in... let's see... I guess it would be OK as long as those formats don't exist in the database. Looks like ->load() returns nothing if the ID doesn't exist. So, you can ignore this. ;)
f) In the test, nitpick:
That comment should be "Add new formats" not "Add new tokens", and then a bit farther down:
Same, should be "Delete a format" I think?
Comment #54
joelpittetThanks for the review @jhodgdon!
Comment #55
jhodgdonLooking good!
I realize I have a few more questions:
a) In the DateFormat entity class:
Is it possible for an update to change the ID of the format? If so, then even on an update, we would still want the token info to be replaced.
b) Same method -- this method is not static, so shouldn't the token service be injected rather than using a call to \Drupal::token()?
c) In the info() hook -- can the existing code that added the tokens for short, medium, and long formats be removed? Because it seems like this part you added
should include those 3 built-in formats? [If not, then the code in the hook_tokens() implementation wouldn't work either?]
d) One last comment nitpick, at the bottom of the test:
That doesn't look like a count to me. :) Probably better "Verify the token was removed."
Comment #56
joelpittet@jhodgdon Sorry for the delay getting back to this. Thanks again for the review.
Comment #57
jhodgdonI just did a quick test, and you are correct -- at least from admin/config/regional/date-time there is no way to change a date format ID once the format has been created.
And I agree about the injection... at least, I see in methods like EntityBase::create() it calls \Drupal methods. Ugh. But at least consistent.
So, it looks like all of my review comments have been addressed. I think the code looks good!
Comment #58
jhedstromIt seems all the feedback above has been addressed. I think this looks good to go! Will be great to get this long-standing issue fixed!
Comment #59
alexpottI'm not sure I agree with @Berdir in #44 - I think this should be done as entity hooks as the tokens are defined by system module and this is core library entity.
We need to manipulate the name here because we have potential clashes. I.e. is you use
custom
,since
orraw
as data format names.Another tricky thing is that we can't rename
short
,medium
orlong
.Fun times.
Earlier comments say
Unfortunately the following code works.
Better to load all the date formats outside of the loops and check if the token name is in the array.
Comment #60
hussainwebDoing a rebase first.
Comment #61
hussainwebSince this is a bug-report, I am not sure if I should continue to target 8.7.x. If you think we should, please change it back. I only changed it because I rebased this on 8.8.x
Comment #66
darvanenNeeds patch (or MR) against 9.3.x
Comment #67
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedPatch for 9.3.x. Please review.
Comment #69
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedIn the last patch #67, Forgot to add class "Drupal\Core\Datetime\Entity\DateFormat" in "TokenReplaceKernelTest.php" file.
Fixed the fail test issue because of the above miss. Please have a look.
Comment #70
darvanenFeedback at #59 from framework manager yet to be addressed.
Comment #72
DamienMcKennaWe've hit a problem with Metatag that appears to be related to this: #3271006. The error is:
FeyP was able to reproduce it locally and saw it stemmed from the image field on the Article content type that comes with the standard install profile having a default path which creates a per-date directory for storing files, and for some reason it hits this error in certain circumstances.
Comment #75
larowlanReroll for 9.5.x
Comment #76
larowlanYeah I messed that up
Comment #78
franceslui CreditAttribution: franceslui at The University of British Columbia commentedI re-roll it to 10.2.x.
Comment #79
smustgrave CreditAttribution: smustgrave at Mobomo commentedCould the issue summary be updated to the standard template.
Also recommend opening an MR as those are quicker to review and test.
Changing back to 11.x as the current development branch.