Problem/Motivation

Currently when being on an content entity route, entity language is set by inspecting the available entity translations and picking the one matching the current content language or falling back to another exisiting one. This approach is not flexible enough as it prevents us from explicitly specifying the target translation language. One relevant consequence is that, if content language negotiation settings are configured in a way that does not allow to switch content language, there is no way to edit a translation not being in the current content language (see #1833196: could not have interface in language A and create a translation from language B to language C).

So there are two major problems:

A: Site configration with domain based language negotiation

Steps to reproduce:

  1. An authenticated user goes to the translate overview "https://en.example.com/node/1/translations".
  2. The user decides to view or edit the french translation of the node entity.
  3. The links for the french translation will look e.g. like this "https://fr.example.com/node/1" or "https://fr.example.com/node/1/edit".
  4. This is a different domain, where the user is not logged in.

B. Site configuration with url based language negotiation

Steps to reproduce:

  1. An only english speaking authenticated user goes to the translate overview "https://example.com/en/node/1/translations".
  2. The user wants to edit the french translation of the node entity and just exchange e.g. a picture in the content.
  3. The links for the french translation will look e.g. like this "https://example.com/fr/node/1" or "https://example.com/fr/node/1/edit".
  4. The user clicks on the edit link and now the interface language changes to french and the user is not able to understand the titles of the action buttons.

Proposed resolution

  • Define a language negotiator for the language type "language content".
  • It will determine the content language from a query parameter e.g. node/1?language_content_entity=fr
  • If the new language negotiator is configured with a higher priority than the url language negotiator, then it will look up in the options for the outbound urls if the language option is specified. If so, the query parameter with the language specified by the language option will be appended to the outbound urls. Then the language option will be unset, so that the url language negotiator does not rewrite the url.
  • If the query parameter is on a url for a content entity route, then all the outbound urls on this route, which are pointing to a route defined in the entity link templates of the entity on which route we currently are, will inherit it, as long as the urls do not have the language option set. In this case they will also have the query parameter but with the language specified in the url options. Otherwise the urls will remain untouched.

This proposed resolution solves both major problems listed under the section "Problem/Motivation".

Remaining tasks

  • Review the current approach and evaluate if documentation or a change notice is needed.

User interface changes

If the new language negotiator is configured with a higher priority than the url negotiator, urls which specifiy the language option will have the query parameter language_content_entity set and pointing to the desired target translation language.

API changes

None

Background

The access-control issue originally reported here has been solved in #1807776: Support both simple and editorial workflows for translating entities.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug report, because as at the moment if a site is configured with a different domain for each language, then a link to a specific content entity translation will redirect an authenticated user to a different domain. This blocks also a bug fix: #1833196: could not have interface in language A and create a translation from language B to language C.
Issue priority Major because this unties the translation UI navigation from the current content language, which may be big limitation for sites using domain-based URL language negotiation.
Prioritized changes This is issue is a follow-up of the issue originally adding the Content Translation module to core. Additionally it unblocks a bug fixing issue and opens the door for further UX improvements.
Disruption None foreseen: old URLs will keep working exactly as before.

Why this should be an RC target

Domain-based language negotiation has been a problem for a long time for Drupal. The multilingual initiative during the Drupal 8 dev cycle helped in making a lot of improvements, but this one problem proved to be a tough one and the issue is already three years old. Finally at the Drupal Con 2015 in Barcelona we agreed after a discussion with @alexpott, @plach, @hchonov and @mkalkbrenner that this is the best solution we can provide. If it made its way into 8.0.0 it would be a great improvement, if not Drupal 8.0.0 would be shipped with an unnecessary and avoidable UX problem.

The change does not introduce any disruptions as it only adds a new feature to Drupal 8 which is disabled by default. Even turning the new feature on does not introduce any disruptions.

Additionally, we are changing the Content Translation route names, which even if it's probably not an API change, it's better to do before 8.0.0.

CommentFileSizeAuthor
#232 Selection_009.png121.61 KBraduungurean
#216 209-216-interdiff.txt1.33 KBhchonov
#216 1810394-216.patch56.61 KBhchonov
#209 195-209-interdiff.txt6.8 KBhchonov
#209 1810394-209.patch55.96 KBhchonov
#201 199-201-interdiff.txt726 byteshchonov
#201 1810394-201.patch56.81 KBhchonov
#199 195-199-interdiff.txt6.35 KBhchonov
#199 1810394-199.patch56.64 KBhchonov
#195 194-195-interdiff.txt527 byteshchonov
#195 1810394-195.patch56.73 KBhchonov
#194 188-194-interdiff.txt979 byteshchonov
#194 1810394-194.patch56.06 KBhchonov
#188 180-188-interdiff.txt8.33 KBhchonov
#188 1810394-188.patch54.85 KBhchonov
#180 interdiff.1810394.174.180.txt1.88 KBYesCT
#180 1810394.180.patch53.01 KBYesCT
#177 interdiff.1810394.174.177.txt4.22 KBYesCT
#177 1810394.177.patch52.95 KBYesCT
#174 170-174-interdiff.txt3.98 KBhchonov
#174 1810394-174.patch52.5 KBhchonov
#170 161-170-interdiff.txt14.29 KBhchonov
#170 1810394-170.patch51.19 KBhchonov
#161 interdiff.1810394.157.161.txt7.36 KBYesCT
#161 1810394.161.patch44.76 KBYesCT
#157 148-157-interdiff.txt5.46 KBhchonov
#157 1810394-157.patch44.85 KBhchonov
#148 147-148-interdiff.txt3.71 KBhchonov
#148 1810394-148.patch45.48 KBhchonov
#147 145-147-interdiff.txt5.7 KBhchonov
#147 1810394-147.patch45.45 KBhchonov
#145 139-145-interdiff.txt3.98 KBhchonov
#145 1810394-145.patch45.02 KBhchonov
#139 136-139-interdiff.txt9.41 KBhchonov
#139 1810394-139.patch44.34 KBhchonov
#136 131-136-interdiff.txt10.28 KBhchonov
#136 1810394-136.patch45.58 KBhchonov
#131 129-131-interdiff.txt1.15 KBhchonov
#131 1810394-131.patch44.49 KBhchonov
#129 125-129-interdiff.txt34.79 KBhchonov
#129 1810394-129.patch44.49 KBhchonov
#125 122-125-interdiff.txt920 byteshchonov
#125 1810394-125.patch23.74 KBhchonov
#123 120-122-interdiff.txt1.09 KBhchonov
#123 1810394-122.patch23.42 KBhchonov
#120 117-120-interdiff.txt26.84 KBhchonov
#120 1810394-120.patch22.18 KBhchonov
#117 1810394-117.patch14.67 KBhchonov
#107 ct-prepare-1810394-107.patch15.62 KBplach
#107 ct-prepare-1810394-107.interdiff.txt2.99 KBplach
#105 ct-prepare-1810394-105.patch15.54 KBplach
#105 ct-prepare-1810394-105.interdiff.txt1.42 KBplach
#98 ct-prepare-1810394-98.patch15.53 KBplach
#98 ct-prepare-1810394-98.interdiff.txt1.18 KBplach
#95 ct-prepare-1810394-95.patch16.29 KBplach
#95 ct-prepare-1810394-95.interdiff.txt3.4 KBplach
#94 ct-prepare-1810394-94.patch14.77 KBplach
#94 ct-prepare-1810394-94.interdiff.txt3.68 KBplach
#92 ct-prepare-1810394-92.patch14.62 KBplach
#92 ct-prepare-1810394-92.interdiff.txt9.36 KBplach
#88 ct-prepare-1810394-88.patch8.85 KBplach
#88 ct-prepare-1810394-88.interdiff.txt7.66 KBplach
#84 entity_prepare_edit_translation-1810394-82.patch4.02 KBtstoeckler
#82 79-82-interdiff.txt1.46 KBhchonov
#82 entity_prepare_edit_translation-1810394-82.patch3.96 KBhchonov
#79 entity_prepare_edit_translation-1810394-79.patch3.99 KBhchonov
#75 entity_prepare_edit_translation-1810394-75.patch4.01 KBhchonov
#72 entity_prepare_edit_translation-1810394-71.patch2.46 KBhchonov
#64 1810394-entity-prepare-64.patch14.64 KBvijaycs85
#62 conflicts.txt1.86 KBYesCT
#62 1810394-entity-prepare-62.patch14.85 KBYesCT
#59 1810394-entity-prepare-59.patch14.88 KBSchnitzel
#59 interdiff-54-59.txt2.25 KBSchnitzel
#54 1810394-entity-prepare-54.patch13.67 KBSchnitzel
#54 interdiff-53-54.txt2.21 KBSchnitzel
#53 1810394-entity-prepare-53.patch13.59 KBSchnitzel
#48 1810394-diff-45-48.txt1.98 KBvijaycs85
#48 1810394-entity-prepare-48.patch12.46 KBvijaycs85
#45 1810394-entity-prepare-45.patch12.48 KBvijaycs85
#43 ct-prepare-1810394-43.patch12.53 KBplach
#38 ct-prepare-1810394-38.patch13 KBplach
#30 ct-prepare-1810394-29.patch13 KBplach
#29 ct-prepare-1810394-29.interdiff.do_not_test.patch1.5 KBplach
#29 et-language_fallback-2019055-22.patch57.33 KBplach
#24 ct-prepare-1810394-24.patch12.77 KBplach
#21 interdiff.20-21.txt945 bytespenyaskito
#21 ct-prepare-1810394-21.patch13.49 KBpenyaskito
#20 ct-prepare-1810394-20.patch.interdiff.do_not_test.patch3.07 KBplach
#20 ct-prepare-1810394-20.patch.patch13.49 KBplach
#18 ct-prepare-1810394-18.patch11.35 KBplach
#14 exploit-hook_entity_presave-2025991-34+1810394-12.patch41.73 KBDavid Hernández
#10 exploit-hook_entity_presave-1810394-10.patch919 bytesDavid Hernández
#10 exploit-hook_entity_presave-2025991-34+1810394-10.patch41.73 KBDavid Hernández
#8 exploit-hook_entity_presave-1810394.patch899 bytesDavid Hernández
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Title: exploit hook_entity_prepare so not skipping access checks when adding translation [Follow-up to Entity Translation UI in core] » Exploit hook_entity_prepare so not skipping access checks when adding translation
Status: Postponed » Active

Obviously we need to introduce a hook_entity_prepare() call in EntityFormController::preprareEntity().

plach’s picture

Component: language system » translation_entity.module
plach’s picture

Assigned: Unassigned » plach

I'll work on this.

plach’s picture

Title: Exploit hook_entity_prepare so not skipping access checks when adding translation » Exploit hook_entity_prepare to set the form language without having to rely on the current language

Actually this is more like it.

plach’s picture

Status: Active » Postponed

Actually postponed on #2025991: Introduce hook_entity_prepare_form() to generalize hook_node_prepare(), which should be an easy one, reviews welcome.

Gábor Hojtsy’s picture

Component: translation_entity.module » content_translation.module
plach’s picture

Issue tags: +sprint
David Hernández’s picture

Assigned: plach » David Hernández
Status: Postponed » Needs work
FileSize
899 bytes

Instead of hook_entity_presave we are using hook_entity_presave_form, from #2025991: Introduce hook_entity_prepare_form() to generalize hook_node_prepare().

Now working on a test to cover the usage of this feature.

plach’s picture

Looks good, thanks! Just some minor remarks:

+++ b/core/modules/content_translation/content_translation.module
@@ -1040,3 +1040,17 @@ function content_translation_save_settings($settings) {
+function content_translation_entity_presave_form(\Drupal\Core\Entity\EntityInterface $entity, $form_display, $operation, array &$form_state) {

We don't need the full namespace for the entity interface, it's already imported through the use statment.

+++ b/core/modules/content_translation/content_translation.module
@@ -1040,3 +1040,17 @@ function content_translation_save_settings($settings) {
+  $target = Drupal::Request()->get('target');
+  $source = Drupal::Request()->get('source');

I was thinking that it might be better to namespace the parameter names to avoid conflicts: content_translation_source /

content_translation_target<code>. What do you think?

<code>
+++ b/core/modules/content_translation/content_translation.module
@@ -1040,3 +1040,17 @@ function content_translation_save_settings($settings) {
+  $source = Drupal::Request()->get('source');

Request() should be lowercase :)

David Hernández’s picture

Here's the patch with the issues from #9 fixed and a patch including the changes from #2025991: Introduce hook_entity_prepare_form() to generalize hook_node_prepare() so it's not required to apply both.

I still have to add the test to cover the usage of this change.

plach’s picture

Looks great, thanks. Unfortunately the hook issue has been pushed back atm.

David Hernández’s picture

Assigned: David Hernández » Unassigned
Status: Needs work » Postponed

Postponed until #2025991: Introduce hook_entity_prepare_form() to generalize hook_node_prepare() is resolved.

Doing some testing I found that the new hook hook_entity_presave_form is not being called and I'm not sure if it's because I have to manually call it on the page callback or somewhere else or the hook is not being called at all.

plach’s picture

Maybe, because it is hook_entity_prepare_form? ;)
(unless you just mistyped it :)

David Hernández’s picture

You are right, I mistyped it. I attached a new patch with this fixed.

Anyways, now I know the hook is being called (using dpm to print the values being passed), but is not working. The translation is being created but in the original language, not the one passed as parameter on the path. Also, when creating the translation, I'm changing the content of the fields and the change is not being saved either, is using the content of the original language.

plach’s picture

Mmh, ok, thanks for the test. Let's wait for the parent issue then. If you wish to move to something else just ping me in IRC.

plach’s picture

Status: Postponed » Needs work

The parent issue has been committed. Let's go on with this.

plach’s picture

Assigned: Unassigned » plach

Working a bit on this.

plach’s picture

Assigned: plach » Unassigned
Status: Needs work » Needs review
FileSize
11.35 KB

Got a bit distracted but I have finally something to post.

Status: Needs review » Needs work

The last submitted patch, ct-prepare-1810394-18.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
13.49 KB
3.07 KB

This should fix the failures.

penyaskito’s picture

Reviewed code and only found two minor typos I fixed myself.

+++ b/core/modules/content_translation/content_translation.moduleundefined
@@ -272,6 +279,89 @@ function _content_translation_menu_strip_loaders($path) {
+    // If we have a source lanaguage defined we are creating a new translation
+    // for which we need to prepare the inital values.

lanaguage => language
inital => initial

+++ b/core/modules/content_translation/content_translation.moduleundefined
@@ -272,6 +279,89 @@ function _content_translation_menu_strip_loaders($path) {
+      // @todo The "key" part should not be needed. Remove it as soon as things
+      // do not break.

Is this still needed?

Otherwise looks OK and well tested, so I would say RTBC.

plach’s picture

I made a quick test and that code is still breaking without the key part, however #1810370: Entity Translation API improvements is going to remove that @todo in another way, so let's just not bother with it ;)

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

OK, RTBC then if green (that should be because only comments were changed). Thanks @plach!

plach’s picture

FileSize
12.77 KB

Rerolled after #1810370: Entity Translation API improvements. As promised the @todo mentioned in #21 is gone :)

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/content_translation/content_translation.moduleundefined
@@ -57,6 +57,13 @@ function content_translation_module_implements_alter(&$implementations, $hook) {
+    // And some to the top of the list.
+    case 'entity_prepare_form':
+      $group = $implementations['content_translation'];
+      unset($implementations['content_translation']);
+      $implementations = array('content_translation' => $group) + $implementations;
+      break;

This comment does not explain what is going on here.

Also I'm not sure that this implementation is the best way to go... thinking about it

plach’s picture

Also I'm not sure that this implementation is the best way to go...

Not sure whether you are referring to the whole request param + hook_entity_prepare() implementation approach or just the fact that we are altering the hook implementation order. If the latter, this solution works even without it obviously, we can remove this hunk if that helps making things cleaner.

thinking about it

I'm looking forward to reading your feedback :)

plach’s picture

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
57.33 KB
1.5 KB

Improved inline comments. Setting back to RTBC while waiting for more indications.

plach’s picture

Wrong patch, sorry.

Gábor Hojtsy’s picture

Issue tags: +API clean-up

At least tag it to identify its an API change.

Gábor Hojtsy’s picture

Issue tags: +API change

Actually make sure proposed API changes have the API change tag.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary copied over orig issue summary text.

plach’s picture

Assigned: Unassigned » alexpott

I realized the issue summary was awfully out of date, updated it. Assigning to @alexpott as it seems he may want to suggest an alternative approach. Hope it's ok to have this back to RTBC meanwhile.

plach’s picture

Issue summary: View changes

Updated issue summary

plach’s picture

Issue summary: View changes

Updated issue summary

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue tags: -API change +API addition

This is more like an API addition as old URLs are still working the same way.

YesCT’s picture

content_translation_prepare_translation() was moved from
content_translation.pages.inc
to
content_translation.module

I wanted to see if anything also changed so I did a diff. Posting it here in case it helps someone else who looks at this.

5,9c5,9
<  *   The entitiy being translated.
<  * @param \Drupal\Core\Language\Language $source
<  *   The language to be used as source.
<  * @param \Drupal\Core\Language\Language $target
<  *   The language to be used as target.
---
>  *   The entity being translated.
>  * @param string $source
>  *   The language code to be used as source.
>  * @param string $target
>  *   The language code to be used as target.
11c11
< function content_translation_prepare_translation(EntityInterface $entity, Language $source, Language $target) {
---
> function content_translation_prepare_translation(EntityInterface $entity, $source, $target) {
15,16c15,16
<     $source_translation = $entity->getTranslation($source->id);
<     $entity->addTranslation($target->id, $source_translation->getPropertyValues());
---
>     $source_translation = $entity->getTranslation($source);
>     $entity->addTranslation($target, $source_translation->getPropertyValues());
24c24
<         $value[$target->id] = isset($value[$source->id]) ? $value[$source->id] : array();
---
>         $value[$target] = isset($value[$source]) ? $value[$source] : array();
alexpott’s picture

Status: Reviewed & tested by the community » Needs review

There are a couple of things that concern me about this patch.

Firstly, we have totally different forms appearing on the same route eg. http://example.org/node/1/edit vs http://example.org/node/1/edit?content_translation_target=it. Catch and I discussed this and we wondering whether or not this could just be a new route? Then it being a different form feels fixable differently - and less hackily.

+++ b/core/modules/content_translation/content_translation.moduleundefined
@@ -57,6 +57,16 @@ function content_translation_module_implements_alter(&$implementations, $hook) {
+    // Since in this hook implementation we are changing the form language, by
+    // acting first we minimize the risk of having inconsistent behaviors due to
+    // different hook_entity_prepare_form() implementations assuming different
+    // form languages.
+    case 'entity_prepare_form':
+      $group = $implementations['content_translation'];
+      unset($implementations['content_translation']);
+      $implementations = array('content_translation' => $group) + $implementations;
+      break;

This is just smelly... we have the module weight system for this and here we say well whatever content_translation comes first.

plach’s picture

I had a long discussion with @alexpott about this in IRC.

tldr: I am going to see whether using a new route to explicitly specify the form language is viable and keep the query string parameter only if it's not.

[11:17]	plach	alexpott: well, I not sure I understand why you are saying that we have a completely different form
[11:17]	plach	actually the query string parameter changes only the *values* displayed in the form, which remains the same
[11:18]	alexpott	plach: correct me if I'm wrong but won't non translatable fields be hidden?
[11:18]	plach	we have a double approach introduced in https://drupal.org/node/1807776
[11:18]	Druplicon	https://drupal.org/node/1807776 => Support both simple and editorial workflows for translating entities #1807776: Support both simple and editorial workflows for translating entities => Drupal core, translation_entity.module, major, closed (fixed), 177 comments, 3 IRC mentions
[11:19]	plach	on the node/1/edit route we have the regular entity form with untranslatable fields shown for every language
[11:19]	plach	on the node/1/translate/edit/..
[11:19]	plach	route we have te translation form where untransltable fields are hidden
[11:20]	plach	alexpott: this is to allow different workflows depending on the iser permissions
[11:20]	plach	* user
[11:20]	alexpott	plach: so what's with the comment "// Translators do not see the full entity form, just the translatable bits."
[11:21]	-->|	xaa (~xaa@91.178.0.86) has joined #drupal-i18n
[11:21]	plach	alexpott: it was already there (the function is being moved around), if I'm not mistaken it's used in both cases
[11:23]	plach	alexpott: no, sorry, the comment has been added in this patch, the code was already there
[11:23]	|<--	aax has left freenode (Ping timeout: 276 seconds)
[11:24]	alexpott	plach: yep and when content_translation_prepare_translation_form is working on the node/1/edit route you're telling me we're not hiding any fields?
[11:25]	plach	alexpott: yuip, because you wouldn't get there if you hadn't the 'update' permission
[11:25]	plach	at least that's the assumption
[11:25]	plach	maybe an inline comment?
[11:27]	plach	alexpott: if you think it's safer we can move that line in the translation edit page callback, but I think it's ok to have it there: those fields are not meant to be changed by people not having the update permission
[11:27]	plach	and in thata case they would be hidden regardless of langauge so your inital concern wouldn't apply
[11:28]	alexpott	plach: my biggest concerns about this patch are the two comments I made...
[11:29]	plach	alexpott: well I think the first one was cased by a misunderstanding, am I wrong?
[11:29]	plach	I eman people always see the same form, it just depends on their permissions
[11:29]	plach	* mean
[11:29]	alexpott	plach: I just don't like using a query string to set the translation language... it seems brittle
[11:29]	plach	alexpott: well, that's another concern
[11:30]	alexpott	plach: and as you point out we're now maintaining two routes to translate an entity
[11:30]	alexpott	plach: and I don't really understand why
[11:31]	plach	alexpott: one it's editing the other is translating
[11:31]	plach	but this has beem introduced in https://drupal.org/node/1807776
[11:31]	Druplicon	https://drupal.org/node/1807776 => Support both simple and editorial workflows for translating entities #1807776: Support both simple and editorial workflows for translating entities => Drupal core, translation_entity.module, major, closed (fixed), 177 comments, 4 IRC mentions
[11:32]	plach	alexpott: the issue summary over there has plenty of details about why we went this way (lots of discussions)
[11:32]	plach	a raw summary is
[11:33]	|<--	n3or has left freenode (Quit: goodbye)
[11:33]	plach	in simple workflows people expect to see all fields no matter which language they are changing
[11:33]	plach	in editorial workflows people editing the oriignal values (inclding untranslatable fields) are usually not the same of those editing the translated values, which shouldn't change untransltable fields
[11:33]	alexpott	plach: sure so I guess the real question is why are not providing a route to edit the entity in whatever language you want.
[11:34]	plach	alexpott: becuase it would mean introducing another tab I guess, which would bad for dx
[11:35]	alexpott	plach: I'm going to actually test the patch...
[11:36]	plach	alexpott: sure, just keep in mind that the patch only provides the ability to use the query string parameter to set the form language
[11:36]	-->|	rvilar (~ramon@84.88.33.150) has joined #drupal-i18n
[11:36]	plach	the behavior I was referring to is already in core
[11:37]	alexpott	plach: well the ability to set the language and prepare the entity values is not
[11:38]	plach	alexpott: I mean, if you don't play with the language negotiation settings there is not apparent change in Drupal's behavior
[11:42]	alexpott	plach: but I should be able to enable the Italian language and then allow content to be translated
[11:42]	alexpott	plach: and then go to node/1/edit?content_translation_target=it and the language should be set to italian right?
[11:42]	plach	alexpott: yes, if you have italian content for that node 1
[11:43]	alexpott	plach: which I don't... so this is what I don't really get
[11:46]	plach	alexpott: I think you just spot a bug in the patch
[11:46]	plach	do you mean you are able to switch form language to a non-existing translation?
[11:47]	alexpott	plach: yep there's very little error handling around what should happen if ?content_translation_target=it has no effect
[11:47]	alexpott	plach: which is the case if you create an english node and then edit it with that query string...
[11:47]	alexpott	plach: once you've created a translation it works fine
[11:48]	alexpott	plach: but I'm not sure this makes much sense
[11:48]	plach	alexpott: the intended behavior is that the query string prameter is just ignored when it refers to a non-existing translation
[11:49]	plach	do you mean there should be a warning or something like that?
[11:49]	alexpott	plach: this is why I don't like query strings for this functionality... as I think this behaviour is completely unexpected
[11:50]	plach	alexpott: but this is not a behavior the average is going to experience
[11:51]	plach	I mean you can trigger lots of weird stuff if you stard messing with URLs (not only query string bits)
[11:51]	plach	but that's your fault IMHO
[11:51]	alexpott	plach: yes but they wouldn't need to mess with url's
[11:52]	plach	alexpott: sorry, then I'm missing your point
[11:52]	plach	which is the behavior you find unexpected?
[11:52]	alexpott	plach: well atm the query string is not added in core any where so this is supposition...
[11:53]	plach	alexpott: the query string is only used in the translate overview page
[11:53]	plach	when the query string is missing you edit the entity in the current langauge
[11:54]	plach	which most of the time means you edit what you are seeing (the usual contextual editing)
[11:54]	alexpott	plach: yep I'm wrong...
[11:55]	alexpott	plach: I guess I just don't get why we are not using a separate route for this...
[11:56]	plach	alexpott: how would you expect that to work?
[11:56]	alexpott	plach: like we do when we add a translation
[11:57]	alexpott	plach: node/1/translations/edit/it ... but I guess that you're using this for the translators
[11:58]	plach	do you mean /node/1/edit gives you access to the entity in the current language and, say, node/1/edit/it lets you explictly specify the form language?
[11:58]	plach	alexpott: ^
[11:58]	alexpott	plach: that'd also work for me
[11:58]	plach	alexpott: ET in D7 does something very similar
[11:59]	alexpott	but also another weird thing is that node/1/edit/it == it/node/1/edit if you have url based language detection (right?)
[12:00]	plach	the main problem with this approach is you have to replicate access control in the new route with may be tricky to accomplish in a entity type agnostic fashion
[12:00]	plach	alexpott: yes
[12:00]	plach	alexpott: not sure if the new routing system can help with this
[12:00]	alexpott	plach: catch and I was wondering if the new router made this simpler
[12:01]	plach	well, if this is the case I'd be happy to get rid of the query string parameter
[12:01]	plach	
[12:01]	plach	but I know nearly nothing about the new routing system
[12:02]	alexpott	plach: ping dawehner or timplunkett when they are online...
[12:02]	plach	alexpott: will do
[12:03]	plach	alexpott: are you ok to go with the qeury string parameter if it turns out that going with a different route is tricky even with the new routing system?
[12:03]	alexpott	plach: I think you might be able to just use _entity_access: 'node.update'
[12:03]	plach	alexpott: for any entity type?
[12:03]	alexpott	plach: nope...
[12:04]	plach	alexpott: well, my fear is that there is now way to generalize that approach, otherwise II'd be happy to ditch that query string parameter
[12:04]	plach	* no way
plach’s picture

Keeping up with HEAD while I wait to talk with timplunkett and dawhener.

plach’s picture

Found #2061811: Only dynamic routes are available to route subscribers while working on this after talking with @dawehner.

plach’s picture

After starting exploring the alternative approach discussed with @alexpott, I realized copying route definitions is not enough: to be able to set the form language we also need to replace the original controller with one provided by CT and the proxy the call to the former. This is more or less what we are doing in D7, which is one of the most hacky and fragile parts of the whole ET codebase, since it breaks as soon as another module needs to replace the page callback.

I am starting to fear that going this way would be a bigger hack than just bite the bullet and go with query string parameters, but if there is a clean way I am not seeing to achieve this with the new routing system, which is totally possible (see #39), I am still fully onboard.

msonnabaum’s picture

Providing a different, language specific route that points to a different controller sounds reasonable to me. It appears that you could just subclass the existing form controller and override getFormLangcode() to return the language from the route.

That in no way seems like a hack to me.

plach’s picture

Title: Exploit hook_entity_prepare to set the form language without having to rely on the current language » Exploit hook_entity_prepare_form() to set the form language without having to rely on the current language

A couple of days ago I had an interesting discussion with Mark and Damien about this. To roughly summarize it there were two opposite positions, if I am not mistaken:

  1. As pointed out in #41, Mark is in favor of providing a new route in the following form: entity/{entity}/{language}/edit. I'd personally prefer to avoid swapping entity form controllers, since doing that in a generic way might be tricky and might also introduce the risk of module clashes. We agreed that relying on hook_entity_prepare_form() might be an acceptable alternative.
  2. Damien instead was suggesting to ditch the new route approach and just bake this capability into the HtmlEntityFormController (the route controller), relying on a query string, which he deemed to be more correct from a REST perspective in this case (I hope I got this right).

Honestly I am a bit disoriented after talking with them: both approaches look valid to me and both have pros and cons on the implementation level. If we could come to a consensus it would be great, but shouldn't we be able to, I'd prefer to keep the current approach (more or less #2), which would not require additional work :)

plach’s picture

FileSize
12.53 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, ct-prepare-1810394-43.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
12.48 KB

Re-roll from #38

Status: Needs review » Needs work

The last submitted patch, 1810394-entity-prepare-45.patch, failed testing.

plach’s picture

Assigned: alexpott » Unassigned
plach’s picture

Issue summary: View changes

Updated issue summary.

vijaycs85’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
12.46 KB
1.98 KB

Re-roll + fixing test fails...

The last submitted patch, 48: 1810394-entity-prepare-48.patch, failed testing.

plach’s picture

Schnitzel’s picture

Assigned: Unassigned » Schnitzel

working on this during Sprint Weekend 2014

Schnitzel’s picture

FileSize
13.59 KB

first a reroll.

Schnitzel’s picture

fixing failing tests

plach’s picture

@Schnitzel:

Thanks for working on this, but I am wondering whether it would make sense to postpone this on #2160965: Content entities are not upcast in the page language, inconsistent with config entities, which may imply that way to go here is the one proposed by Damien above (see #40 - #42).

The last submitted patch, 53: 1810394-entity-prepare-53.patch, failed testing.

The last submitted patch, 53: 1810394-entity-prepare-53.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 54: 1810394-entity-prepare-54.patch, failed testing.

Schnitzel’s picture

FileSize
2.25 KB
14.88 KB

Thanks for working on this, but I am wondering whether it would make sense to postpone this

oh well, my mind was already contexted for this, so I finished it.

some stuff is not really clear for me though:

there is
node/1/edit?content_translation_target=de
and also
node/1/translations/edit/de
but the later one is nowhere linked in the UI?

The problem is now that both use different systems to load the target language. I guess this is something bad.
Actually I had to do:

   public function getFormLangcode(array &$form_state) {
     if (empty($form_state['langcode'])) {
-      // Imply a 'view' operation to ensure users edit entities in the same
-      // language they are displayed. This allows to keep contextual editing
-      // working also for multilingual entities.
-      $form_state['langcode'] = $this->entityManager->getTranslationFromContext($this->entity)->language()->id;
+      // Try to load the target langcode from a query parameter. If this is not
+      // given, load it via current context.
+      $content_translation_target = \Drupal::request()->get('content_translation_target');
+      if (isset($content_translation_target)) {
+        $form_state['langcode'] = $content_translation_target;
+      }
+      else {
+        $form_state['langcode'] = $this->entityManager->getTranslationFromContext($this->entity)->language()->id;
+      }
     }
     return $form_state['langcode'];
   }

that the functionality works. But looks like in older Versions of 8.x the functionality worked without this. So some logic has changed that this is now necessary, but not sure what exactly has changed.

Schnitzel’s picture

Status: Needs work » Needs review
YesCT’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

I'm trying to reroll this now. (https://drupal.org/patch/reroll)

YesCT’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
14.85 KB
1.86 KB

just context lines changed. can see the conflicts in the txt file.

Status: Needs review » Needs work

The last submitted patch, 62: 1810394-entity-prepare-62.patch, failed testing.

vijaycs85’s picture

Another re-roll from #59

there is
node/1/edit?content_translation_target=de
and also
node/1/translations/edit/de
but the later one is nowhere linked in the UI?

Yep, one of them in deprecated/not cleaned up proper. @gabor might have a task for it.

vijaycs85’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 64: 1810394-entity-prepare-64.patch, failed testing.

Schnitzel’s picture

Assigned: Schnitzel » Unassigned

Unassigning, don't really have time to work on :/

Gábor Hojtsy’s picture

Issue tags: -sprint

Nobody is working on this one, back to the pool.

tstoeckler’s picture

Issue tags: +sprint

Putting this back on the board. Per #2451693: Operations links on the translations overview page do not link to the correct route I'm inclined to mark this as a bug, but not doing so yet. In any case, we could get this done.

tstoeckler’s picture

Assigned: Unassigned » tstoeckler
Issue tags: +drupaldevdays

Will take a stab at getting this up to speed.

tstoeckler’s picture

Assigned: tstoeckler » Unassigned

Unassigning, as @hchonov is now working on this.

hchonov’s picture

Assigned: Unassigned » hchonov
Status: Needs work » Needs review
FileSize
2.46 KB

This patch makes it possible to edit the entity in a desired language independent of the current content language.

However I am not sure if it will not be better to define a route for this case.

This should later be used to set the correct edit links on the content translation overview.

penyaskito’s picture

penyaskito’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

In this last patch we lost the tests modifications we already had on #64, so we need tests for this new approach.

I would love specific sign-off from @plach according to #42.

+++ b/core/modules/content_translation/content_translation.module
@@ -52,12 +52,18 @@ function content_translation_help($route_name, RouteMatchInterface $route_match)
+    // Move our entity_type_alter hook implementation to the end of the list.
...
+    // Move our entity_prepare_form hook implementation to the beginning of the list.

Why? I mean, we are describing what the code does instead of why are we doing that there.

hchonov’s picture

So I've included the test for the manual switch of the entity form language in ContentTranslationUITest.
The other tests for ContentTranslationWorkflowsTest are already adjusted in the other issue dealing with the broken translation links on the translation overview page -> #2451693: Edit links on the translations page are not pointing to the translation form.

I've also edited the comment about moving the entity_prepare_form hook implemenation to the beginning of the list:

// Move our entity_prepare_form hook implementation to the beginning of the list,
// so that the entity in the form object is exchanged with it's requested translation
// before any other hook implementations have had access on it.

hchonov’s picture

Status: Needs work » Needs review
penyaskito’s picture

Thanks for the quick response!
It looks great to me if green. Let's see if we can fix this minor nitpicks and I hope we can get @plach to look at it today.

  1. +++ b/core/modules/content_translation/content_translation.module
    @@ -52,12 +52,20 @@ function content_translation_help($route_name, RouteMatchInterface $route_match)
    +    // Move our entity_prepare_form hook implementation to the beginning of the list,
    +    // so that the entity in the form object is exchanged with it's requested translation
    +    // before any other hook implementations have had access on it.
    

    Nitpick: should fit in 80 char lines.

    s/it's/its/g

  2. +++ b/core/modules/content_translation/content_translation.module
    @@ -568,3 +576,28 @@ function content_translation_page_attachments(&$page) {
    \ No newline at end of file
    

    Nitpick.

penyaskito’s picture

Status: Needs review » Needs work
hchonov’s picture

@penyaskito Thank you for reviewing the patch.
Fixed the nitpicks, which you mentioned.

hchonov’s picture

Status: Needs work » Needs review
penyaskito’s picture

Status: Needs review » Needs work
+++ b/core/modules/content_translation/content_translation.module
@@ -568,3 +577,28 @@ function content_translation_page_attachments(&$page) {
+ * Provides an ability to edit an entity in a different language than the current content language.

This one is also > 80 chars.

Otherwise looks good to me.

hchonov’s picture

Thanks, I've changed that as well.

hchonov’s picture

Status: Needs work » Needs review
tstoeckler’s picture

Conflicted on #2473907: Tests not being run by testbot due to missing summary line and #2428795: Translatable entity 'changed' timestamps are not working at all, so rerolled.

I had another look at the patch vs. previous versions and I'm fairly confident that the current patch is sufficient. I will have another final look at this now and step through some things with a debugger and hopefully mark this RTBC then.

tstoeckler’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests +Needs issue summary update, +Needs beta evaluation

Was about to RTBC this, patch works great.

Unfortunately this needs an issue summary update and a beta evaluation.

plach’s picture

Mmh, unfortunately the current patch does not solve any concern brought up in #36, so I don't see how we can RTBC it again.

+++ b/core/modules/content_translation/content_translation.module
@@ -568,3 +577,28 @@ function content_translation_page_attachments(&$page) {
+      $entity_from_form_object = $form_object->getEntity();

Btw, why aren't we using just $entity here? It should be exactly the object stored in the form state.

plach’s picture

Assigned: hchonov » plach

Working on this

plach’s picture

Status: Needs work » Needs review
FileSize
7.66 KB
8.85 KB

Trying a new approach to address @alexpott's concerns about the request parameter: by adding a new language type we decouple the entity form from the request. I think it's ok to leave content_translation_module_implements_alter() because module weights do not allow to target single hooks. Instead module weight can be used to determine when mymodule_module_implements_alter() is run and thus act before CT, if needed.

Status: Needs review » Needs work

The last submitted patch, 88: ct-prepare-1810394-88.patch, failed testing.

hchonov’s picture

I do not understand your concerns about the route query parameter. We are not using different form for the translation like mentioned in #36.

So with or withouth the query parameter we still get the entity form. What only happens is that we exchange the entity used for generating the entity form with the target translation.

Please have a look at:
\Drupal\content_translation\Controller\ContentTranslationController::edit

$this->entityFormBuilder()->getForm($entity, $operation, $form_state_additions)
plach’s picture

I meant #37 (and following): @alexpott was not convinced by the current approach.

plach’s picture

Status: Needs work » Needs review
FileSize
9.36 KB
14.62 KB

This should fix test failures hopefully

Status: Needs review » Needs work

The last submitted patch, 92: ct-prepare-1810394-92.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
14.77 KB

Let's try again

plach’s picture

Assigned: plach » Unassigned
FileSize
3.4 KB
16.29 KB

Performed some final clean-up. I think it should fine to send this back to committers now, reviews permitting, as the current solution decouples the entity form from the request URL, although the basic mechanism remains the same. I think this is cleaner than implementing a separate route to edit the entity in a specific language, as it would be hard to replicate possible customizations to the page and access checks, because the entity form would be couple with original route in this case.

Reviews welcome :)

plach’s picture

Priority: Normal » Major

Mmh, and given the amount of time spent on this and the community interest around it, I'd say this is major.

plach’s picture

Updated IS and added beta evaluation.

plach’s picture

Moar clean-up!

plach’s picture

plach’s picture

@tstoeckler:

@penyaskito told me you have ideas™ about this and @Gábor Hojtsy said you could be the right person to review/RTBC this again. Your feedback would be greatly appreciated :)

tstoeckler’s picture

Yeah, sorry for being unavailable for this (and in general). Getting back to this now.

Just read up on a bunch of language module stuff to understand what is going on.

I like the approach with the new language type a lot as it allows more flexibility. I.e. if I want to determine the content language in a different way on my custom site this patch would now actually allow do that without any hackery by just implementing hook_language_type_info_alter() in a custom module.

The one thing I had reservations about - and I think that is what @penyaskito was referring to - is that the query parameter handling is not at all specific to content translation. It's totally conceivable that I would want to have query-parameter-based language handling for the interface language. So I was about to suggest that that be split into a generic "LanguageNegotiationQuery" plugin, and just have the content translation language type use that...

... until - upon reading up on some more of the language negotiation stuff - I realized that LanguageNegotiationSession is also abusing the query parameter to do language switching. I personally think that is very strange and convoluted (and - in fact - broken; will open an issue in a sec) but that is not something for us to solve at this moment.

The other thing that sort of bothered me is that we have to explicitly add the query parameter to the URL options instead of path processing handling that for us. This is because A) PathProcessorLanguage only cares about configurable language types (which seems strange), and B) because Url only allows us to specify a single language, but in this case we would want something like:

$options = [
  'language' => [
    'interface' => 'fr',
    'content_translation' => 'es',
  ],
];

(give or take some syntax details). I will open a follow-up about that.

The final thing I am not super excited about is the addition of the $form_state checks to content_translation_entity_form_prepare(). I was quite pleased that earlier versions of the patch were able to avoid that. @plach you re-added that with the comment

This should fix test failures hopefully

but could you be more specific on what problems arose without it? I also considered whether we could use the so far unused $operation in the hook for a check to see if we are on the edit form (i.e. if $operation were add, edit, translation-add, and translation-edit at the appropriate times) but it's always default (but for the deletion forms), so that's not possible (and a massive #fail).

So I would only like some feedback on the last item, otherwise I consider this RTBC.

Sorry again for taking so long.

tstoeckler’s picture

Yeah, sorry for being unavailable for this (and in general). Getting back to this now.

Just read up on a bunch of language module stuff to understand what is going on.

I like the approach with the new language type a lot as it allows more flexibility. I.e. if I want to determine the content language in a different way on my custom site this patch would now actually allow do that without any hackery by just implementing hook_language_type_info_alter() in a custom module.

The one thing I had reservations about - and I think that is what @penyaskito was referring to - is that the query parameter handling is not at all specific to content translation. It's totally conceivable that I would want to have query-parameter-based language handling for the interface language. So I was about to suggest that that be split into a generic "LanguageNegotiationQuery" plugin, and just have the content translation language type use that...

... until - upon reading up on some more of the language negotiation stuff - I realized that LanguageNegotiationSession is also abusing the query parameter to do language switching. I personally think that is very strange and convoluted (and - in fact - broken; will open an issue in a sec) but that is not something for us to solve at this moment.

The other thing that sort of bothered me is that we have to explicitly add the query parameter to the URL options instead of path processing handling that for us. This is because A) PathProcessorLanguage only cares about configurable language types (which seems strange), and B) because Url only allows us to specify a single language, but in this case we would want something like:

$options = [
  'language' => [
    'interface' => 'fr',
    'content_translation' => 'es',
  ],
];

(give or take some syntax details). I will open a follow-up about that.

The final thing I am not super excited about is the addition of the $form_state checks to content_translation_entity_form_prepare(). I was quite pleased that earlier versions of the patch were able to avoid that. @plach you re-added that with the comment

This should fix test failures hopefully

but could you be more specific on what problems arose without it? I also considered whether we could use the so far unused $operation in the hook for a check to see if we are on the edit form (i.e. if $operation were add, edit, translation-add, and translation-edit at the appropriate times) but it's always default (but for the deletion forms), so that's not possible (and a massive #fail).

So I would only like some feedback on the last item, otherwise I consider this RTBC.

Sorry again for taking so long.

plach’s picture

@tstoeckler:

Thanks for reviewing! No time for a proper reply now, just wanted to point out that #1137074: Make obtaining language-aware URLs less painful is aiming to do exactly what you are suggesting here:

[...] and B) because Url only allows us to specify a single language, but in this case we would want something like:

$options = [
  'language' => [
    'interface' => 'fr',
    'content_translation' => 'es',
  ],
];

So no need to open an issue for that. Very good point, btw :)

tstoeckler’s picture

Ahh thanks for the pointer. Will watch that. In the meantime opened #2505263: Session language switch links are (sometimes) broken.

plach’s picture

The one thing I had reservations about - and I think that is what @penyaskito was referring to - is that the query parameter handling is not at all specific to content translation.

The new language type is explicitly meant to determine the language of the entity form: I can't see anything inherently wrong with having a dedicated query string parameter for that. We have a new negotiation method that's tied to the language type so it's not available elsewhere. The regular negotiation method based on request/session is not affected at all by this, so it's unclear to me why you see this as problematic.

The other thing that sort of bothered me is that we have to explicitly add the query parameter to the URL options instead of path processing handling that for us.

We have a issue for that™ :)
#1137074: Make obtaining language-aware URLs less painful

The final thing I am not super excited about is the addition of the $form_state checks to content_translation_entity_form_prepare(). I was quite pleased that earlier versions of the patch were able to avoid that.

Again I'm not sure I get what's wrong with those checks: they are the only reliable way to tell whether we are editing an existing translation in an edit form instead of a translation form. This is the only case where content translation language should be taken into account, as in the other we already specify the form language, so we would get conflicts otherwise.

I added a check for the operation though.

Status: Needs review » Needs work

The last submitted patch, 105: ct-prepare-1810394-105.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
2.99 KB
15.62 KB

Fixed merge issue and reverted unnecessary changes.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

The new language type is explicitly meant to determine the language of the entity form: I can't see anything inherently wrong with having a dedicated query string parameter for that. We have a new negotiation method that's tied to the language type so it's not available elsewhere. The regular negotiation method based on request/session is not affected at all by this, so it's unclear to me why you see this as problematic.

The language type is inherently bound to the specific use case of translation forms, but the method to determine this - i.e. pulling the language code out of a query string parameter - is not conceptually bound to this at all. That's my reservation, i.e. in my perfect world the class would not be called ContentTranslationFormLanguage (but instead LanguageNegotiationQuery or similar) and have a configurable query parameter to look for the langcode. Then we could wire that up for the content translation language type. As I already noted in #102 this is currently not possible because LanguageNegotiationSession is already using the query parameter as well. So this is the best we can do at the moment. I just wanted to explain my comments above.

Again I'm not sure I get what's wrong with those checks: they are the only reliable way to tell whether we are editing an existing translation in an edit form instead of a translation form. This is the only case where content translation language should be taken into account, as in the other we already specify the form language, so we would get conflicts otherwise.

I would have liked to see a more concrete explanation of what actually breaks without those checks (not only here, but also in code) because previous patches got along fine without them, or at least some of them. Because this is really a minor point, though (one check more or less in an if statement that is pretty complex regardless), I'm setting this to RTBC. I've already stalled this issue long enough with my lack of response, so let's not hold this up any longer on really minor implementation details, when this is fixing a pretty major usability problem we currently have with content translation.

Thanks @plach!!! (and of course @hchonov!!!)

plach’s picture

The language type is inherently bound to the specific use case of translation forms, but the method to determine this - i.e. pulling the language code out of a query string parameter - is not conceptually bound to this at all.

Ah, now I get what you mean. In theory I agree with you, and we could split the session negotiation handler in request negotiation handler and a (dependent) session negotiation handler. To achieve that we would need to:

  • Introduce the concept of dependencies between negotiation methods, since otherwise a user enabling the session method would need to realize she needs to manually enable also the request one.
  • Introduce the ability of providing different configurations for various "instances" of the same negotiation method, likely via plugin derivatives.

Feasible, but definitely not trivial. Not sure the gain would be worth the effort :)

tstoeckler’s picture

Yeah, in any case that's far out of scope here (hence, the issue status ;-))

plach’s picture

I realized I owed you a better explanation for this:

I would have liked to see a more concrete explanation of what actually breaks without those checks (not only here, but also in code) because previous patches got along fine without them, or at least some of them.

In the previous patches we were accessing the request parameter directly from the hook implementation, so we could tell whether we had a value for it. By going via the language manager we can no longer do this because it will return the current language if no parameter is specified. However in the cases covered by the form state checks we are explicitly providing a form language already, so the current language would override it.

tstoeckler’s picture

Ahhhh thanks. I totally missed that, but it makes a lot of sense. Thanks for getting me up to speed.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/content_translation/content_translation.module
@@ -52,16 +53,37 @@ function content_translation_help($route_name, RouteMatchInterface $route_match)
+    // Move our entity_prepare_form hook implementation to the beginning of the
+    // list, so that the entity in the form object is exchanged with its
+    // requested translation before any other hook implementations have had
+    // access on it.
+    case 'entity_prepare_form':
+      $group = $implementations['content_translation'];
+      unset($implementations['content_translation']);
+      $implementations = array_merge(array('content_translation' => $group), $implementations);
+      break;
   }

This seems very fragile. What happens if another module does the same thing but the module name begins with d? Not sure what to do about this though. Shouldn't we just be setting an lower module weight here? Ah but lol sometimes we want content translation hooks at the end and sometimes first. Nice. How do we document this so that other hook implementations are aware of the possibility that they're going to break content_translation?

Gábor Hojtsy’s picture

Issue tags: -sprint

Does not seem to be worked on anymore :/ Even though this still blocks #2451693: Operations links on the translations overview page do not link to the correct route :(

hchonov’s picture

As @alexpot mentioned in #113 using the hook_entity_prepare_form brings a couple of misadvantages like if there is a contrib/custom module starting with 'd' and relying on $entity->translation()->getId().

So I would propose a different approach. Everything that is done in content_translation_entity_prepare_form is not dependent on the CT module, but is just basic funtionality except checking in form_state if we are on a CT translation form. What about if we move the code from that function to ContentEntityForm::prepareEntity? Content entities are translation aware and everything that happens in this function would be fine to exists in ContentEntityForm::prepareEntity.

Any opinion?

plach’s picture

I more or less agree with that, I was planning to make the new language type a core one instead of having it defined by CT and move the related logic into the ContentEntityForm. Didn't have time to dive into that yet, sorry :(

hchonov’s picture

We had a discussion with @alexpott, @plach and @mkalkbrenner and we decided that the right way to solve our problem will be to introduce a new language negotiator, which relies on a query parameter e.g. content_translation=it. If a url is called with that query parameter and the page contains Urls, which have an option parameter set, specifying that they should be overwritten, then they will inherit the content_translation query parameter as well.

At the moment the current patch will do this for all local tasks. It was desired to do it only for content entities, but I am not sure how I can check in hook_menu_local_tasks_alter if the current route is containing a content entity.

The content translation overview now also modifies the links and the edit link for german will look like /node/1/edit?content_translation=de. That works fine. But there is a problem with the add link "/node/1/translations/add/en/de?content_language=de". After the new translation is saved, a redirect will occur, but the Url for that redirect does not have the overwrite option set and so the redirect url will not inherit the content_language=de query parameter. For that to work I think we should overwrite all the outbound links without requiring that they have an url option set, which is allowing that they get overwritten.

And we also have problem with the caching. If we open the page "/node/1?content_language=de" with cold caches, than all the local task links will inherit the query parameter. But if we fist open "/node/1" and after that "/node/1?content_language=de", then the local task links will not inherit the query parameter.

Status: Needs review » Needs work

The last submitted patch, 117: 1810394-117.patch, failed testing.

The last submitted patch, 117: 1810394-117.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
22.18 KB
26.84 KB

So I encoutered the problem, that if we have to explictly define the url option parameter then we'll not be able to cover all the cases. For example for adding a german translation we have the following path : "/node/1/translations/add/en/de?content_language=de". But after saving the entity NodeForm::save is going to redirect to the entity view and there we do not have the ovewritte option set and so after saving the german translation we'll be redirected to the english translation.

We discussed with @plach and decided that we need a more general approach, so we came up with the conclusion to not use an overwritte option parameter. Instead the links have to set, like previously, the language option in the url and if the language negotiator ContentLanguage is being executed before the language negotiator Url then we are going to remove the language option so that the Url language negotiator does not change the interface language (the url) and while removing the language option we set the query parameter for the content language for all the outbound url links, which are defined in the entity's link templates. Thus I had to adjust all the routes added by CT to match the schema for link templates and then also define them as link templates.

The language negotiator ContentLanguage is turned off by default, but as soon as it is enabled and is configured with a higher priority than the language negotiator Url it is going to be used to change the content language.

We still have problems with the caching and need tests for the new language negotiator. I am also asking myself if we should give the developers the oportunity to add paths, which should also inherit the query parameter except the entity's link templates or they should always define link templates for this to happen.

Status: Needs review » Needs work

The last submitted patch, 120: 1810394-120.patch, failed testing.

The last submitted patch, 120: 1810394-120.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
23.42 KB
1.09 KB

So the tests were failing as now I am using the weight from the method definition and it was not set in the mock for the test.

hchonov’s picture

hchonov’s picture

Fixed the caching issue with the local task urls.

hchonov’s picture

In LanguageNegotiationContentLanguage::processOutbound we have to add the query parameter and along with this also alter the path and manually append the new query parameter, as UrlGenerator::generateFromRoute is not evaluating the new query parameters added by the outbound processors.

#2507005: UrlGenerator:processPath() doesn't use altered query parameters is fixing this and as soon as it is commited we do not have to manually alter the path in LanguageNegotiationContentLanguage::processOutbound anymore, but only set the query param in the options array.

hchonov’s picture

Created a follow up #2576945: PathProcessorLanguage::initProcessors is not sorting the methods by weight for sorting the processors in PathProcessorLanguage::initProcessors, so that the outbound functions are called in an order defined by the method weights. This will allow us to alter the options array in LanguageNegotiationContentLanguage::processOutbound and remove the language option, so that LanguageNegotiationUrl::processOutbound does not alter the url if LanguageNegotiationUrl is defined with a lower priority than LanguageNegotiationContentLanguage.

plach’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Looks good!

  1. +++ b/core/modules/content_translation/content_translation.module
    @@ -69,7 +69,6 @@ function content_translation_language_types_info_alter(array &$language_types) {
    -  unset($language_types[LanguageInterface::TYPE_CONTENT]['fixed']);
    

    Why do we need this?

  2. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentLanguage.php
    @@ -0,0 +1,147 @@
    +
    

    Surplus empty line.

  3. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentLanguage.php
    @@ -0,0 +1,147 @@
    +class LanguageNegotiationContentLanguage extends LanguageNegotiationMethodBase implements OutboundPathProcessorInterface, LanguageSwitcherInterface {
    

    Since this is now specifically about content entity language, what about naming it LanguageNegotiationContentEntity?

  4. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentLanguage.php
    @@ -0,0 +1,147 @@
    +  const QUERY_PARAMETER = 'content_language';
    

    Can we use language_content_entity? This way the parameter is properly namespaced.

  5. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentLanguage.php
    @@ -0,0 +1,147 @@
    +  public function processOutbound($path, &$options = array(), Request $request = NULL, BubbleableMetadata $bubbleable_metadata = NULL) {
    ...
    +          $query = array();
    

    Let's use the [] form here.

  6. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentLanguage.php
    @@ -0,0 +1,147 @@
    +      $route_provider = \Drupal::getContainer()->get('router.route_provider');
    

    Why not using \Drupal::service() here?

  7. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentLanguage.php
    @@ -0,0 +1,147 @@
    +      if (in_array($route_name_path, $this->getContentEntityPaths())) {
    

    I think we should add two more checks: 1) that the current route is also among the content entity routes 2) that the specified route has the same entity type as the current one. Otherwise we could switch from the node view page to the author user page and keep the parameter around.

  8. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentLanguage.php
    @@ -0,0 +1,147 @@
    +      if (in_array($route_name_path, $this->getContentEntityPaths())) {
    ...
    +          $this->contentEntityPaths = array_merge($this->contentEntityPaths, array_values($entity_type->getLinkTemplates()));
    

    This will be called a lot, it would be probably better to return a flipped array and use isset() instead of in_array.

  9. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentLanguage.php
    @@ -0,0 +1,147 @@
    +    if ($this->contentEntityPaths === NULL) {
    

    Normally we use !isset(), this makes me wonder whether something subtle is going on.

  10. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentLanguage.php
    @@ -0,0 +1,147 @@
    +        if ($entity_type->isSubclassOf('\Drupal\Core\Entity\ContentEntityInterface')) {
    
    +++ b/core/tests/Drupal/Tests/Core/PathProcessor/PathProcessorTest.php
    @@ -134,7 +135,10 @@ function testProcessInbound() {
    +        'class' => 'Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrl',
    

    Can we use ::class here?

We also need test coverage for the new method.

hchonov’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
44.49 KB
34.79 KB

@plach:

1. my mistake we do not have to remove this line.
2. removed surplus empty line.
3. renamed.
4. renamed.
5. changed to [].
6. instead of \Drupal::service() now I am implementing the ContainerFactoryPluginInterface.
7. added both conditions.
8. fixed.
9. fixed.
10. fixed.

Also provided test coverage, which I hope is sufficient?

Sorry for the interdiff not being much helpful.....

Status: Needs review » Needs work

The last submitted patch, 129: 1810394-129.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
44.49 KB
1.15 KB

a small mistake in the configuration check.. now corrected and it should be green again :).

plach’s picture

Status: Needs review » Needs work

Nice, last remarks!

  1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
    @@ -622,7 +622,7 @@
    -    $form_state->setRedirect("entity.$entity_type_id.content_translation_add", array(
    +    $form_state->setRedirect('content_translation.translation_add_' . $entity_type_id, array(
    
    @@ -652,14 +652,14 @@
    -      $form_state->setRedirect("entity.$entity_type_id.content_translation_delete", [
    +      $form_state->setRedirect('content_translation.translation_delete_' . $entity_type_id, [
    

    Are these correct?

  2. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,281 @@
    +   * Implements Drupal\Core\PathProcessor\InboundPathProcessorInterface::processOutbound().
    

    {@inheritdoc}, I'd say

  3. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,281 @@
    +   * @param $outbound_route_name
    

    Missing param type

  4. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,281 @@
    +  protected function meetsContentEntityRoutesCondition($outbound_route_name, Request $request) {
    

    We should really try to statically cache also this method. Maybe an SplObjectStorage keyed by request and having associative arrays of bools keyed by outbound route names as values, so we can perform the following check:

    isset($cache[$request][outbound_route_name])
    
  5. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,281 @@
    +      if (isset($this->getContentEntityPaths()[$outbound_route_path]) && $this->getContentEntityPaths()[$outbound_route_path]) {
    

    I think this can be simplified:

      if (!empty($this->getContentEntityPaths()[$outbound_route_path])) {
    
  6. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,281 @@
    +      $entity_types = \Drupal::entityManager()->getDefinitions();
    

    Can we inject also the entity manager?

  7. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,281 @@
    +        if ($entity_type->isSubclassOf('\Drupal\Core\Entity\ContentEntityInterface')) {
    

    Can we use ::class here?

  8. +++ b/core/modules/language/src/Tests/EntityUrlLanguageTest.php
    @@ -37,33 +49,108 @@ protected function setUp() {
    +     $this->assertTrue(strpos($this->entity->urlInfo()->toString(), '/en/entity_test/' . $this->entity->id()) !== FALSE);
    +     $this->assertTrue(strpos($this->entity->getTranslation('es')->urlInfo()->toString(), '/es/entity_test/' . $this->entity->id()) !== FALSE);
    +     $this->assertTrue(strpos($this->entity->getTranslation('fr')->urlInfo()->toString(), '/fr/entity_test/' . $this->entity->id()) !== FALSE);
    

    Wrong indentation

  9. +++ b/core/modules/language/src/Tests/EntityUrlLanguageTest.php
    @@ -37,33 +49,108 @@ protected function setUp() {
    +   * Ensures correct entity URLs with the method language-content-entity enabled.
    +   *
    +   * Test case with the method language-content-entity enabled and configured with
    +   * higher and also with lower priority than the method language-url.
    

    80 chars

  10. +++ b/core/modules/language/src/Tests/EntityUrlLanguageTest.php
    @@ -37,33 +49,108 @@ protected function setUp() {
    +    // Without being on an content entity route the default entity url tests
    +    // should still pass.
    +    $this->testEntityUrlLanguage();
    

    Nice!

  11. +++ b/core/modules/language/src/Tests/LanguageNegotiationContentEntityTest.php
    @@ -0,0 +1,189 @@
    +    $request = Request::create($path);
    +    $request->attributes->set(RouteObjectInterface::ROUTE_NAME, $route_name);
    +    $request->attributes->set(RouteObjectInterface::ROUTE_OBJECT, new Route($path));
    +    \Drupal::requestStack()->push($request);
    ...
    +  protected function rebuildContainer() {
    +    $this->kernel->rebuildContainer();
    +    $this->container = $this->kernel->getContainer();
    +    \Drupal::setContainer($this->container);
    +  }
    

    It would be nice to move these two bits of logic to WebTestBase so they can be reused.

The last submitted patch, 131: 1810394-131.patch, failed testing.

The last submitted patch, 129: 1810394-129.patch, failed testing.

The last submitted patch, 131: 1810394-131.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
45.58 KB
10.28 KB

@plach:

1. yes, I adjusted them accordingly as well in the ContentTranslationRouteSubscriber
2. corrected.
3. corrected.
4. implemented the SplObjectStorage static cache.
5. corrected.
6. yes, done.
7. yes ,done.
8. corrected.
9. corrected.
10. :)
11. moved both to Testbase.

Both EntityUrlLanguageTest and LanguageNegotiationContentEntityTest pass locally. Let's see what the testbot thinks about it :).

Status: Needs review » Needs work

The last submitted patch, 136: 1810394-136.patch, failed testing.

The last submitted patch, 136: 1810394-136.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
44.34 KB
9.41 KB

Personally I think we should be ready to go :).

hchonov’s picture

Title: Exploit hook_entity_prepare_form() to set the form language without having to rely on the current language » Introduce a new language negotiator to allow for independent content language for content entities
Issue summary: View changes

Updating the IS to the approach we agreed on during DrupalCon.

hchonov’s picture

Issue summary: View changes
hchonov’s picture

Title: Introduce a new language negotiator to allow for independent content language for content entities » Site configuration with different domains for each language results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language
Category: Task » Bug report
Issue summary: View changes

Changing category to bug report, as at the moment if a site is configured with a different domain for each language, then a link to a specific content entity translation will redirect authenticated users to a different domain.

Status: Needs review » Needs work

The last submitted patch, 139: 1810394-139.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
45.02 KB
3.98 KB

Adding a more detailed information message to the assertions and instead of calling $this->drupalGet() with $translation->urlInfo()->toString() I call it with $translation->urlInfo(), so that the function generates itself the url string. Lets see what happens. Locally everything is green.

plach’s picture

A few more nitpicks:

  1. +++ b/core/modules/content_translation/content_translation.module
    @@ -53,7 +53,7 @@ function content_translation_help($route_name, RouteMatchInterface $route_match)
    +    // Move our entity_type_alter hook implementation to the end of the list.
    

    our hook_entity_type_alter() implementation...

  2. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
    @@ -207,13 +210,15 @@ protected function doTestBasicTranslation() {
    +        $elements = $this->xpath('//table//a[@href=:href]', array(':href' => $view_url));
    

    Please let's use [] instead of array().

  3. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,304 @@
    +namespace Drupal\language\Plugin\LanguageNegotiation;
    +
    +
    +use Drupal\Component\Utility\UrlHelper;
    +use Drupal\Core\Entity\EntityManagerInterface;
    +use Drupal\Core\PathProcessor\OutboundPathProcessorInterface;
    

    Surplus blank line

  4. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,304 @@
    +        $path .= (strpos($path, '?') !== FALSE ? '&' : '?') . UrlHelper::buildQuery($query_addon);
    

    It would be good to add a @todo to remove this once #2507005: UrlGenerator:processPath() doesn't use altered query parameters is fixed.

  5. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,304 @@
    +   * @return bool
    ...
    +   * @return bool
    ...
    +   * @return string
    ...
    +   * @return bool
    

    Missing return description.

  6. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,304 @@
    +        $check_interface_method = isset($enabled_methods_interface[LanguageNegotiationUrl::METHOD_ID]) ? TRUE : FALSE;
    

    Can be just isset(...), no need for the ternary operator.

  7. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,304 @@
    +    $storage = $this->paths->offsetExists($request) ? $this->paths->offsetGet($request) : [];
    ...
    +      $this->paths->offsetSet($request, $storage);
    

    We can use the array syntax here.

hchonov’s picture

@plach:
thanks. fixed all of the points.

hchonov’s picture

FileSize
45.48 KB
3.71 KB

renamed a misleading function name and description....

plach’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +rc deadline

Thanks!

I guess this is either RC1 or 8.1 now...

alexpott’s picture

What about existing sites and the upgrade path?

plach’s picture

@alexpott:

The new method is disabled by default also on new installations.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Sorry too tired to do a full review but this really jumps out...

+++ b/core/modules/language/src/Tests/EntityUrlLanguageTest.php
@@ -37,33 +46,86 @@ protected function setUp() {
+    $this->rebuildContainer();

+++ b/core/modules/simpletest/src/TestBase.php
@@ -1586,4 +1589,28 @@ public function getTempFilesDirectory() {
+  /**
+   * Force a container rebuild.
+   */
+  protected function rebuildContainer() {
+    // Rebuild the kernel and bring it back to a fully bootstrapped state.
+    $this->container = $this->kernel->rebuildContainer();
+  }
+

This is a sign of something fishy. We should not need to do this in a KernelTest. What service is supposed to be refreshed after changing the url prefixes?

dawehner’s picture

// In order to reflect the changes for a multilingual site in the container
// we have to rebuild it.

This test adds a language so we need to rebuild the container due to the usage of multiple languages.

plach’s picture

Status: Needs review » Reviewed & tested by the community

What Daniel said :)

Tentatively moving back to RTBC...

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Yes but there are other KernelTests that have rebuilt the container - none saw fit to do a wider change. Which I agree with because as far as I know you don't actually need to rebuild the container here you just need to do $this->container = \Drupal::getContainer() because there are config listeners that cause a container rebuild when a language is installed. So essentially this test is doing an unnecessary container rebuild.

hchonov’s picture

Status: Needs work » Needs review

Exchanging $this->rebuildContainer(); with $this->container = \Drupal::getContainer(); makes all the tests fail, which require a refresh of the services.

The container rebuild in the setup function is needed as @dawehner said and the other container rebuilds during the tests are needed so that the processors array list in \Drupal\language\HttpKernel\PathProcessorLanguage is newly generated with the methods sorted differently according to the new configuration and without a container rebuild they are not collected again.

hchonov’s picture

@alexpott suggested in IRC to not change the WebTestBase and TestBase classes as it is not in the scope of this issue. I adjusted the patch accordingly. Hope this looks better now.

Status: Needs review » Needs work

The last submitted patch, 157: 1810394-157.patch, failed testing.

hchonov queued 157: 1810394-157.patch for re-testing.

plach’s picture

Status: Needs work » Reviewed & tested by the community

@alexpott's suggestion has been implemented, back to RTBC

YesCT’s picture

read through the issue, the proposed resolution wording is... pretty complicated. but this is complicated.

read also through the patch a few times, found some nits which this fixes (and should not effect any functionality).

didn't see anything to block this. so leaving at rtbc.

  1. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,308 @@
    +   * Determines the execution permission based on the current configuration.
    ...
    +  protected function meetsConfigurationCondition() {
    

    not sure about the naming of this method. (not blocking)

  2. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,308 @@
    +      $enabled_methods_content = $this->config->get('language.types')->get('negotiation.language_content.enabled') ? : [];
    

    no space between ?:

    ag "\?:" core | wc -l
    413
    vs.
    ag "\? :" core | wc -l
    4

  3. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,308 @@
    +   *   The array of the link templates paths of all content entities, as keys.
    

    I'm not sure what are the keys and what are the values. (not blocking)

    wait it says right above it.

    I'll combine.

  4. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,308 @@
    +  }
    +}
    

    https://www.drupal.org/node/608152#indenting

    blank line before closing brace of a class.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 161: 1810394.161.patch, failed testing.

YesCT’s picture

Status: Needs work » Reviewed & tested by the community

old testbot,

CollapsedDrupal\book\Tests\BookTest	969	1	0
Message	Group	Filename	Line	Function	Status
The test did not complete due to a fatal error.	Completion check	BookTest.php	585	Drupal\book\Tests\BookTest->testSaveBookLink()

I just dont believe you.

hchonov queued 161: 1810394.161.patch for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
    @@ -310,6 +310,7 @@ public function generateFromRoute($name, $parameters = array(), $options = array
    +    $options['route_name'] = $name;
    

    This seems a pretty big one line change - making this available to path processors. Is it really necessary - is this not available form something else? I think this is the only change in the patch that means we have to do this change in core and can not achieve the desired outcome with a contrib module.

  2. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,307 @@
    +  /**
    +   * Checks criteria for outbound processing.
    +   *
    +   * @var bool
    +   */
    +  protected $meetsConfigurationCondition;
    ...
    +  protected function meetsConfigurationCondition() {
    

    I agree with YesCT this is pretty tricky name - condition is a pretty overloaded word in Drupal 8. How about checkLanguageNegiotationOrder()? Also the protected property checks nothing it is a static cache of the check - which begs the question is it clearer correctly when language negotiation is re-ordered in a request.

  3. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,307 @@
    +    // Check if processing conditions are met.
    +    if (!($request && isset($options['route_name']) && ($outbound_route_name = $options['route_name']) && $this->meetsConfigurationCondition() && $this->meetsContentEntityRoutesCondition($outbound_route_name, $request))) {
    +      return $path;
    +    }
    

    This is a pretty complex if that is checking multiple things and setting a variable. Can this be simplified - I'm not sure why it is even doing ($outbound_route_name = $options['route_name']) and not just using $options['route_name']

  4. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,307 @@
    +      $meets_condition = $content_entity_type_id_for_current_route ? $this->meetsOutboundRouteToCurrentContentEntityCondition($outbound_route_name, $request) : FALSE;
    ...
    +  /**
    +   * Determines if outbound route points to the current entity.
    +   *
    +   * @param $outbound_route_name
    +   *   The route name for the current outbound url being processed.
    +   * @param \Symfony\Component\HttpFoundation\Request $request
    +   *   The HttpRequest object representing the current request.
    +   *
    +   * @return bool
    +   *   TRUE if the outbound route points to the entity on the current route,
    +   *   FALSE otherwise.
    +   */
    +  protected function meetsOutboundRouteToCurrentContentEntityCondition($outbound_route_name, Request $request) {
    +    if ($content_entity_type_id_for_current_route = $this->getContentEntityTypeIdForCurrentRequest($request)) {
    +      $outbound_route_path = $this->routeProvider->getRouteByName($outbound_route_name)->getPath();
    +      if (!empty($this->getContentEntityPaths()[$outbound_route_path]) && $content_entity_type_id_for_current_route == $this->getContentEntityPaths()[$outbound_route_path]) {
    +        return TRUE;
    +      }
    +    }
    +
    +    return FALSE;
    +  }
    

    This method could be inlined it is only called once from here. And it repeats some of the code in the calling code.

hchonov’s picture

Title: Site configuration with different domains for each language results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language » 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
Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
hchonov’s picture

Status: Needs work » Needs review
FileSize
51.19 KB
14.29 KB

@alexpott:

1. Unfortunately there is no other possibility to do it.
2. fixed naming and solved the problem with changing config during a request by adding additional logic for this case in the ConfigSubscriber in the language module to reset the LanguageNegotiator and the PathProcessorLanguage services. This helped to remove the container rebuild calls in the EntityUrlLanguageTest which extends KernelTestBase
3. simplified.
4. method inlined.

Status: Needs review » Needs work

The last submitted patch, 170: 1810394-170.patch, failed testing.

alexpott’s picture

  1. Nice work on getting of the container rebuilds. This is exactly how things should work.
  2. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,296 @@
    +  /**
    +   * Checks criteria for outbound processing.
    +   *
    +   * @var bool
    +   */
    +  protected $checkLanguageNegotiationOrder;
    

    This protected does not check anything - it is a static cache of the check.

  3. +++ b/core/modules/language/src/HttpKernel/PathProcessorLanguage.php
    @@ -152,4 +164,23 @@ protected function initProcessors($scope) {
    +  /**
    +   * Initializes the injected event subscriber with the language path processor.
    +   *
    +   * The language path processor service is registered only on multilingual
    +   * site configuration, thus we inject it in the event subscriber only when
    +   * it is initialized.
    +   */
    +  public function initConfigSubscriber() {
    +    $this->configSubscriber->setPathProcessorLanguage($this);
    +  }
    

    Can't we inject the path processor language into the config event rather than doing it this way around? Hmmm no because this is only created when the language manager is multilingual.

  4. +++ b/core/modules/language/src/HttpKernel/PathProcessorLanguage.php
    @@ -152,4 +164,23 @@ protected function initProcessors($scope) {
    +    debug('reset path processor language');
    

    Debug left in

The last submitted patch, 170: 1810394-170.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
52.5 KB
3.98 KB

2. Changed the comment and the variable name.
3. Exactly, it happens only if the site is multilingual.
4. oups... removed.

and also fixed the failing PathProcessorTest.

hchonov’s picture

YesCT’s picture

+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
@@ -52,11 +52,11 @@
-  protected $checkLanguageNegotiationOrder;
+  protected $languageNegotiationOrderCheckResult;

since it is a bool, maybe name it like

isLanguageNegotiationOrderChecked

maybe I'm not clear on what the value is representing yet.

it's set around...

"Check if the content language is configured to be dependent on the
// url negotiator directly or indirectly over the interface negotiator."

isLanguageNegotiationOrderOverridden

??

[edit: ]
I see. something like:

   /**
    * Static cache for the language negotiation order check.
    *
+   * @see \Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationContentEntity::checkLanguageNegotiationOrder()
+   *
    * @var bool
    */
   protected $languageNegotiationOrderCheckResult;

would add clarity.

YesCT’s picture

what about something like this?

[edit:
(changes the name of the function, and the docs about what the return value means... and thus the name of the property that caches the value.)

snippit:

    * @return bool
-   *   TRUE if the configuration condition is met, FALSE otherwise.
+   *   TRUE if the the content entity language negotiator has higher priority
+   *   than the url language negotiator, FALSE otherwise.
    */
-  protected function checkLanguageNegotiationOrder() {
-    if (!isset($this->languageNegotiationOrderCheckResult)) {
+  protected function hasHigherLanguageNegotiationOrder() {
+    if (!isset($this->hasHigherLanguageNegotiationOrderResult)) {

]
if yes, [edit: there might be some other places in this patch to do something similar.]

hchonov’s picture

@YesCT:
yes, it looks and sounds better now :).

+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
@@ -178,16 +180,20 @@ public function getLanguageSwitchLinks(Request $request, $type, Url $url) {
+   * should be executed to remove the language option, so that url negotiator

Maybe for completeness we could write "should be executed to add a query parameter to the url and remove the language option, so that the url negotiator".

YesCT’s picture

Even though I added that myself,
I'm a little iffy on if we are documenting what another method is going to do here.

if the docs on the other method need to change, I'm not sure people will know to change it here also.
maybe we can improve the docs on the other method and write something more generic here. and point to the other docs.

YesCT’s picture

YesCT’s picture

  1. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,304 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function processOutbound($path, &$options = [], Request $request = NULL, BubbleableMetadata $bubbleable_metadata = NULL) {
    +    // If appropriate, process outbound to add a query parameter to the url and
    +    // remove the language option, so that url negotiator does not rewrite the
    +    // url.
    

    since should not put docs *in* an @inheritdoc, this section is to give an overview for what this implementation does.

  2. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,304 @@
    +   * Determines if content entity language negotiator has higher priority.
    +   *
    +   * The content entity language negotiator having higher priority than the url
    +   * language negotiator, is a criteria in
    +   * \Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationContentEntity::processOutbound().
    +   *
    +   * @return bool
    +   *   TRUE if the the content entity language negotiator has higher priority
    +   *   than the url language negotiator, FALSE otherwise.
    +   */
    +  protected function hasHigherLanguageNegotiationOrder() {
    

    then I didn't have to document processOutbound here. :)

hchonov’s picture

YesCT++

looks much better now.

Status: Needs review » Needs work

The last submitted patch, 180: 1810394.180.patch, failed testing.

hchonov queued 180: 1810394.180.patch for re-testing.

YesCT’s picture

Status: Needs work » Needs review

but new bot says green.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
    @@ -310,6 +310,7 @@ public function generateFromRoute($name, $parameters = array(), $options = array
    +    $options['route_name'] = $name;
    

    I think this should be documented on OutboundPathProcessorInterface

  2. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,304 @@
    +   * Determines if content entity language negotiator has higher priority.
    ...
    +   *   TRUE if the the content entity language negotiator has higher priority
    +   *   than the url language negotiator, FALSE otherwise.
    ...
    +        $this->hasHigherLanguageNegotiationOrderResult = $enabled_methods_content[static::METHOD_ID] < $check_against_weight;
    

    We say higher priority and then check which has the lowest weight. This is because the lower the weight the higher the priority. And this gets really confusing when you read this line... $this->hasHigherLanguageNegotiationOrderResult = $enabled_methods_content[static::METHOD_ID] < $check_against_weight;
    Which ends up setting the property to TRUE when the number is lower :)

hchonov’s picture

Status: Needs work » Needs review
FileSize
54.85 KB
8.33 KB

1. I've extended the documentation in OutboundPathProcessorInterface.
2. I am really confused how we should name the variable and also the function. It is just like this in Drupal, lower weight means higher priority.....

I've optimized and simplified the patch a little bit by removing the RouteProvider from the new language negotiator LanguageNegotiationContentEntity. Now instead of providing the route name in the options, there will be the route object. Thus there are less function calls.

Gábor Hojtsy’s picture

Issue tags: -rc deadline +rc target

I guess this cannot be an RC deadline because then it would need to now be postponed to 8.1.x or 9.x. As per @alexpott's support above, retagging as rc target.

alexpott’s picture

Issue tags: -rc target +rc target triage

The committers still need to agree and discuss this.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Also, to have committers to consider it for inclusion in RC, tag the issue with rc target triage and add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. You can add a section <h3>Why this should be an RC target</h3> to the summary. Thanks!

hchonov’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Status: Needs review » Needs work

The last submitted patch, 188: 1810394-188.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
56.06 KB
979 bytes

I think the routes have to be rebuild as their names changed and the container should be rebuild as the PathProcessorLanguage and ConfigSubscriber now have additional parameters in the constructor.

hchonov’s picture

Oups and we have to run the updates in the RC1 test...

The last submitted patch, 194: 1810394-194.patch, failed testing.

plach’s picture

Issue summary: View changes
  1. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -13,13 +13,13 @@
    +use Symfony\Component\Routing\Route as SymfonyRoute;
    

    Why do need the alias?

  2. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -61,11 +61,11 @@
    +   * Static cache of outbound routes per request.
    

    I'd say "outbound route *paths*", otherwise the variable name is confusing.

  3. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,294 @@
    + *   description = @Translation("Determines the translation language from a request parameter."),
    

    Can we use "content language" also in the description for consistency?

  4. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
    @@ -0,0 +1,294 @@
    +  protected function hasHigherLanguageNegotiationOrder() {
    +    if (!isset($this->hasHigherLanguageNegotiationOrderResult)) {
    ...
    +      // being executed before the outbound function of LanguageNegotiationUrl.
    +      $enabled_methods_content = $this->config->get('language.types')->get('negotiation.language_content.enabled') ?: [];
    +
    +      // Check if the content language is configured to be dependent on the
    +      // url negotiator directly or indirectly over the interface negotiator.
    +      if (isset($enabled_methods_content[LanguageNegotiationUrl::METHOD_ID]) && ($enabled_methods_content[static::METHOD_ID] > $enabled_methods_content[LanguageNegotiationUrl::METHOD_ID])) {
    +        $this->hasHigherLanguageNegotiationOrderResult = FALSE;
    +      }
    +      else {
    +        $check_interface_method = FALSE;
    +        if (isset($enabled_methods_content[LanguageNegotiationUI::METHOD_ID])) {
    +          $enabled_methods_interface = $this->config->get('language.types')->get('negotiation.language_interface.enabled') ?: [];
    +          $check_interface_method = isset($enabled_methods_interface[LanguageNegotiationUrl::METHOD_ID]);
    +        }
    +        if ($check_interface_method) {
    +          $check_against_weight = $enabled_methods_content[LanguageNegotiationUI::METHOD_ID];
    +          $check_against_weight = isset($enabled_methods_content[LanguageNegotiationUrl::METHOD_ID]) ? max($check_against_weight, $enabled_methods_content[LanguageNegotiationUrl::METHOD_ID]) : $check_against_weight;
    +        }
    +        else {
    +          $check_against_weight = isset($enabled_methods_content[LanguageNegotiationUrl::METHOD_ID]) ? $enabled_methods_content[LanguageNegotiationUrl::METHOD_ID] : PHP_INT_MAX;
    +        }
    +
    +        $this->hasHigherLanguageNegotiationOrderResult = $enabled_methods_content[static::METHOD_ID] < $check_against_weight;
    +      }
    +    }
    +
    +    return $this->hasHigherLanguageNegotiationOrderResult;
    +  }
    

    I spoke with @alexpott in IRC about this: we agreed that since the weight terminology is well understood and is used through all the language negotiation code it's ok to standardize on it here too. Thus this would become hasLowerLanguageNegotiationWeight().

    Also to improve readability, I'd suggest to rename $enabled_methods_content to $content_method_weights (same for interface) and $check_against_weight to $max_weight.

plach’s picture

Status: Needs review » Needs work
hchonov’s picture

@plach:

Thank you again for your review.

1. I've included Route as SymfonyRoute because it is included in the URLGenerator the same way and I thought of naming consistency, but I'll include it without an alias now.
2. Fixed.
3. Fixed.
4. Fixed.

Status: Needs review » Needs work

The last submitted patch, 199: 1810394-199.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
56.81 KB
726 bytes

I guess we have to clear the cached entity type definitions as well, but it is strange that the previous test didn't fail....

Status: Needs review » Needs work

The last submitted patch, 201: 1810394-201.patch, failed testing.

hchonov’s picture

The patch from #199 is the correct one, but unfortunatelly the test is failing not because of the current patch, but the test is not passing on a clean head as well - #2607364: RC1 filled database update fails due to beta-12 to RC1 upgrade path issue.

hchonov’s picture

Status: Needs work » Needs review

Setting to Needs Review as actually the test should be passing as it already passed in #195 and afterwards only variable naming changed.

The last submitted patch, 195: 1810394-195.patch, failed testing.

The last submitted patch, 199: 1810394-199.patch, failed testing.

hchonov’s picture

Status: Needs review » Needs work

The patch from #2602996: Upgrade path for #2597628 solves the failing test here.

plach’s picture

Status: Needs work » Postponed
hchonov’s picture

Status: Postponed » Needs review
FileSize
55.96 KB
6.8 KB
hchonov’s picture

Status: Needs review » Reviewed & tested by the community

Moving back to RTBC as of #208.

The last submitted patch, 195: 1810394-195.patch, failed testing.

plach’s picture

RTBC +1

plach’s picture

Status: Reviewed & tested by the community » Needs review

Sorry, I didn't realize you posted a new patch in #209.

+++ a/core/modules/system/src/Tests/Update/UpdatePathRC1TestBaseFilledTest.php
@@ -30,7 +30,8 @@
+    // @todo there are no updates to run.
+    //$this->runUpdates();
-    $this->runUpdates();
 

This looks wrong...

The last submitted patch, 195: 1810394-195.patch, failed testing.

plach’s picture

+++ b/core/modules/language/src/EventSubscriber/ConfigSubscriber.php
@@ -102,6 +121,25 @@ public function onConfigSave(ConfigCrudEvent $event) {
+    elseif($saved_config->getName() == 'language.types' && $event->isChanged('negotiation')) {

Missing space after elseif

hchonov’s picture

@plach:
This line is only in the interdiff, but not in the patch itself.

This happens because in the previous patch I had to turn the updates on, but now they are turned on by default in the HEAD.

However we have to remove the todo line for executing the updates in the RC1 test.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, no functional change so back to RTBC, hopefully the bot will agree...

The last submitted patch, 195: 1810394-195.patch, failed testing.

xjm’s picture

Assigned: Unassigned » alexpott

@effulgentsia and I looked at this briefly but did not want to make a call on whether it should go in during RC without @alexpott, so assigning to him and we will discuss the issue when he is available.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 216: 1810394-216.patch, failed testing.

plach’s picture

Status: Needs work » Reviewed & tested by the community

Bot fluke

The last submitted patch, 195: 1810394-195.patch, failed testing.

alexpott’s picture

Assigned: alexpott » Unassigned

I'm +1 on this change because it solves a really hard to solve problem for people using url based language negotiation. It also makes the content_translation module add proper link templates for the entity routes it provides which will make contrib's life much easier when it comes to extending the UI and building additional functionality.

I will discuss with @xjm and @effulgentsia and see if we can make this an rc target.

xjm’s picture

Issue tags: -rc target triage +rc target, +Needs change record

@alexpott, @effulgentsia, and I agreed on this being an rc target. There is a disruption with the change of route names, and we need a change record for it.

alexpott’s picture

Issue tags: -Needs change record
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0e507e4 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/content_translation/content_translation.install b/core/modules/content_translation/content_translation.install
index 0dfd587..d44b2d1 100644
--- a/core/modules/content_translation/content_translation.install
+++ b/core/modules/content_translation/content_translation.install
@@ -31,8 +31,17 @@ function content_translation_enable() {
 }
 
 /**
+ * @addtogroup updates-8.0.0-rc
+ * @{
+ */
+
+/**
  * Rebuild the routes as the content translation routes have now new names.
  */
 function content_translation_update_8001() {
   \Drupal::service('router.builder')->rebuild();
 }
+
+/**
+ * @} End of "addtogroup updates-8.0.0-rc".
+ */

Added on commit

Wim Leers’s picture

+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
@@ -0,0 +1,294 @@
+        $bubbleable_metadata
+          // - varied by the content language query parameter.
+          ->addCacheContexts(['url.query_args:' . static::QUERY_PARAMETER]);

:)

plach’s picture

Yay, thanks!

hchonov’s picture

@plach, @alexpott: Thanks for all the reviews, the support and the help making this happen!

  • alexpott committed 0e507e4 on 8.1.x
    Issue #1810394 by hchonov, plach, YesCT, David Hernández, Schnitzel,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

raduungurean’s picture

FileSize
121.61 KB

I'm not able to keep the same domain when editing translations.
translation redirect to the corresponding domain

Am I doing something wrong or is this an issue again?

I'm using Drupal 8.3.4.

However it seems to be working when going to the main node and clicking translate (is adding language_content_entity in the url).

Thanks

hchonov’s picture

@raduungurean, what you observe is the desired behavior - core only adds the language to links pointing to the entity on which route you currently are which means you have to be on node/5 in order to have links pointing to e.g. node/5/edit and in this case the link will contain the language.

But what you except is already covered in contrib, please try the module language_negotiator_content_entity_all_routes.

nwellnhof’s picture

@hchonov But language_negotiator_content_entity_all_routes seems to add language_content_entity to all URLs, not only admin pages. This makes the module useless for me.

"Major problem A" from the issue description still isn't fixed. Language detection by domain is still unusable if you have "example.de" and "example.com" domains because you can't share cookies and having different domains on admin pages forces you to login multiple times. What's really needed is separate language detection mechanisms for admin and public pages.