Problem/Motivation

Currently when editing an entity translation, entity form 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).

Proposed resolution

  • Define a new non-configurable language type for the content translation form language.
  • Negotiate its value via a new detection method, using query string parameters, e.g. node/1/edit?content_translation_target=fr.
  • Exploit hook_entity_prepare_form() to set the entity form language based on the negotiated value. Use the current content language just as a fallback, if no query parameter is specified.

Remaining tasks

  • Post a patch to implement
  • Do reviews, update or write tests, evaluate if documentation or a change notice is needed.

User interface changes

When editing an entity through its regular entity form, URLs like http://example.org/node/1/edit have now a query string parameter: http://example.org/node/1/edit?content_translation_target=it.

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 Task because there is no specific functionality broken, however this blocks 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.
Files: 
CommentFileSizeAuthor
#98 ct-prepare-1810394-98.patch15.53 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,138 pass(es).
[ View ]
#98 ct-prepare-1810394-98.interdiff.txt1.18 KBplach
#95 ct-prepare-1810394-95.patch16.29 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,134 pass(es).
[ View ]
#95 ct-prepare-1810394-95.interdiff.txt3.4 KBplach
#94 ct-prepare-1810394-94.patch14.77 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,072 pass(es).
[ View ]
#94 ct-prepare-1810394-94.interdiff.txt3.68 KBplach
#92 ct-prepare-1810394-92.patch14.62 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,658 pass(es), 2 fail(s), and 25 exception(s).
[ View ]
#92 ct-prepare-1810394-92.interdiff.txt9.36 KBplach
#88 ct-prepare-1810394-88.patch8.85 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,097 pass(es), 255 fail(s), and 105 exception(s).
[ View ]
#88 ct-prepare-1810394-88.interdiff.txt7.66 KBplach
#84 entity_prepare_edit_translation-1810394-82.patch4.02 KBtstoeckler
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,659 pass(es).
[ View ]
#64 1810394-entity-prepare-64.patch14.64 KBvijaycs85
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,885 pass(es), 114 fail(s), and 54 exception(s).
[ View ]
#62 conflicts.txt1.86 KBYesCT
#62 1810394-entity-prepare-62.patch14.85 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 63,181 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#59 1810394-entity-prepare-59.patch14.88 KBSchnitzel
PASSED: [[SimpleTest]]: [MySQL] 63,139 pass(es).
[ View ]
#59 interdiff-54-59.txt2.25 KBSchnitzel
#54 1810394-entity-prepare-54.patch13.67 KBSchnitzel
FAILED: [[SimpleTest]]: [MySQL] 62,577 pass(es), 0 fail(s), and 6 exception(s).
[ View ]
#54 interdiff-53-54.txt2.21 KBSchnitzel

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
StatusFileSize
new899 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,418 pass(es).
[ View ]

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

StatusFileSize
new41.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch exploit-hook_entity_presave-2025991-34+1810394-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new919 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,416 pass(es).
[ View ]

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

StatusFileSize
new41.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch exploit-hook_entity_presave-2025991-34+1810394-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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
StatusFileSize
new11.35 KB
FAILED: [[SimpleTest]]: [MySQL] 56,670 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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
StatusFileSize
new13.49 KB
PASSED: [[SimpleTest]]: [MySQL] 56,411 pass(es).
[ View ]
new3.07 KB

This should fix the failures.

penyaskito’s picture

StatusFileSize
new13.49 KB
PASSED: [[SimpleTest]]: [MySQL] 56,452 pass(es).
[ View ]
new945 bytes

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

StatusFileSize
new12.77 KB
PASSED: [[SimpleTest]]: [MySQL] 57,005 pass(es).
[ View ]

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
StatusFileSize
new57.33 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new1.5 KB

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

plach’s picture

StatusFileSize
new13 KB
PASSED: [[SimpleTest]]: [MySQL] 56,777 pass(es).
[ View ]

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] => 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] => 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

StatusFileSize
new13 KB
PASSED: [[SimpleTest]]: [MySQL] 57,888 pass(es).
[ View ]

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

StatusFileSize
new12.53 KB
FAILED: [[SimpleTest]]: [MySQL] 58,403 pass(es), 363 fail(s), and 90 exception(s).
[ View ]

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
StatusFileSize
new12.48 KB
FAILED: [[SimpleTest]]: [MySQL] 58,969 pass(es), 325 fail(s), and 84 exception(s).
[ View ]

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
StatusFileSize
new12.46 KB
FAILED: [[SimpleTest]]: [MySQL] 58,857 pass(es), 23 fail(s), and 13 exception(s).
[ View ]
new1.98 KB

Re-roll + fixing test fails...

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

plach’s picture

plach’s picture

Schnitzel’s picture

Assigned:Unassigned» Schnitzel

working on this during Sprint Weekend 2014

Schnitzel’s picture

StatusFileSize
new13.59 KB
FAILED: [[SimpleTest]]: [MySQL] 62,508 pass(es), 23 fail(s), and 18 exception(s).
[ View ]

first a reroll.

Schnitzel’s picture

StatusFileSize
new2.21 KB
new13.67 KB
FAILED: [[SimpleTest]]: [MySQL] 62,577 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

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

StatusFileSize
new2.25 KB
new14.88 KB
PASSED: [[SimpleTest]]: [MySQL] 63,139 pass(es).
[ View ]

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
StatusFileSize
new14.85 KB
FAILED: [[SimpleTest]]: [MySQL] 63,181 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new1.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

StatusFileSize
new14.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,885 pass(es), 114 fail(s), and 54 exception(s).
[ View ]

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: Edit links on the translations page are not pointing to the translation form 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
StatusFileSize
new2.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,589 pass(es).
[ View ]

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

StatusFileSize
new4.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,318 pass(es).
[ View ]

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

StatusFileSize
new3.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,352 pass(es).
[ View ]

@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

StatusFileSize
new3.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,521 pass(es).
[ View ]
new1.46 KB

Thanks, I've changed that as well.

hchonov’s picture

Status:Needs work» Needs review
tstoeckler’s picture

StatusFileSize
new4.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,659 pass(es).
[ View ]

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
StatusFileSize
new7.66 KB
new8.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,097 pass(es), 255 fail(s), and 105 exception(s).
[ View ]

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

<?php
$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
StatusFileSize
new9.36 KB
new14.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,658 pass(es), 2 fail(s), and 25 exception(s).
[ View ]

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
StatusFileSize
new3.68 KB
new14.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,072 pass(es).
[ View ]

Let's try again

plach’s picture

Assigned:plach» Unassigned
StatusFileSize
new3.4 KB
new16.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,134 pass(es).
[ View ]

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

StatusFileSize
new1.18 KB
new15.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,138 pass(es).
[ View ]

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:

<?php
$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:

<?php
$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:

<?php
$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.