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.

CommentFileSizeAuthor
#108 2451693-content-translations-links-108.patch2.28 KBConnBi
#107 2451693-content_translation-links_issue.patch3.27 KBbiancaradu27
#97 interdiff_95-97.txt881 bytesadityasingh
#97 2451693-97.patch12.09 KBadityasingh
#95 2451693-95.patch11.54 KBravi.shankar
#91 drupal9.3.2-2451693-91.patch11.63 KBDonAtt
#87 2451693-87.patch11.59 KBkevin.brocatus
#83 2451693-83.patch11.93 KBkevin.brocatus
#73 Screenshot 2021-07-05 at 11.38.53.png133.6 KBjames.williams
#70 Screenshot_2021-03-18_09-30-57.png87.52 KBclayfreeman
#70 Screenshot_2021-03-18_09-29-17.png55.66 KBclayfreeman
#70 Screenshot_2021-03-18_08-59-00.png75.82 KBclayfreeman
#64 2451693-64.patch11.92 KBNWOM
#60 2451693-59.patch11.92 KBNWOM
#58 2451693-58.patch12.41 KBclayfreeman
#58 2451693-58.interdiff.txt2.2 KBclayfreeman
#57 2451693-57.patch10.21 KBclayfreeman
#57 2451693-57.interdiff.txt10.34 KBclayfreeman
#55 2451693-55.interdiff.txt2.22 KBclayfreeman
#55 2451693-55.patch6.57 KBclayfreeman
#53 2451693-53-content_translation-links.patch4.35 KBclayfreeman
#52 2451693-52-content_translation-links.patch2.59 KBHLopes
#48 2451693-48-content_translation-links.patch2.37 KBLemonHardos
#46 2451693-46-content_translation-links.patch2.59 KBdouggreen
#25 plach___Drupal_8.png197.02 KBplach
#25 Detection_and_selection___Drupal_8.png253.87 KBplach
#22 content_translation_overview_fix_links-2451693-22-do-not-test.patch22.3 KBhchonov
#21 content_translation_overview_fix_links_extended.patch24.76 KBhchonov
#10 show_correct_edit_links_on_translation_page-2451693-10.patch6.61 KBhchonov
#6 show_correct_edit_links_on_translation_page-2451693-6.patch1.93 KBhchonov
#1 show_correct_edit_links_on_translation_page-2451693-1.patch949 byteshchonov

Issue fork drupal-2451693

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov’s picture

A 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.

Status: Needs review » Needs work

The last submitted patch, 1: show_correct_edit_links_on_translation_page-2451693-1.patch, failed testing.

YesCT’s picture

Issue summary: View changes
Issue tags: +Needs tests, +D8MI

might 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)

hchonov’s picture

The 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:

     // If the user is allowed to edit the entity we point the edit link to
     // the entity form, otherwise if we are not dealing with the original
     // language we point the link to the translation form.
     if ($entity->access('update')) {
       $links['edit']['url'] = $entity->urlInfo('edit-form');
       $links['edit']['language'] = $language;
     }
     elseif (!$is_original && $handler->getTranslationAccess($entity, 'update')->isAllowed()) {
       $links['edit']['url'] = $edit_url;
     }

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.

hchonov’s picture

There 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.

hchonov’s picture

This 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.

hchonov’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: show_correct_edit_links_on_translation_page-2451693-6.patch, failed testing.

plach’s picture

Links 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.

hchonov’s picture

Status: Needs work » Needs review
FileSize
6.61 KB

But 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.

Status: Needs review » Needs work

The last submitted patch, 10: show_correct_edit_links_on_translation_page-2451693-10.patch, failed testing.

tstoeckler’s picture

Issue summary: View changes
Issue tags: -Needs tests +sprint, +drupaldevdays

This 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.

