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:
- An authenticated user goes to the translate overview "https://en.example.com/node/1/translations".
- The user decides to view or edit the french translation of the node entity.
- 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".
- This is a different domain, where the user is not logged in.
B. Site configuration with url based language negotiation
Steps to reproduce:
- An only english speaking authenticated user goes to the translate overview "https://example.com/en/node/1/translations".
- The user wants to edit the french translation of the node entity and just exchange e.g. a picture in the content.
- 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".
- 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
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.
Comment | File | Size | Author |
---|---|---|---|
#216 | 209-216-interdiff.txt | 1.33 KB | hchonov |
#216 | 1810394-216.patch | 56.61 KB | hchonov |
#209 | 195-209-interdiff.txt | 6.8 KB | hchonov |
#209 | 1810394-209.patch | 55.96 KB | hchonov |
#195 | 1810394-195.patch | 56.73 KB | hchonov |
|
Comments
Comment #1
plachObviously we need to introduce a
hook_entity_prepare()
call inEntityFormController::preprareEntity()
.Comment #2
plachComment #3
plachI'll work on this.
Comment #4
plachActually this is more like it.
Comment #5
plachActually postponed on #2025991: Introduce hook_entity_prepare_form() to generalize hook_node_prepare(), which should be an easy one, reviews welcome.
Comment #6
Gábor HojtsyComment #7
plachComment #8
David Hernández CreditAttribution: David Hernández commentedInstead 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.
Comment #9
plachLooks good, thanks! Just some minor remarks:
We don't need the full namespace for the entity interface, it's already imported through the use statment.
I was thinking that it might be better to namespace the parameter names to avoid conflicts:
content_translation_source
/Request()
should be lowercase :)Comment #10
David Hernández CreditAttribution: David Hernández commentedHere'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.
Comment #11
plachLooks great, thanks. Unfortunately the hook issue has been pushed back atm.
Comment #12
David Hernández CreditAttribution: David Hernández commentedPostponed 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.
Comment #13
plachMaybe, because it is
hook_entity_prepare_form
? ;)(unless you just mistyped it :)
Comment #14
David Hernández CreditAttribution: David Hernández commentedYou 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.
Comment #15
plachMmh, 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.
Comment #16
plachThe parent issue has been committed. Let's go on with this.
Comment #17
plachWorking a bit on this.
Comment #18
plachGot a bit distracted but I have finally something to post.
Comment #20
plachThis should fix the failures.
Comment #21
penyaskitoReviewed code and only found two minor typos I fixed myself.
lanaguage => language
inital => initial
Is this still needed?
Otherwise looks OK and well tested, so I would say RTBC.
Comment #22
plachI 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 ;)
Comment #23
penyaskitoOK, RTBC then if green (that should be because only comments were changed). Thanks @plach!
Comment #24
plachRerolled after #1810370: Entity Translation API improvements. As promised the @todo mentioned in #21 is gone :)
Comment #25
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #26
alexpottThis 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
Comment #27
plachNot 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.
I'm looking forward to reading your feedback :)
Comment #28
plachComment #29
plachImproved inline comments. Setting back to RTBC while waiting for more indications.
Comment #30
plachWrong patch, sorry.
Comment #31
Gábor HojtsyAt least tag it to identify its an API change.
Comment #32
Gábor HojtsyActually make sure proposed API changes have the API change tag.
Comment #32.0
Gábor HojtsyUpdated issue summary copied over orig issue summary text.
Comment #33
plachI 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.
Comment #33.0
plachUpdated issue summary
Comment #33.1
plachUpdated issue summary
Comment #33.2
plachUpdated issue summary.
Comment #33.3
plachUpdated issue summary.
Comment #34
plachThis is more like an API addition as old URLs are still working the same way.
Comment #35
YesCT CreditAttribution: YesCT commentedcontent_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.
Comment #36
alexpottThere 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.
This is just smelly... we have the module weight system for this and here we say well whatever content_translation comes first.
Comment #37
plachI 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.
Comment #38
plachKeeping up with HEAD while I wait to talk with timplunkett and dawhener.
Comment #39
plachFound #2061811: Only dynamic routes are available to route subscribers while working on this after talking with @dawehner.
Comment #40
plachAfter 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.
Comment #41
msonnabaum CreditAttribution: msonnabaum commentedProviding 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.
Comment #42
plachA 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:
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 onhook_entity_prepare_form()
might be an acceptable alternative.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 :)
Comment #43
plachRerolled
Comment #45
vijaycs85Re-roll from #38
Comment #47
plachComment #47.0
plachUpdated issue summary.
Comment #48
vijaycs85Re-roll + fixing test fails...
Comment #50
plachComment #51
plachComment #52
Schnitzel CreditAttribution: Schnitzel commentedworking on this during Sprint Weekend 2014
Comment #53
Schnitzel CreditAttribution: Schnitzel commentedfirst a reroll.
Comment #54
Schnitzel CreditAttribution: Schnitzel commentedfixing failing tests
Comment #55
plach@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).
Comment #59
Schnitzel CreditAttribution: Schnitzel commentedoh 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:
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.
Comment #60
Schnitzel CreditAttribution: Schnitzel commentedComment #61
YesCT CreditAttribution: YesCT commentedI'm trying to reroll this now. (https://drupal.org/patch/reroll)
Comment #62
YesCT CreditAttribution: YesCT commentedjust context lines changed. can see the conflicts in the txt file.
Comment #64
vijaycs85Another re-roll from #59
Yep, one of them in deprecated/not cleaned up proper. @gabor might have a task for it.
Comment #65
vijaycs85Comment #67
Schnitzel CreditAttribution: Schnitzel commentedUnassigning, don't really have time to work on :/
Comment #68
Gábor HojtsyNobody is working on this one, back to the pool.
Comment #69
tstoecklerPutting 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.
Comment #70
tstoecklerWill take a stab at getting this up to speed.
Comment #71
tstoecklerUnassigning, as @hchonov is now working on this.
Comment #72
hchonovThis 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.
Comment #73
penyaskitoBlocking #2451693: Operations links on the translations overview page do not link to the correct route, so tagging as blocker.
Comment #74
penyaskitoIn 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.
Why? I mean, we are describing what the code does instead of why are we doing that there.
Comment #75
hchonovSo 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:
Comment #76
hchonovComment #77
penyaskitoThanks 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.
Nitpick: should fit in 80 char lines.
s/it's/its/g
Nitpick.
Comment #78
penyaskitoComment #79
hchonov@penyaskito Thank you for reviewing the patch.
Fixed the nitpicks, which you mentioned.
Comment #80
hchonovComment #81
penyaskitoThis one is also > 80 chars.
Otherwise looks good to me.
Comment #82
hchonovThanks, I've changed that as well.
Comment #83
hchonovComment #84
tstoecklerConflicted 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.
Comment #85
tstoecklerWas about to RTBC this, patch works great.
Unfortunately this needs an issue summary update and a beta evaluation.
Comment #86
plachMmh, unfortunately the current patch does not solve any concern brought up in #36, so I don't see how we can RTBC it again.
Btw, why aren't we using just
$entity
here? It should be exactly the object stored in the form state.Comment #87
plachWorking on this
Comment #88
plachTrying 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 whenmymodule_module_implements_alter()
is run and thus act before CT, if needed.Comment #90
hchonovI 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
Comment #91
plachI meant #37 (and following): @alexpott was not convinced by the current approach.
Comment #92
plachThis should fix test failures hopefully
Comment #94
plachLet's try again
Comment #95
plachPerformed 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 :)
Comment #96
plachMmh, and given the amount of time spent on this and the community interest around it, I'd say this is major.
Comment #97
plachUpdated IS and added beta evaluation.
Comment #98
plachMoar clean-up!
Comment #99
plachBtw, can anyone RTBC #2494767: Rename MenuLinkContentUITest to MenuLinkContentTranslationUITest, pleeze :)
Comment #100
plach@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 :)
Comment #101
tstoecklerYeah, 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) becauseUrl
only allows us to specify a single language, but in this case we would want something like:(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 tocontent_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 commentbut 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
wereadd
,edit
,translation-add
, andtranslation-edit
at the appropriate times) but it's alwaysdefault
(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.
Comment #102
tstoecklerYeah, 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) becauseUrl
only allows us to specify a single language, but in this case we would want something like:(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 tocontent_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 commentbut 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
wereadd
,edit
,translation-add
, andtranslation-edit
at the appropriate times) but it's alwaysdefault
(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.
Comment #103
plach@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:
So no need to open an issue for that. Very good point, btw :)
Comment #104
tstoecklerAhh thanks for the pointer. Will watch that. In the meantime opened #2505263: Session language switch links are (sometimes) broken.
Comment #105
plachThe 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.
We have a issue for that™ :)
#1137074: Make obtaining language-aware URLs less painful
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.
Comment #107
plachFixed merge issue and reverted unnecessary changes.
Comment #108
tstoecklerThe 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 insteadLanguageNegotiationQuery
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 becauseLanguageNegotiationSession
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.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!!!)
Comment #109
plachAh, 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:
Feasible, but definitely not trivial. Not sure the gain would be worth the effort :)
Comment #110
tstoecklerYeah, in any case that's far out of scope here (hence, the issue status ;-))
Comment #111
plachI realized I owed you a better explanation for this:
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.
Comment #112
tstoecklerAhhhh thanks. I totally missed that, but it makes a lot of sense. Thanks for getting me up to speed.
Comment #113
alexpottThis 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?
Comment #114
Gábor HojtsyDoes 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 :(
Comment #115
hchonovAs @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?
Comment #116
plachI 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 :(Comment #117
hchonovWe 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.
Comment #120
hchonovSo 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.
Comment #123
hchonovSo 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.
Comment #124
hchonovComment #125
hchonovFixed the caching issue with the local task urls.
Comment #126
hchonovIn 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.
Comment #127
hchonovCreated 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.
Comment #128
plachLooks good!
Why do we need this?
Surplus empty line.
Since this is now specifically about content entity language, what about naming it
LanguageNegotiationContentEntity
?Can we use
language_content_entity
? This way the parameter is properly namespaced.Let's use the
[]
form here.Why not using
\Drupal::service()
here?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.
This will be called a lot, it would be probably better to return a flipped array and use
isset()
instead ofin_array
.Normally we use
!isset()
, this makes me wonder whether something subtle is going on.Can we use
::class
here?We also need test coverage for the new method.
Comment #129
hchonov@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.....
Comment #131
hchonova small mistake in the configuration check.. now corrected and it should be green again :).
Comment #132
plachNice, last remarks!
Are these correct?
{@inheritdoc}
, I'd sayMissing param type
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:I think this can be simplified:
Can we inject also the entity manager?
Can we use
::class
here?Wrong indentation
80 chars
Nice!
It would be nice to move these two bits of logic to
WebTestBase
so they can be reused.Comment #136
hchonov@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 :).
Comment #139
hchonovPersonally I think we should be ready to go :).
Comment #140
hchonovUpdating the IS to the approach we agreed on during DrupalCon.
Comment #141
hchonovComment #142
hchonovComment #143
hchonovChanging 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.
Comment #145
hchonovAdding 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.
Comment #146
plachA few more nitpicks:
our hook_entity_type_alter() implementation...
Please let's use [] instead of array().
Surplus blank line
It would be good to add a @todo to remove this once #2507005: UrlGenerator:processPath() doesn't use altered query parameters is fixed.
Missing return description.
Can be just
isset(...)
, no need for the ternary operator.We can use the array syntax here.
Comment #147
hchonov@plach:
thanks. fixed all of the points.
Comment #148
hchonovrenamed a misleading function name and description....
Comment #149
plachThanks!
I guess this is either RC1 or 8.1 now...
Comment #150
alexpottWhat about existing sites and the upgrade path?
Comment #151
plach@alexpott:
The new method is disabled by default also on new installations.
Comment #152
alexpottSorry too tired to do a full review but this really jumps out...
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?
Comment #153
dawehnerThis test adds a language so we need to rebuild the container due to the usage of multiple languages.
Comment #154
plachWhat Daniel said :)
Tentatively moving back to RTBC...
Comment #155
alexpottYes 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.Comment #156
hchonovExchanging
$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.Comment #157
hchonov@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.
Comment #160
plach@alexpott's suggestion has been implemented, back to RTBC
Comment #161
YesCT CreditAttribution: YesCT commentedread 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.
not sure about the naming of this method. (not blocking)
no space between ?:
ag "\?:" core | wc -l
413
vs.
ag "\? :" core | wc -l
4
I'm not sure what are the keys and what are the values. (not blocking)
wait it says right above it.
I'll combine.
https://www.drupal.org/node/608152#indenting
blank line before closing brace of a class.
Comment #163
YesCT CreditAttribution: YesCT commentedold testbot,
I just dont believe you.
Comment #165
alexpottThis 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.
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.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']
This method could be inlined it is only called once from here. And it repeats some of the code in the calling code.
Comment #166
hchonovComment #167
alexpottComment #168
alexpottComment #169
alexpottComment #170
hchonov@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.
Comment #172
alexpottThis protected does not check anything - it is a static cache of the check.
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.
Debug left in
Comment #174
hchonov2. 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.
Comment #175
hchonovComment #176
YesCT CreditAttribution: YesCT commentedsince 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:
would add clarity.
Comment #177
YesCT CreditAttribution: YesCT commentedwhat 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:
]
if yes, [edit: there might be some other places in this patch to do something similar.]
Comment #178
hchonov@YesCT:
yes, it looks and sounds better now :).
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".
Comment #179
YesCT CreditAttribution: YesCT commentedEven 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.
Comment #180
YesCT CreditAttribution: YesCT commentedaddresses #179
Comment #181
YesCT CreditAttribution: YesCT commentedsince should not put docs *in* an @inheritdoc, this section is to give an overview for what this implementation does.
then I didn't have to document processOutbound here. :)
Comment #182
hchonovYesCT++
looks much better now.
Comment #185
YesCT CreditAttribution: YesCT commentedbut new bot says green.
Comment #186
plachThanks, back to RTBC
Comment #187
alexpottI think this should be documented on OutboundPathProcessorInterface
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 :)
Comment #188
hchonov1. 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.
Comment #189
Gábor HojtsyI 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.
Comment #190
alexpottThe committers still need to agree and discuss this.
Comment #191
xjmAlso, 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!Comment #192
hchonovComment #194
hchonovI 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.
Comment #195
hchonovOups and we have to run the updates in the RC1 test...
Comment #197
plachWhy do need the alias?
I'd say "outbound route *paths*", otherwise the variable name is confusing.
Can we use "content language" also in the description for consistency?
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
.Comment #198
plachComment #199
hchonov@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.
Comment #201
hchonovI guess we have to clear the cached entity type definitions as well, but it is strange that the previous test didn't fail....
Comment #203
hchonovThe 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.
Comment #204
hchonovSetting to Needs Review as actually the test should be passing as it already passed in #195 and afterwards only variable naming changed.
Comment #207
hchonovThe patch from #2602996: Upgrade path for #2597628 solves the failing test here.
Comment #208
plachOk, I'll move this back to RTBC, once #2607364: RC1 filled database update fails due to beta-12 to RC1 upgrade path issue is fixed.
Comment #209
hchonov#2607364: RC1 filled database update fails due to beta-12 to RC1 upgrade path issue got in. Rerolling the patch.
Comment #210
hchonovMoving back to RTBC as of #208.
Comment #212
plachRTBC +1
Comment #213
plachSorry, I didn't realize you posted a new patch in #209.
This looks wrong...
Comment #215
plachMissing space after
elseif
Comment #216
hchonov@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.
Comment #217
plachThanks, no functional change so back to RTBC, hopefully the bot will agree...
Comment #219
xjm@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.
Comment #221
plachBot fluke
Comment #223
alexpottI'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.
Comment #224
xjm@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.
Comment #225
alexpottI've created the CR https://www.drupal.org/node/2614380
Comment #226
alexpottCommitted 0e507e4 and pushed to 8.0.x. Thanks!
Added on commit
Comment #227
Wim Leers:)
Comment #228
plachYay, thanks!
Comment #229
hchonov@plach, @alexpott: Thanks for all the reviews, the support and the help making this happen!
Comment #232
raduungurean CreditAttribution: raduungurean commentedI'm not able to keep the same domain when editing translations.
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
Comment #233
hchonov@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.
Comment #234
nwellnhof CreditAttribution: nwellnhof commented@hchonov But
language_negotiator_content_entity_all_routes
seems to addlanguage_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.