Problem/Motivation
Entity token implementations do not consider that base fields may be translatable. node.tokens.inc only considers body and summary to be multilingual, not title, author, changed or created. (URLs are generated appropriate for the language though). taxonomy.tokens.inc does not consider any of the fields translatable, eg. title, description, URL, etc. are not displayed in the appropriate language.
Proposed resolution
1. Compile a list of affected tokens.
2. Load the appropriate translation like node.tokens.inc for body and summary.
3. Tests.
Remaining tasks
Review all tokens. Fix. Tests.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#155 | 2529182-155.patch | 18.56 KB | corneboele |
#152 | 2529182-152.patch | 18.52 KB | Suresh Prabhu Parkala |
#137 | interdiff_136-137.txt | 2.21 KB | nikitagupta |
#137 | 2529182-137.patch | 19.42 KB | nikitagupta |
#136 | 2529182-136.patch | 19.48 KB | nikitagupta |
Issue fork drupal-2529182
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
andypostRelated issue need review for a long time
Comment #2
Gábor HojtsySince we have #1885962: Comment tokens should use entity translation API for comments, rescoping this for node and taxonomy. Should be rescoped as more are found. @andypost do you know of other issues on multilingual tokens?
Comment #3
andypostno other issues, but it makes sense to have common test class for them all
Comment #4
SweetchuckThis is what I have done so far. Tests for translated tokens are still missing.
Comment #5
Gábor HojtsyLooks good to me. I agree tests are needed :)
Comment #6
thenchev CreditAttribution: thenchev at MD Systems GmbH commentedWanted to see if this is a starting point for tests.
Comment #7
legolasboWorking on extending these tests.
Comment #8
thenchev CreditAttribution: thenchev at MD Systems GmbH commented@legolasbo Hi. I finished some part of the test. If you didn't do much work maybe its ok of I take over this part?
Comment #9
legolasbo@Denchev,
I've changed your patch to reduce code duplication and actually test for correct token replacements. Feel free to take over from here. It still needs to test for the token replacement of the other fields.
After uploading this patch I'll do a code review of the patch from #4.
Comment #10
BerdirYes, that last patch is definitely an improvement.
This is about testing tokens, not the node UI. We should be able to limit it to only use the API, just create a node with Node::create(); save, and then ->addTranslation('it') or so, save that too. Then generate tokens.
Comment #11
legolasboPatch didn't apply. Performed a basic reroll and removed some scope creep caused by array reformatting while fixing merge conflicts.
Comment #12
legolasboComment #14
vijaycs85Fixing failing test on #11
Comment #15
thenchev CreditAttribution: thenchev at MD Systems GmbH commentedThe tests will probably pass but its not correct yet.
Need some feedback on translating the author, and for some reason the "changed" and "created" using the service date.formater and passing the langcode didn't translate the string correctly its still in English.
Comment #16
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedWhen we created the node, we set the user id to
$this->user->id()
. Can we use here$this->user->getUsername()
instead ofUser::load($fields['uid'])->getUsername()
?Can we just modify
verifyTokensCorrectlyReplaced()
method, pass the required values (e.g. mapping of token names - field values) and call it once as a helper method instead of callingassertEquals()
multiple times? I guess it will be more readable.Comment #17
legolasboworking on this.
Comment #18
legolasboThis patch addresses 16.2
Comment #19
legolasboAuthor translation is now properly tested. Also had a look at the created/changed fields. They are being translated properly, but the italian translations don't seem to be imported when enabling a language in the test. The date string is therefor still in English, because it falls back to English. The behaviour of the translation api is however thoroughly tested, so I think we are fine like this.
I removed the following because we don't need to test the node/translation creation. We only want to test the token replacement.
Comment #20
thenchev CreditAttribution: thenchev at MD Systems GmbH commentedAdded term tests
Comment #21
Gábor HojtsyUpdates look great to me :) Thanks all for working on this!
Comment #22
alexpottMaking this change makes the patch way harder to review.
Missing docs
Use SafeMarkup::format() - format_string() is deprecated.
Afaics this has not changed a thing.
Comment #23
legolasboOn this!
Comment #24
legolasbo22.1 Reverted that out of scope change and also some additional out of scope code style changes. Greatly simplified the patch for review.
22.2 Documentation added
22.3 done
22.4 Did not change a thing indeed. Reverted the change.
Comment #27
legolasboOops. forgot to restore $replacements variable.
Comment #30
legolasboI forgot Simpletest tries to run every function which starts with test. Should pass now.
Comment #31
andypostI see no need in that
needs empty line between
needs description
needs {@inheritdoc}
string $langcode
and both needs description
should be full namespace and needs description
NodeType::create([..])
should be one-liner
needs proper indent, looks like copy-paste
Comment #32
thenchev CreditAttribution: thenchev at MD Systems GmbH commentedShould address all from #31.
Comment #33
legolasbos/language/languages
Exceeds 80 characters.
Exceeds 80 characters
Exceeds 80 characters
Exceeds 80 characters
This codestyle change makes the patch much harder to review and should be reverted.
Comment #34
thenchev CreditAttribution: thenchev at MD Systems GmbH commentedAddressed 1-5 and some styling errors that i noticed. Will maybe take a look later at the big one.
Comment #35
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented#33.6 needs to be addressed.
Comment #36
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedComment #37
thenchev CreditAttribution: thenchev at MD Systems GmbH commented#33.6 Reverted changes, hope i understood that right
Comment #40
legolasbo@Denchev,
Patch no longer applies and needs a reroll, also found more unneeded codestyle changes.
Codestyle changes, add review noise.
Use shorthand array notation for all newly introduced/updated arrays
let's put this on a single line
Codestyle changes add review noise.
Codestyle changes add review noise. And also seems to have introduced an error since
$bubbleable_metadata
is now nolonger set.Comment #41
thenchev CreditAttribution: thenchev at MD Systems GmbH commentedLets try to make the patch apply. I will continue to fix whats breaks.
Comment #43
alvar0hurtad0Reroll of #34
I was not really sure about what patch should I reroll.
Comment #45
thenchev CreditAttribution: thenchev at MD Systems GmbH commentedSo I went over the changes from the start and there might be some changes missing, not sure. Here I also tried fixing NodeTokenLanguageTest and NodeTokenLanguageTest by adding some changes to tokens.php. Was wondering if thats ok
Comment #47
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedhook_tokens() is not responsible for sanitization any more (see https://www.drupal.org/node/2578365 for more info). This effectively means that we don't have
$options['sanitize']
any more and we have to remove it from patch entirely.Comment #48
visabhishek CreditAttribution: visabhishek at Azri Solutions commentedRemoving $options['sanitize'] from #45 patch.
Comment #50
BerdirTry to remove as many unrelated changes as possible.
All those changes are unrelated. You need to drop them, they're incorrectly merged from an updated patch.
Comment #51
legolasboOn this for a while
Comment #52
legolasboI've removed a whole bunch of unrelated changes and all sanitize = true/false i could find. Several tests are still failing but I am out of time. Let's see how testbot thinks about it anyway.
Comment #54
alvar0hurtad0IMHO "needs reroll" tag is not needed
Comment #55
legolasboWorking on this some more.
Comment #56
legolasboFixed the broken tests and removed some more unrelated code. Also found that the tests don't provide full test coverage where it concerns bubbleable metadata. Will improve test coverage some more.
Comment #57
legolasboLet's have testbot take a look to see if I missed something.
Comment #60
legolasboThis should do it.
I introduced
AssertTokenReplacementTrait
to reduce code duplication and ensure all token replacements are tested in the same way and also extended the cacheability metadata testing inTokenReplaceTest
to cover all tested tokens.Comment #61
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedThis seems like an unrelated change.
This should be typehinted as \Drupal\user\UserInterface[] not null.
Would also make sense to make it protected instead of private.
Is it necessary to have two identical users?
Comment #62
legolasboThanks for the review.
61.1 - Could be unrelated, I don't know why it was added, so I guess we could try reverting the change.
61.2 - You are correct, should be changed.
61.3 - Yes it is necessary to have two "identical" users, because they will differ by username which is what we need to make sure they are properly replaced in different languages.
Unfortunately I don't have time to work on this until Friday at least.
Comment #63
thenchev CreditAttribution: thenchev at MD Systems GmbH commented@slashrsm Thanks for feedback
Addressed 61.1 and 61.2
Comment #65
thenchev CreditAttribution: thenchev at MD Systems GmbH commentedSorry. Missed the gitignore. Here is a new fixed patch
Comment #66
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedLooks good to me.
Comment #69
andypostWould be great to get this in and re-use the same trait for #1885962: Comment tokens should use entity translation API
Comment #71
legolasboTest failures seem unrelated. Back to RTBC while Retesting.
Comment #74
andypostremoved outdated
Comment #76
Gábor HojtsyComment #77
alexpottThis does not appear to be used.
Out of scope changes.
Why are we making the change to the protected property type doc? The method says it is returning the VocabularyInterface
Whilst these changes look correct I'm also not sure that they are in scope for the issue.
Comment #78
thenchev CreditAttribution: thenchev at MD Systems GmbH commentedShould cover everything from #77
Comment #79
Gábor HojtsyYay, thanks!
Comment #80
alexpottThis looks wrong... I think it should be
$langcode = isset($options['langcode']) ? $options['langcode'] : NULLl
. But looking at the node code don't we need to set the language option for term urls?Comment #82
gngn CreditAttribution: gngn at Computer Manufaktur GmbH commentedI think alexpott is right:
smells funny.
I tried it with the proposed
and it worked for me.
Comment #83
Berdirconfig entities don't support this API, so this doesn't actually do anything.
it is more complicated for those, you need to switch the config translation override language and then switch it back, see user_mail()
Comment #84
gngn CreditAttribution: gngn at Computer Manufaktur GmbH commentedSorry - I just tested the 'term:parent' token. That worked.
Comment #86
eiriksmGoing to look at this today
Comment #87
eiriksmOK, so did a reroll. And tried to address comments in #80 and #83.
Trying to find a way to test my changes manually, but let's see if it passes first.
Comment #88
eiriksmComment #89
eiriksmHm, feel like this is more correct.
The token replacements works as I expect, but not sure about if I am testing all cases.
Comment #90
Gábor HojtsyWe don't seem to be dealing with a possibly translated vocabulary. I am fine with this if we want to not include this in the scope, but let's be clear about it.
Comment #94
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedHello, guys!
The patch #89 was no longer cleanly applying to 8.5.x so I attach its reroll. Had to manually update a few things, mostly it was about short-array syntax and a couple of test files moving from simpletest to phpunit. As far as functionality, I have changed no thing.
Comment #95
yogeshmpawarTriggering test bots.
Comment #96
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedI am not sure why it failed. I ran that test locally through phpunit and through run-tests.sh both times successfully. Let me requeue it in case it was one-off failure.
Comment #98
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedehmmm, I am not sure why it fails, the error message really says nothing particular and it does pass on my localhost. At the moment I do not have time to look into it.
Comment #99
thenchev CreditAttribution: thenchev commentedThis info might help from the jenkins console output. I don't have my dev setup currently to check it out more.
Comment #100
pguillard CreditAttribution: pguillard commentedIn fact AssertTokenReplacementTrait does not exist in core, and has been removed from patch #89 to patch #94.
Should be much better.
Comment #101
pguillard CreditAttribution: pguillard commentedThe interdiff was missing
Comment #102
pbonnefoi CreditAttribution: pbonnefoi commentedThanks @pguillard it's working fine on Drupal 8.5.1
Comment #104
pbonnefoi CreditAttribution: pbonnefoi commented@pguillard I probably found a new buggy case. Let say I have a vocabulary like this :
Vegetables
They're all translated this way :
Légumes
With the patch provided by @pguillard, If I save the children then the problem is solved. But If I save the parent term, then the problem is still there (Drupal 8.6.10). I save term "Vegetables" and then I have url updated like this : /fr/ingredients/vegetables/pommes-de-terre.html
Any clue ?
Comment #105
vijaycs85Rerolling against 8.8.x
#104:
Are you sure you don't have
Flag other translations as outdated
is checked?Comment #107
vijaycs85Fixing deprecation errors.
Comment #110
ndobromirov CreditAttribution: ndobromirov at FFW commentedAdded some more tags on the issue to improve visibility, as this is still relevant in late 2019.
Tokens are used for all aliases in Drupal in the context of path-auto module, current usage reported in 150k+ at time of writing. When you have content translation and path-auto you will be getting wrong URL aliases due to this.
In the context of metatag module, you will be presenting wrongly translated information on the pages. This is another module with over 100k installations as of late 2019.
Comment #111
megadesk3000 CreditAttribution: megadesk3000 at Unic commentedHey together
The patch #107 isn't applying against 8.8.x
Do others have the same problem?
Comment #112
andypostComment #113
rpayanmComment #114
rpayanmIt was missing a file.
Comment #115
andypostComment #116
marassa CreditAttribution: marassa commentedWorks fine for me.
Comment #118
aleevasLooks like in #114 failed test not related to patch.
I've re-rolled patch against 9.1.x.
Let's see that testing bot say
Comment #119
ndobromirov CreditAttribution: ndobromirov at FFW commentedAny chance to see a reroll of this in 8.x?
Comment #120
andypostProbably as backport as of bug to 8.9.bugfix
Comment #121
sakhan CreditAttribution: sakhan as a volunteer commentedThis patch in comment 118 is working on Drupal 9 and is good to go!
Comment #122
alexpottThis needs a reroll.
Let's not make these changes here. They are not in scope.
Can this be moved to core/tests/Drupal/Tests/Traits/Core
I think this line can be removed. It's wrong because WebTestBase is a thing of the past and I don't think we need to say this is for BrowserTestBase tests.
This construction of passing an array of tests and an array of metadata tests is confusing.
I'm not sure about passing in the message here. I think we could build the message here using the $token and the key of $data.
$this->assertEquals() and the expected value should go first.
If this fails then it be helpful to have a message saying which token it was for. See above point about the $message param. I think we can build a message for each assertion in the test and it'll be better.
The comments about the test method above apply here too.
Separating the test and metadata test set-up makes this harder to think about than it needs to be.
Yes this is an existing problem but we're now making an API because we're adding the trait. I think we should consider ditching the trait here and file a follow-up to make a nicer trait. With a clearer API. It would be nice if each test passed to the trait looked something like
Comment #123
megadesk3000 CreditAttribution: megadesk3000 at Unic commentedHey together
The patch in #114 isn't applying to 8.9.1 anymore. Looks like it needs a reroll?
Comment #124
megadesk3000 CreditAttribution: megadesk3000 at Unic commentedRerolled the patch from #114 against 8.9.x.
Comment #125
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedWorking on this.
Comment #126
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have added re-roll of patch #118.
I have also fixed points 1, 2, 3 and 6 of comment #122.
It still needs work for the remaining points of comment #122.
Comment #127
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedWorking on this.
Comment #128
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed use statement issue of patch #126.
Comment #130
andypostComment #131
ankithashettyRerolled the patch in #128, thanks!
Comment #132
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedUpdated patch.
Comment #134
cgoffin CreditAttribution: cgoffin at Sopra Steria commentedI needed the patch against the 9.1.5 version. Here the rerolled patch.
Comment #136
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedRerolled the patch #132
Comment #137
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedfixed the failed test case.
Comment #139
Berdirthis refactoring seems unrelated and only saves a single line of code at the cost of making the patch bigger and the code harder to understand. I'd suggest to revert these changes.
I know this is copied from node tokens, but the problem is that the operations argument is weird and content_translation only implements it for the default entity_view operation. So I'd suggest to leave out the argument, we might deprecate that thing.
See #2978048: Unpublished translations should fallback to the original language
This too is unrelated refactoring and it's not the same. getDescription() returns the value, not the processed string.
I'm pretty sure this does nothing.
The API for config entities is very different and applies to when they are loaded. the vocabulary is already loaded here. This sadly needs to be done whenever token replace is called and the vocabulary is loaded. For example the term:vocabulary token.
I think @alexpott proposed to not introduce this trait here, makes the test harder to understand.
how is escaping or unsafe names related to translating them? Is it possible this is a rebase of a very old version of this patch that still had that. I think we can drop all these changes.
this needs to be reverted too.
this is testing the test, I'm not sure what this is for. remove?
Beside the refactoring and some more tokens, I don't see how the fixes are actually asserted here.
No translations are created, no language options are passed. to the test.
To be honest, I would consider to just start from scratch with the test. As a bugfix, this is actually a good candidate for writing failing tests first. add the language modules, add a second language, add translations to the term and then fetch the same tokens with different options to get the translated values.
Comment #140
apadernoComment #141
Jeroen Dost CreditAttribution: Jeroen Dost commentedDoes anyone have a patch that works against 8.9.18?
Comment #145
Eduardo Morales AlbertiI created a merge request to understand better the changes and to be more clear with the review, we need this for our project.
Comment #148
AardWolf CreditAttribution: AardWolf at ADCI Solutions commentedUpdate patch to 9.5 in work
Comment #149
AardWolf CreditAttribution: AardWolf at ADCI Solutions commentedComment #150
matdemeuePatch #148 did apply for me on 9.5.4.
Comment #151
quietone CreditAttribution: quietone at PreviousNext commentedThis is blocking #1885962: Comment tokens should use entity translation API
Comment #152
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedRe-rolled the patch. Please review.
Comment #154
hongqing CreditAttribution: hongqing commentedThanks for the contribution. It works with Drupal 10.1.4.
Comment #155
corneboele CreditAttribution: corneboele commentedRerolled for 10.2.x