+++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
@@ -164,7 +155,7 @@ public function overview(RouteMatchInterface $route_match, $entity_type_id = NUL
-          if ($entity->access('update')) {
+          if ($entity->access('update') && $is_original) {

Let's switch the conditions like in the patch above. $entity->access() is (potentially) a non-trivial operation.

plach’s picture

@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?

plach’s picture

A 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.

tstoeckler’s picture

Talked 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.

plach’s picture

Well, #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.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: show_correct_edit_links_on_translation_page-2451693-10.patch, failed testing.

hchonov’s picture

Something strange.... I ran the test on two machines here on the devdays and it is not failing, but on the server it fails....

tstoeckler’s picture

Status: Needs work » Postponed

Talked with @hchonov today. Let's postpone this for now.

hchonov’s picture

Status: Postponed » Needs review
FileSize
24.76 KB

I 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.

hchonov’s picture

penyaskito’s picture

penyaskito’s picture

plach’s picture

I'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.

hchonov’s picture

What 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.

plach’s picture

#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.

plach’s picture

hchonov’s picture

@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.

hchonov’s picture

The other patch is removing something different not the language from the Url options.
I am talking about this:

        $edit_url = new Url(
          'content_translation.translation_edit_' . $entity_type_id,
          array(
            'language' => $language->getId(),
            $entity_type_id => $entity->id(),
          ),
          array(
            'language' => $language,
          )
        );

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.

plach’s picture

Ok, I'm fine with that, but then the title and the IS definitely need an update.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

gapple’s picture

Issue summary: View changes
Status: Postponed » Active

Blocking issue was closed, so un-postponing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sukanya.ramakrishnan’s picture

Folks,

Wondering if this issue can be taken up again?

regards
Sukanya

sukanya.ramakrishnan’s picture

Title: Edit links on the translations page are not pointing to the translation form » Edit links on the translations page do not point to the edit form of translation!
sukanya.ramakrishnan’s picture

Title: Edit links on the translations page do not point to the edit form of translation! » Edit links on the translations overview page do not point to the edit form of translation!

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sukanya.ramakrishnan’s picture

Folks,

Can this issue be please taken up again. This is an important issue with respect to translation!

Thanks
Sukanya

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

douggreen’s picture

Version: 8.6.x-dev » 8.7.x-dev
FileSize
2.59 KB

Attached 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.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

LemonHardos’s picture

Bug 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.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

HLopes’s picture

Having the same issue on 8.9.0 beta2, #48 worked with complains about the patch ending on the middle of a file

HLopes’s picture

#48 reroll for 8.9.0-beta2

clayfreeman’s picture

I 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).

Status: Needs review » Needs work

The last submitted patch, 53: 2451693-53-content_translation-links.patch, failed testing. View results

clayfreeman’s picture

Status: Needs work » Needs review
FileSize
6.57 KB
2.22 KB

Attempting to resolve test failures by updating tests as needed.

Status: Needs review » Needs work

The last submitted patch, 55: 2451693-55.patch, failed testing. View results

clayfreeman’s picture

Assigned: Unassigned » clayfreeman
Status: Needs work » Active
FileSize
10.34 KB
10.21 KB

End of day work. Still some tests to fix. Will return to this tomorrow.

clayfreeman’s picture

Status: Active » Needs review
FileSize
2.2 KB
12.41 KB

I'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.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

NWOM’s picture

#58 no longer applied to 9.1.x. Here is a re-roll.

Status: Needs review » Needs work

The last submitted patch, 60: 2451693-59.patch, failed testing. View results

clayfreeman’s picture

Status: Needs work » Needs review

Getting 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.

NWOM’s picture

#60 no longer applied against 9.1.x or 9.2.x.

Checking patch core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php...
error: while searching for:
        $view_url = $entity->toUrl('canonical', ['language' => $language])->toString();
        $elements = $this->xpath('//table//a[@href=:href]', [':href' => $view_url]);
        $this->assertEqual($elements[0]->getText(), $entity->getTranslation($langcode)->label(), new FormattableMarkup('Label correctly shown for %language translation.', ['%language' => $langcode]));
        $edit_path = $entity->toUrl('edit-form', ['language' => $language])->toString();
        $elements = $this->xpath('//table//ul[@class="dropbutton"]/li/a[@href=:href]', [':href' => $edit_path]);
        $this->assertEqual($elements[0]->getText(), t('Edit'), new FormattableMarkup('Edit link correct for %language translation.', ['%language' => $langcode]));
      }

error: patch failed: core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php:227
error: core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php: patch does not apply

Here is another re-roll that applies successfully to both versions.

Status: Needs review » Needs work

The last submitted patch, 64: 2451693-64.patch, failed testing. View results

raman.b made their first commit to this issue’s fork.

raman.b’s picture

Status: Needs work » Needs review
clayfreeman’s picture

Status: Needs review » Reviewed & tested by the community

Marking RTBC; the push I did most recently was just to remove a junk file & catch up the branch to 9.2.x.

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing, +Needs issue summary update, +Needs subsystem maintainer review

I 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.

clayfreeman’s picture

Not sure if this is sufficient for the requested manual testing, but here's a content entity with an English and French translation:

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:

The edit operation link takes you to a language-prefixed edit page for the default translation - not the specific translation for which the operation link was generated

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:

The edit operation link taking you to the correct destination

clayfreeman’s picture

Assigned: clayfreeman » Unassigned

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

james.williams’s picture

Status: Needs review » Needs work
FileSize
133.6 KB

Now 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.)

