Rendered HTML Output doesnt respect activeLanguage completely

Can you specify in a bit more detail what parts of the HTML output use the wrong language?
Also, if you say yourself this might be a general issue, did you also raise it in the Core issue queue? Would of course be great if this could be fixed there.
Otherwise: What do you suggest? Switching out the language negotiator temporarily in our plugin? Might work, but sounds risky. Don’t have any other ideas, though, so it seems our best shot. Can you provide a patch?

I've come across the same issue with D7. There we've the same problem - passing in the language to entity_view() won't necessarily have effect on lots of things e.g. the rendering of links (url() still relying on $GLOBALS['language_url'])

Switching out the language negotiator temporarily in our plugin? Might work, but sounds risky.

For D7 this seems the only way too. Even thought we can't guarantee that we won't have weird effects due static caches which are generally not language aware and hence could make trouble. However, the output is definitely better with the adjustment.

Unfortunately can only provide a D7 patch for now.

Timely addition. Thanks @vierlex!

I'm using it in a dev environment and it seems to be working for me on 8.7.1 with search api 8.13.

I'll let you know if I run into any issues.

Thanks!

Taking a look at those failing tests.

I've found a way to swap out the language negotiator without having to add language as a dependency. This resolves most of the test failures, but there are still some failures remaining when I run the tests locally. I'm out of time today, so posting my work in progress for now.

I think this should fix the last failing tests.

