Problem/Motivation
Assume the interface language is english and a new entity e.g. node is created with only one default translation - german. In \Drupal\node\NodeForm::save() there is a redirect where the entity language isn't put into the url options, which doesn't allow the language negotiation methods to properly prepare the url so that when it is visited the entity language and the content type language both are german. As this is not the case when the url is visited then the EntityConverter will initiate the language detection, but as the request miss any metadata the interface language will be used as content language leading to seeing the entity in german but having the language manager reporting content type language being english.
I think we have a lot of problems in this direction therefore I think this issue should be considered major.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #43 | 2940992-43.patch | 47.69 KB | smustgrave |
| #43 | interdiff-39-43.txt | 3.44 KB | smustgrave |
Comments
Comment #2
hchonovAdding an assertion to
\Drupal\Core\Entity\ContentEntityForm::initFormLangcodes(), which should check that the current entity language and the content type language match.Comment #4
hchonovIt looks like, at least locally with
PHP 7.0.27, that PHP cannot correctly evaluate this assertion. PHP bug? I've rewritten it to use a callback which is now working locally.Comment #5
hchonovI've found a couple of places where I think the language option should be set when building an entity url.
Comment #6
hchonovIt looks pretty complicated and I am not sure how we should proceed here, but if the content type language couldn't be relied on, then probably we should deprecate it.
Assigning to @plach for his opinion and attention on this subject :).
Comment #7
tstoecklerI think here and elsewhere we should use
$entity->toUrl()instead, which already adds the language option, at least where possible. That both improves readibility of the code and fixes these language related problems.Comment #10
hchonov@tstoeckler, you are right :). I've adjusted this where it is possible, but we have places where we don't have a link relation even if the route points to an entity. This is the case for the route
forum.page. Should we add a link relation for this route to the taxonomy term in ahook_entity_type_alterimplementation in the forum module?What do you think about the changes in the user module, where I check if the user entity has a translation for the current interface language and and if so use it, otherwise fallback to the default entity translation of the user entity. I thought about running the user entity through
EntityRepository::getTranslationFromContext(), but I think that in this case the user entity routes will be different if the content type language changes and I am not sure if we want this as this might happen if the user just visits the e.g. spanish translation of a random entity.P.S. I haven't uploaded an interdiff, because I had to rewrite almost everything :).
Comment #12
hchonovComment #13
tstoecklerI'm not necessarily opposed to this, but generally speaking this is not always possible because the route parameters that are necessary for a particular link relation (i.e.
Entity::urlRouteParameters()) cannot be extended in any way, so if you need an additional route parameter for a link relation you are out of luck. That's why I generally stick to those link relations defined by the entity itself. In this case it would actually work, though, because just the term is required as a parameter and it would make the affected code quite a bit nicer. So don't feel strongly either way, not really sure.I'm sorry but I can't quite follow that. Can you elaborate what your concerns with
::getTranslationFromContext()are?Comment #15
hchonovConsider the user visits a german translation of an entity and then the link pointing to the user profile will point to the german translation of the user entity.
Then the user visits the spanish translation of an entity and the link to the user profile might change and point to the spanish translation of the user entity.
I am just not sure this is the correct behavior.
Comment #16
hchonovI've found some more places where we should use the entity to build the url.
Comment #18
hchonovWe have to add the language option to the node add links as well. However changing the language in the the language widget should be changing the form action to which the form will be posted, which doesn't happen currently. This is the second time I encounter problems with the language widget - the first was that when it is changed the form langcode is not changed, so I guess we would need another issue for that.
Comment #21
hchonovHmm probably we should rely on the entity repository to retrieve the translation language from the context?
Comment #23
hchonovComment #24
tstoecklerRerolled this one. Also took a look at the patch.
Let's call this
$entityRepositoryas that's what's actually being used here. We can still assing the entity manager, for now, but that will reduce the things we have to update once we properly deprecate the entity manager.Minor, but now that this is using the same method, the
setRedirectUrl()call could be moved outside of the if-else and we could only assing$url = ...in the if-else.Comment #27
geaseRe-roll of the previous patch.
Comment #28
geaseThis update fixes tests (sometimes this required changing tests logic, because on entity translation save the redirect is now to the translation, not to the original entity), sometimes changing the patch itself (eg, for the user, whih is also a content entity, logic on login should be different - language should be kept), sometimes removing deprecations in the patch, or changing mocks and expectations for unit tests.
Comment #29
geaseComment #30
geaseAnother attempt to fix all tests. And, are we sure that on user login and redirect to user profile, we want to keep content language, not the interface language?
Comment #31
geaseSimple reroll for 8.8.x
Comment #33
hchonovWe've noticed that on user logins the user preferred langcode is not considered, which this patch is trying to change. It also introduced some minor cleanups.
Comment #36
sanjayk commentedRe-roll patch #33 on 8.9
Comment #37
sanjayk commentedNow fixed issues partially working on rest of issue once fix. I will update here.
Comment #38
sanjayk commentedComment #39
sanjayk commentedRe-roll for 9.2.
Not getting any clue how to fix (FATAL Drupal\Tests\language\Functional\LanguageUrlRewritingTest: test runner returned a non-zero error code (2).) error if anybody have any idea let me know. Thanks
Comment #43
smustgrave commentedRerolled for 9.5 and fixed some phpcs errors.
The IS appears to be incomplete.
Comment #44
smustgrave commentedFor the issue summary.