clayfreeman’s picture

Status: Needs work » Needs review

@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.

Berdir’s picture

Hm, 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.

clayfreeman’s picture

@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.

james.williams’s picture

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?

Sure - 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.

clayfreeman’s picture

This 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.

james.williams’s picture

Sure, 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!

clayfreeman’s picture

Here are the things that I've tried thus far, each having major drawbacks:

  1. Use "Session" detection for both interface & content.

    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.

  2. Use "Session" detection for interface and "Content language" detection for content.

    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.

  3. Use "URL" (path prefix) detection for both interface & content.

    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.

clayfreeman’s picture

After 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.

james.williams’s picture

Cool, OK, we’ll I’m happy to defer to a wider consensus then. @Berdir, any thoughts on the direction this should take?

kevin.brocatus’s picture

FileSize
11.93 KB

#64 no longer applied to Drupal 9.2.x.

Status: Needs review » Needs work

The last submitted patch, 83: 2451693-83.patch, failed testing. View results

clayfreeman’s picture

Status: Needs work » Needs review
Idas’s picture

I have the same issue on Drupal 8.9.11

kevin.brocatus’s picture

#83 no longer applied to Drupal 9.2.x.

Status: Needs review » Needs work

The last submitted patch, 87: 2451693-87.patch, failed testing. View results

clayfreeman’s picture

Version: 9.3.x-dev » 9.4.x-dev
jefuri’s picture

Status: Needs work » Needs review
DonAtt’s picture

#87 doesn't apply to Drupal 9.3.2, here's a patch for that version.

Status: Needs review » Needs work

The last submitted patch, 91: drupal9.3.2-2451693-91.patch, failed testing. View results

clayfreeman’s picture

Status: Needs work » Needs review

Please 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.

joachim’s picture

Status: Needs review » Needs work

Needs 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.

ravi.shankar’s picture

Added reroll on Drupal 9.4.x.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

adityasingh’s picture

Status: Needs work » Needs review
FileSize
12.09 KB
881 bytes

Fixed custom commands failed.

clayfreeman’s picture

Version: 9.5.x-dev » 10.0.x-dev
Assigned: Unassigned » clayfreeman
Status: Needs review » Needs work
Issue tags: -Needs subsystem maintainer review

Rebased on 10.0.x. Will try decipher/add comments to the highlighted portion of code soon(-ish).

tallytarik’s picture

I think entityFormDeleteTranslation in ContentTranslationHandler.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.

heddn’s picture

I tend to agree with #99.

heddn’s picture

@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.

heddn’s picture

Status: Needs work » Reviewed & tested by the community

Actually, 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.

heddn changed the visibility of the branch 2451693-operations-links-on to hidden.

quietone’s picture

Version: 10.0.x-dev » 11.x-dev
Assigned: clayfreeman » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs steps to reproduce, +Needs title update, +Needs subsystem maintainer review

I'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 did not find any unanswered questions.
  • The first thing I notice is that there are no steps to reproduce this. I tried anyway using Umami and wasn't able to reproduce the problem. I am tagging this for steps to reproduce.
  • In #32 an Issue Summary update and title change was asked for. The issue summary was updated in #70 but I don't see a title change. I am tagging for a title update. Correct me if I am wrong and I missed that it was changed. Or is this title still accurate?
  • I found manual testing and screenshots in #70. Unfortunately, the steps taken to test this were not provided. But this is the type of information that should have links in the IS so that reviewers can find it.
  • In #98 the 'Needs subsystem maintain review' tag was removed but I do not see a comment from a subsystem maintainer. Where is that comment? I am adding that tag back until there is confirmation.
  • The original approach, in #22, was changed in #46. There was no discussion about that. Is the current solution preferred over #22?
  • @joachim made a comment on a previous MR asking for comments to explain the login. Those comments still need to be added.
  • The may be a self RTBC but I have not looked closely

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.

s_leu made their first commit to this issue’s fork.

biancaradu27’s picture

ConnBi’s picture

I Create a patch for drupal 10.2.3, And the #107 patch will change the node translation edit link, I revert the code.