As always, I haven't figured out yet why they won't apply when running through the Testbot :(

Glad you could make it work! You’ve probably realized it by now, but the key is to have something that will be accepted by git apply. Using git diff is the simplest way to make sure of that.
Your initial patch missed the usual a/*/b/* prefixes for file paths, so git apply removed the leading src/ part of the paths – which of course breaks them. Not sure what was wrong with your later patches – maybe not using the current dev version?

In any case, thanks a lot for posting your work!
Some of the code was not quite perfect in my opinion, and it also ignored the Drupal coding standards partially. Please see/test/review my attached revision!
Two more things:

1. This still needs a regression test for verifying that a) there was a problem and b) the patch fixes it. This will make sure we’re actually solving a problem, and that we don’t break this again in the future. Probably, it can just live in the existing RenderedItemTest class – install the language module, set the current language to English and then try to render a French translation of a node with comments (or term references, or something like that).
2. As I’m still not convinced this won’t break things for some sites, and as it might not actually be needed on them, should we maybe make this configurable on the processor? Or would you say, as long as the Language module is present it’s pretty sure people will want this?

@ das-peter/#7: Thanks for posting, looks very good! While tests are not necessary for D7, my other comment still applies there, too: Do you think we should maybe make this configurable?

“Needs work” status for tests.

@ das-peter/#7: Thanks for posting, looks very good! While tests are not necessary for D7, my other comment still applies there, too: Do you think we should maybe make this configurable?

...should we maybe make this configurable on the processor? Or would you say, as long as the Language module is present it’s pretty sure people will want this?

Thanks for the feedback :)
I think it's fair to be cautious regarding existing sites. Making it configurable sounds like a good way to go. I've update the D7 patch to make the behavior configurable and it also sets a "sensible" default. This means for existing configurations the language switch will be disabled unless explicitly enabled but for new configurations it will be enabled by default upon creation.

Thanks for the updated patch, Peter! Looks very good now.
Just expanded the form field description to hopefully cause less confusion to everyone who didn’t follow this issue and moved the code around a bit to look cleaner to me.
Otherwise, RTBC, if you agree? Thanks again, in any case!

Back to NW for D8 tests.

Otherwise, RTBC, if you agree? Thanks again, in any case!

Sure! Thanks for the review and the pro-active feedback, much appreciated!!

Great, thanks for the feedback!
Committed the D7 patch, then. Thanks again!

D8 still needs such a final push. Any takers?

Thanks for your work, the #20 patch solves my problem of the default language is used while rendering twig files, resulting in wrong tranlslations when the t-filter is used, urls linking to wrong language or untranslated dates. My use case is storing the rendered search results in solr and fetch them from there. (D8.6, Search-API 1.13, Search API Solr Search 3.1)

Edit: I have to correct myself, the t-Filter is still using the default language. A workaround is to pass the String via preprocess or use the trans-function, and pass the now correct available langcode from preprocessor:

{% set link_title %}
  {% trans with {langcode: current_langcode} %}
    Details anzeigen
  {% endtrans %}
{% endset %}

Patch 20 for D8 works well for us when indexing via the UI, however on the command line it fails to make sure that the rendered html output is in the right language.
I was able to track the cause down.
Somewhere after the static negotiator (introduced by the patch) is put into the language manager, the language_negotiator service gets initialised by the service container (it seems to be somewhere in the dependency tree of content moderation's EntityOperations class). And the language_negotiator service initialises the language manager, setting itself again as the negotiator and thus replacing our static negotiator again.

core/modules/language/language.services.yml:

  language_negotiator:
    class: Drupal\language\LanguageNegotiator
    arguments: ['@language_manager', '@plugin.manager.language_negotiation_method', '@config.factory', '@settings', '@request_stack']
    calls:
      - [initLanguageManager]

core/modules/language/src/LanguageNegotiator.php,

/**
   * Initializes the injected language manager with the negotiator.
   *
   * This should be called right after instantiating the negotiator to make it
   * available to the language manager without introducing a circular
   * dependency.
   */
  public function initLanguageManager() {
    $this->languageManager->setNegotiator($this);
  }

To solve this, I've put one line of code before setting up the static negotiator, to initialize the language_negotiatior service. Updated patch attached.

After installing the patch, I'm experiencing the issue described here for some of my nodes: https://www.drupal.org/project/drupal/issues/2869347

We've been using the patch in #29 but has noticed a bug where translations are not rendered in the correct language. After a lengthy debug session I found that the string_translation service is initialized with the default language so we need to update that as well.

I created an issue fork, rerolled the patch from #29 and then implement my changes.

https://git.drupalcode.org/issue/search_api-3035977/-/compare/8.x-1.x......

URL to patch:
https://git.drupalcode.org/project/search_api/-/merge_requests/2.patch

I don’t think there is a need for patches when we already have a fork? Anyways, also no harm in creating an MR before this is completely done. That way, there is also constant automated testing. (Even though something seems to have gone wrong in the last test …)

Anyways, thanks a lot for working on this!
I looked into it, and your change does make sense, even though the comment explains it a bit poorly, and you forgot to reset the language afterwards. I hopefully fixed both of those things now, plus made a few cosmetic changes.

Also, apart from all that, as RenderedItem is a plugin, it uses dependency injection, so we shouldn’t get those services via \Drupal::… but through DI.
So, in addition to the automated tests, this would also need to be fixed before this can be merged.

Created #3190024: Problem with test dependencies when testing issue forks for the test failures.

Good catch!

Here's the original patch from #35. It looks like the MR is updated dynamically, and the MR-patch can no longer be applied cleanly with composer-patches.

Last patch fixes the issue in the OP completely for me using rendered item indexing!

Same, patch from #39 solves the issue — Thanks!
We had the problem, that the UI translations were rendered in the language negotiated by Drupal --> so always German.

Reroll the MR and make a patch for it.

I'm just doing some light work related to #3190024: Problem with test dependencies when testing issue forks.

@fjgarlin: Thanks a lot for your work on that!
However, please do such work in #3396064: (Try to) fix the GitLab CI in the future, it’s quite unrelated to this ticket.

@drunken-monkey 100% you’re right, apologies. I was meant to undo the changes but totally forgot. My bad.

I arrived at this issue following the related ones and just wanted to prove something (which I did) in the related issues.

I’ll undo my commits Monday first thing (away from computer right now).

My changes are now reverted.

#3380219-2: Rendering is not changing the current content and string translation language is a duplicate that contains a patch with a different approach, not needing a new service class (which I’d count as a big plus). Please try it out and see whether this doesn’t also resolve this problem for you.

In either case, though, we still need a regression test for this.