Problem/Motivation
When attempting to edit or delete a translation using the generated operation links on the translation overview page, the user is always taken to a route pertaining to the default translation.
Proposed resolution
Make the operation links on the translation overview page link to routes specific to each translation.
(An inline code comment mentions that the behavior of linking to pages for the default translation was intended for the source language only because in that case there are not specific translation pages, re-enforcing the notion that this is in fact a bug.)
Remaining tasks
TBD after subsystem maintainer review.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#108 | 2451693-content-translations-links-108.patch | 2.28 KB | ConnBi |
#107 | 2451693-content_translation-links_issue.patch | 3.27 KB | biancaradu27 |
Issue fork drupal-2451693
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:
- 2451693-rebased changes, plain diff MR !5955
- 2451693-operations-links-on changes, plain diff MR !159
Comments
Comment #1
hchonovA wrong condition check was leading to always using the entity edit form instead of the translation form for translated entities.
A patch fixing the issue is attached.
Comment #3
YesCT CreditAttribution: YesCT commentedmight be worth having someone new do a git bisect on it to find out which commit broke it
git bisect is webchick's how to post http://webchick.net/node/99
--
added the issue summary template and the tasks template (see dreditor.org for browser plugin which provides buttons to make those templates easy to use)
Comment #4
hchonovThe content_translation routes were converted to a new style controller in revision 77c666c39ac2e5d1e81b42f50b0d00406288d85c. The logic was taken from the function content_translation_overview in content_translation.pages.inc, where it was also wrong, at least to my understanding.
The problem is in this code snippet:
Here it is first checked it the entity can be edited, and then the edit-form is used for all the translations without also checking if this is the original translation.
Comment #5
hchonovThere is a closed issue, where the links were fixed by adding a language parameter to the route : #2325977: All links lead to same entity translations in translation overviews.
But this is somehow wrong, we do not need to redirect the user to a different language page, but just get the translation for the requested language on the current page. So for this to work, we have to put the condition from the previous comment for the edit url and remove the language parameter from the routes for add, edit and delete.
Comment #6
hchonovThis patch shows the expected behaviour correctly. No optional route language parameter, so that the user stays on it's language page, but the translation form for the entity is shown in the requested language only.
Comment #7
hchonovComment #9
plachLinks are supposed to be generated based on user permissions: if the user has edit permission the link should point to the edit form, if she has translation permissions the link should point to the translation form. Not sure from just reading the IS whether this is not happening, but that's the intended behavior.
Comment #10
hchonovBut if we leave it like this and the user has the edit permission only then she is redirected to the entity form based on the requested translation language. So for example an english speaking editor wants to exchange a picture on a chinese content, she will not know what to do and where to click as the page context will change the language from english to chinese.
If an editor wants to see the page in chinese then she can change her language preference, but through the translations edit page it should be expected to open the entity only for translation edit and leave the context language untouched independent from the permissions.
If instead the edit links on the translation page point to the translation edit form withouth changing the user language context, then everything is ok.
I guess this patch should fix the addressed issues.
Comment #12
tstoecklerThis patch makes a lot of sense, thank you!
Removing the Needs tests tag as the existing test coverage seems to be sufficient.
Also expanded the issue summary a bit and added a beta evaluation.
Let's switch the conditions like in the patch above.
$entity->access()
is (potentially) a non-trivial operation.Comment #13
plach@hchonov:
I didn't look to the code, I was just pointing out that the intended behavior is that if you have access to the edit form, you always should see it, to avoid losing contextual editing. This was discussed at length when we originally introduced the distinction between edit form and translation form, and if we want to change that we need to discuss that with @webchick and/or the UX folks.
The issue you are pointing out looks more like #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language, does it?
Comment #14
plachA similar discussion about showing untranslatable fields or not is happening in #2290101: UI telling you a field is shared across languages is way too subtle.
Comment #15
tstoecklerTalked to @plach about this in IRC.
Apparently the fact that the entity edit page is shown was a much welcomed feature in D7 Entity Translationand was discussed (and decided on) at length during the D8 cycle.
Another issue that would solve (or allow to solve) the interface language switching problem would be #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language. Then we would still be showing the edit page, but the interface language would no longer change. So it might make sense to mark this postponed on that one or duplicate, not sure.
Comment #16
plachWell, #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language needs to be fixed as translation is currently broken when URL language detection is disabled. We could postpone this and re-evaluate it once that is done.
Comment #19
hchonovSomething strange.... I ran the test on two machines here on the devdays and it is not failing, but on the server it fails....
Comment #20
tstoecklerTalked with @hchonov today. Let's postpone this for now.
Comment #21
hchonovI posted a patch for the issue #1810394: Exploit hook_entity_prepare_form() to set the form language without having to rely on the current language.
Now I provide a patch, which includes also the patch from that issue, so that is get's tested. After that I will upload also a patch only for this issue.
Comment #22
hchonovAnd here the standalone patch, not including the one for #1810394: Exploit hook_entity_prepare_form() to set the form language without having to rely on the current language
Comment #23
penyaskitoPostponed on #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language.
Comment #24
penyaskitoReviewed #22. Makes sense to me, but we need #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language to land first (with proper tests).
Comment #25
plachI'm still not convinced about this: I don't get why a user cannot simply enable admin language negotiation settings and configure a preferred language.
Comment #26
hchonovWhat you suggest will not work if I configure my language detection based on the domain name, because then keeping the language in the url will redirect the user to the another site and breaking all the cookies and the session ....
What I achievied with my patch is to not switch the site language and stay on the translation form in the current site.
Comment #27
plach#1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language is already taking care of not switching content language, so what's left here is using translation forms for editors, which I don't support as it breaks contextual editing.
Comment #28
plachEveryone, please read #1807776: Support both simple and editorial workflows for translating entities for some background.
Comment #29
hchonov@plach leaving the language in the options for the edit urls will always switch the site to the target translation.... That is what is wrong here.
Comment #30
plachAAMOF, the latest patch in #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language is removing it.
Comment #31
hchonovThe other patch is removing something different not the language from the Url options.
I am talking about this:
Like this we always have the url pointing to the site in the target translation.... And that is actually what should be removed in the patch here.
Comment #32
plachOk, I'm fine with that, but then the title and the IS definitely need an update.
Comment #33
Gábor HojtsyAdded a note to the summary that this is postponed on #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language. Please correct if not true. It was not very evident.
Comment #34
Gábor HojtsyThere is no movement on #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language for over a month now, so removing from sprint.
Comment #35
gappleBlocking issue was closed, so un-postponing.
Comment #37
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedFolks,
Wondering if this issue can be taken up again?
regards
Sukanya
Comment #38
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedComment #39
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedComment #41
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedFolks,
Can this issue be please taken up again. This is an important issue with respect to translation!
Thanks
Sukanya
Comment #46
douggreen CreditAttribution: douggreen at Tag1 Consulting commentedAttached is a much simpler solution. I didn't update any tests, I'll leave that to someone else. Also bumping this to 8.7.x-dev.
As long as the intent is for the original language to be edited on the /edit page, and the translated pages to be edited with the translation page, then this is indeed a bug, and a bug fix attached.
Without this patch all links on the node/%/translation page lead to node/%/edit (not in the specified language as is suggested in issue description). With this patch, the links are right, and the access is right.
Comment #48
LemonHardos CreditAttribution: LemonHardos commentedBug still here in 8.7.2
Patch 47 needed rework for 8.7.2
Nothing added or removed, just line numbers in diff.
Thanks all for your work so far.
Comment #51
HLopes CreditAttribution: HLopes commentedHaving the same issue on 8.9.0 beta2, #48 worked with complains about the patch ending on the middle of a file
Comment #52
HLopes CreditAttribution: HLopes commented#48 reroll for 8.9.0-beta2
Comment #53
clayfreemanI found that this issue occurs when both non-translation and translation update or delete access is allowed. I've rewritten the patch to prefer translation routes (where available). Translation routes should only appear on non-default translations. I also removed the access check for the non-translation update or delete operations from the translation operation access check, since those are broken when both are always expected to be available (e.g. logged in as the admin user).
Comment #55
clayfreemanAttempting to resolve test failures by updating tests as needed.
Comment #57
clayfreemanEnd of day work. Still some tests to fix. Will return to this tomorrow.
Comment #58
clayfreemanI've modified these tests in a way that should get them working. I'll need someone else to review those to verify that what's being tested still makes sense given the changes to access result(s).
Please also verify that all symptoms are (still) resolved if you've posted anything in this thread since the scope has been adjusted several times and this issue seems to be a rather complex topic between what does / should happen.
Comment #60
NWOM CreditAttribution: NWOM commented#58 no longer applied to 9.1.x. Here is a re-roll.
Comment #63
clayfreemanGetting this opened as an MR to make patch management easier.
Setting to NR for automated test run. Will address any discrepancies from #58 once results become available.
Comment #64
NWOM CreditAttribution: NWOM commented#60 no longer applied against 9.1.x or 9.2.x.
Here is another re-roll that applies successfully to both versions.
Comment #67
raman.b CreditAttribution: raman.b at OpenSense Labs commentedComment #68
clayfreemanMarking RTBC; the push I did most recently was just to remove a junk file & catch up the branch to 9.2.x.
Comment #69
catchI don't see a full review of how the patch relates to the discussion around #28-32 (which was back in 2015). So putting it back to needs review.
I also briefly pinged @plach since he had concerns here originally , and he asked for it to get some more usability feedback. Tagging with 'needs manual testing' and 'needs issue summary' update for now.
Comment #70
clayfreemanNot sure if this is sufficient for the requested manual testing, but here's a content entity with an English and French translation:
Translations can be added without issue, but once a translation is added, clicking any of the operations links for the translation sends you to a route pertaining to the default translation (in this case, English). Here's what happens when clicking the "Edit" operation link with an unmodified Drupal core:
Note that the above path contains a path prefix of
/fr
, which is fine, but the route should be/fr/node/{node}/translations/edit/fr
. Applying MR !159 results in the operations links taking you to the correct destination. Here's what happens when clicking the "Edit" operation after patching Drupal core with MR !159:Comment #71
clayfreemanComment #73
james.williams CreditAttribution: james.williams at ComputerMinds for Hydrotechnik UK Ltd commentedNow that #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language has been completed, I don't see an actual bug remaining, once its language negotiator for the content language is used. It's pretty obscure to find, but that solved the problem for me without needing any of the patches/work here. The route for the original edit form is still linked to, but a query parameter is added to ensure the appropriate language is used for it.
I can understand that people may feel that a different route could be used for these links, but I don't believe there is an actual bug in the behaviour experienced by an end-user on a correctly-configured site any more. I don't think we actually have any tests-only patches to prove the bug's existence here?
Setting to needs work, on the basis that we at least need a tests-only patch to prove a bug. (But I suspect there is no bug, so this issue could be closed - or changed to be a documentation issue, as it's really not obvious that the 'content language' negotiation method needs to be configured - see screenshot.)
Comment #74
clayfreeman@james.williams I'm not sure exactly what you're asking for; MR !159 updates the tests in core that cover the broken functionality to assert the correct behavior, and I've provided manual testing in #70. The issue you linked was also released well before the site used in my manual testing was created, so to me it is of little relevance.
If you wish to observe a failing test case, I recommend applying this patch locally and removing the non-test changes.
Comment #75
BerdirHm, this seems to go in the opposite direction of what I'd like to do. The separation of edit and translate routes is IMHO a leftover of "the old days" when it actually was something different. It's not anymore.
See issues like #2155787: Introduce more flexible access control for content translation operations and #2985657: Content translation create access check with context for the target translation which are about making update access checking consistently apply to the current language of an entity. With that, you can use the entity access system to control translation access, which currently is really hard (e.g. only allowing a user to translate DE but not FR).
If the links are going to the default translation then you are likely using special language negotiation methods that aren't playing nice with URL language.
Comment #76
clayfreeman@Berdir I'd still argue that the links being generated are not semantically correct:
Consider a use case where you have a multilingual site administrator who prefers to have the interface in their primary language, but is also tasked with entering translations for content in various other languages. Should this user be expected to switch the interface language to enter translations in their non-primary language?
It would seem to be much more straightforward to me that the operations links for a translation always refer to that specific translation (i.e., Drupal core should make appropriate use of the language parameters for each translation-specific route).
I don't necessarily care too much about how the permissions are handled as long as the operations links for a translation always refer to the specific translation for which they were generated. I think the primary issue is being complicated by the fact that the access checks are currently incorrect which causes the operations links to fall back to the canonical routes. The links shouldn't be generated at all if the access checks fail in my opinion, but that still leaves us with the problem of the access checks being wrong.
Forgive me if I'm missing something obvious, as it's been quite a while since I've worked on this issue.
Comment #77
james.williams CreditAttribution: james.williams at ComputerMinds for Hydrotechnik UK Ltd commentedSure - that's what #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language made possible! See the screenshot I added in my comment #73, for the configuration that's necessary. It makes use of query string parameters to allow the content in the form to be in one language, even though the interface is in another, all whilst being on /node/xx/edit and using the existing route. So the requirement you've stated is already fulfilled, just by a different implementation - i.e. using a query string to trigger it, rather than a different route altogether.
Comment #78
clayfreemanThis solution isn't quite perfect either; after being redirected to the entity upon saving an updated translation, the language switcher becomes useless since it doesn't clear or update the
language_content_entity
query parameter.Comment #79
james.williams CreditAttribution: james.williams at ComputerMinds for Hydrotechnik UK Ltd commentedSure, and that's annoying. But I think that's a different bug with its own solution on the way - #2864055: LanguageNegotiationContentEntity: don't break interface language switcher links perhaps?
I suggest trying that too, and maybe we should then close this issue?
I've spent most of the last week delving down into rabbit warren of related issues - it's quite a web of awkward issues!
Comment #80
clayfreemanHere are the things that I've tried thus far, each having major drawbacks:
This has the drawback of forcing the user to switch the interface language each time they wish to edit content in a specific language and making the translation overview page (and its operations links) completely useless.
This has the drawback of breaking the language switcher block after using the translation overview page since the language switcher neither clears nor updates the content language negotiation mechanism.
This has the drawback of forcing the user to switch the interface language each time they wish to edit content in a specific language and breaking content URL aliases for non-default translations (at least when using Pathauto).
I can't help but continue to point out that using a translation-specific route with a required language parameter would completely solve these issues and provide greater flexibility in how language detection could be configured.
I haven't yet tested the issue you referenced in #79 - that's next on my list.
Comment #81
clayfreemanAfter applying your patch in the referenced issue, the language switcher still produces links with bifurcated languages in the query string:
language=en&language_content_entity=fr
Ideally, the language switcher block would unset
language_content_entity
, allowing content negotiation to fall back to the result of interface negotiation.Regardless of the language switcher issues, I believe it's a bit too restrictive to purposefully avoid the translation-specific routes. I'm just not understanding why it's more logical to retcon the default translation's routes for a different purpose than to use translation-specific routes that already exist.
At the end of the day, it shouldn't be possible for a user to break the translation overview page's operations links in configuration; that's just absurd.
Comment #82
james.williams CreditAttribution: james.williams at ComputerMinds commentedCool, OK, we’ll I’m happy to defer to a wider consensus then. @Berdir, any thoughts on the direction this should take?
Comment #83
kevin.brocatus CreditAttribution: kevin.brocatus at INDICIA commented#64 no longer applied to Drupal 9.2.x.
Comment #85
clayfreemanComment #86
Idas CreditAttribution: Idas commentedI have the same issue on Drupal 8.9.11
Comment #87
kevin.brocatus CreditAttribution: kevin.brocatus at INDICIA commented#83 no longer applied to Drupal 9.2.x.
Comment #89
clayfreemanComment #90
jefuri CreditAttribution: jefuri as a volunteer and at Synetic commentedComment #91
DonAtt CreditAttribution: DonAtt as a volunteer commented#87 doesn't apply to Drupal 9.3.2, here's a patch for that version.
Comment #93
clayfreemanPlease make sure any version-specific patches are named as follows:
<PATCH-NAME-HERE>-do-not-test.patch
If you don't do this, the patch will fail to apply to the version of Drupal targeted by this issue, and the issue will be reverted to "Needs Work" automatically.
Comment #94
joachim CreditAttribution: joachim at Factorial GmbH commentedNeeds a reroll by the looks of it, as the MR doesn't apply.
The logic is increasingly complex and could really do with comments to explain it.
Comment #95
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll on Drupal 9.4.x.
Comment #97
adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed custom commands failed.
Comment #98
clayfreemanRebased on 10.0.x. Will try decipher/add comments to the highlighted portion of code soon(-ish).
Comment #99
tallytarik CreditAttribution: tallytarik commentedI think
entityFormDeleteTranslation
inContentTranslationHandler.php
also needs to be updated.This is triggered by the 'Delete translation' button on the translation edit form - this is different to the 'Delete' button in the controls on the translation list page (which has been fixed with these patches).
As-is, it will redirect you to the delete confirmation page for the entity rather than the translation.
Comment #100
heddnI tend to agree with #99.
Comment #101
heddn@clayfreeman can you rebase the MR on the 11.x branch please? Or we'll have to (re)create a new branch and clone over the changes from !159.
Comment #103
heddnActually, I'll just open a new branch/MR, it is easy enough.
Looks like the feedback in #99 was handled over in #3108102: Destination url query param affects on form translation delete submission. I just rebased this, nothing else. I think that means I can still RTBC this.
Comment #105
quietone CreditAttribution: quietone at PreviousNext commentedI'm doing some RTBC queue triage. I read the IS and the comments. I seem to be thinking in lists today so here is what I found. I did not do a code review.
I am setting to needs work for the above and un-assigning so it is clear that anyone can contribute here.
Also, I started the test-only changes test run and the good news is that fails. And updated credit.
Comment #107
biancaradu27 CreditAttribution: biancaradu27 at Dream Production commentedComment #108
ConnBi CreditAttribution: ConnBi commentedI Create a patch for drupal 10.2.3, And the #107 patch will change the node translation edit link, I revert the code.