The language of content entities incorrectly falls back to the default language of the content whilst ignoring the current language of the user

I'm adding 2 patches:

One patch to prove this is failing on the ImageFormatter and another one with a potential fix.

After discussing this with @alexpott @berdir and @phenaproxima I'm trying to get closer to the right approach here:

According to Berdir (correct me if I'm wrong) it's a no-go to change the EntityBase::language() which would be API changing. Possibly we can discuss the option to change the toUrl method on the ContentEntity.

We can either try to do that, or resolve the language in each formatter before $entity->toUrl() is called.

Personally I think $entity->toUrl();should just resolve to the right language. When there's no language context whatsoever, then it should resolve to the current language of the user. But I realise cannot oversee all the implications.

I'm uploading two patches

1. A fail patch to prove that it's currently failing on the ImageFormatter and on the MediaThumbnailFormatter
2. A patch to propose to override the toUrl() method in the content entity - mainly to see what other tests this might break.

Tests are failing because of the testbot subdirectory.

Uploading them again, and if everything goes right, from a testbot perspective, we only have to review 4 failing tests.

Accidentally misnamed the patches, but the failtest actually contains the fix and vice versa.

Accidentally misnamed the patches, but the failtest actually contains the fix and vice versa.

This is not even true (it must have been late). Both patches in #8 in seem to be exactly the same, and are probably failing for other reasons.

Sending two new patches for the testbot to report back.

Since all the 'child issues' are caused by the same problem, writing tests for all different issues would result in practically the same tests for all different image formatters. I'm not sure if this is necessary. Love to hear feedback about this.

Instead of testing this at the level of an image formatter, I'm adding a test to the LanguageNegotiationContentEntityTest to test the correct 'fall back' behaviour when ::toUrl is called without passing any language context but the language prefix in the URL.

The tests are failing in #10 basically, as far as I can understand, there is no way to know for sure what the intention is when calling to ::toUrl(). Either:

1. When no language context is given, fall back to the default language of the entity (currently the case and cause of many issues reported) - or
2. When no language context given, fall back to the current language of the user (using language negotiation) - currently not the way it works, but how I think it should work.

However in some cases, you want to target the translation (and change the language prefix). This is why my patch in #10 breaks FieldEntityLinkBaseTest, because the intention there is not to fall back to the user's language, but to the default language of the entity, since we're listing translations of the same node.

Not troubled by a very in dept knowledge of the Entity API, nor whether or not it would be possible to add such a change, I would say that it needs to be possible to mark the entity as 'called in a translation context', so that the URL resolves correctly when necessary. But that in all other cases, the current user language is assumed.

I'm also not sure about the naming of the methods added.

Anyway, as a strating point and proof of concept I'm uploading this new patch.

After a couple of iterations I finally have a patch that doesn't break things.

Some (perhaps some superfluous) observations:

1. The patch changes the default language fallback of content entities to the current user language (using language negotiation) instead of the default language (x-default) of the entity.
2. The patch allows entities to keep on resolving to the right language when they are indeed language specific. That's the case when the entity is loaded from EntityRepository::getTranslationFromContext() or from ContentEntityBase::getTranslation.
3. To flag an entity as 'LanguageSpecific' a property was added to the ContentEntityBase with corresponding getter and setter.
4. A test was added to prove that things are currently not working (https://www.drupal.org/files/issues/2019-07-11/3061761-10-fail.patch)

Questions:

1. Should we change the naming of the 'translation' property, maybe to 'languageSpecific' and the corresponding methods, eg ::markAsTranslation to markAsLanguageSpecific?
2. Should we add further tests to prove that this solves the linked issues or is the test added to this issue enough?

I don't know if this kind of API change/addition is acceptable, but I think all the linked issues prove that this is something that needs to be addressed in a more generic way. I think I can further iterate on this but it's probably best for some of the core maintainers to say something about this. I would love to receive some feedback.

I deleted a line in an existing test that should have been deleted. On my localhost the test at issue continues to pass.

@charginghawk

DOH! I'm doing bits and pieces on my holiday destination, lot's of distractions...Obviously not focussed enough. Let me revise the whole thing once again.

Setting this to 'needs work'

Uploading two new patches:

- One fail patch with the working code commented out.
- One patch that does work.

The patch takes into account that the testbot runs in a subdirectory, which in previous patches meant that the patches failed, even though they were meant to pass. I've tried this with a local d8 installation in a subdirectory. Hope it works the same with the testbot.

I've also run the patch locally with the tests that failed in #25 and #23, and locally they seem to pass.

I'm also toying around with the naming of things: in this patch I'm proposing to call content entities 'Language Aware' and 'Language unaware', the difference being that when the entity is language unaware, it should inherit the language from the current user language, and not from it's own default language.

Let's see what the test bot returns.

• The testbot doesn't specify why TaxonomyFieldTidTest::testViewsHandlerTidField fails in #34, but it's because of a mismatch in the hreflang attribute in the link, which is added by LinkGenerator::generate (110). (Why a hreflang is added in a non multilingual installation is food for another issue).

  The hreflang attribute is added based on the #options['language'] of the Url object, which defaults to the entities default language. See EntityBase::toUrl.

  In this patch I'm also marking entities whose requested translation is LANGUAGE_NOT_SPECIFIED as 'language unaware' . This stops TaxonomyFieldTidTest::testViewsHandlerTidField from failing. However, it would break TermIndexTest::testTaxonomyTermHierarchyBreadcrumbs, for similar reasons.

• The test in TermIndexTest::testTaxonomyTermHierarchyBreadcrumbs would not add any hreflang attributes currently. However with the patch, since we're explicitly adding the language to the #options array in ContentEntityBase, so the 'hreflang' attribute gets added to the expected result by EntityBase::toUrl, and not to the actual result, where ::toUrl() is not used, but Link::createFromRoute().

  That's why I'm also making sure that the 'hreflang' attribute is not added on calling ::toUrl on a language unaware entity.

• Finally, I've added a condition that the working code only applies on mutlilingual sites by checking ::isMultilingual().

I realise that this fix is far from a solution. There are just so many things that tie in together, that it's hard to know what we could potentially break, and whether or not it's compliant with the way things were meant to work.

Things to take into account:

1. I haven't had the chance to look into language cache context. Not sure if something else needs to be done.
2. The EntityRepository::getTranslationFromContext actually seems to do what I'm trying to do in this issue: It's resolving language based on both the users' language and the entity language.
3. After speaking to people about this issue, I believe there are more issues out there regarding this specific issue. We should try and find them.

If this patch passes the tests, I think a core maintainer should have a look at this. Hope this helps though.