Get RSS feed title & comments in correct language

Seeing the same thing. It is because row/Rss->render() calls entity->link() and entity->label, without designating any language code. This default to defaultLangcode,which was the language code selected when creating the node. Not the language code provided in the URL (or whatever you are using to designate the URL on the views rss page.

this initial pacth is working for me.

That makes more sense. The initial test failure didn't seem at all related to the changes here.

1. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
   @@ -163,4 +167,32 @@ public function render($row) {
   +    return 'node';
   

   I don't see any way around this. Any other suggestions? It just feels icky to do it this way.

2. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
   @@ -163,4 +167,32 @@ public function render($row) {
   +    return \Drupal::languageManager();
   

   Let's inject this, rather than use \Drupal

I'm also seeing a similar pattern in Comment and Aggregator. Do we need to fix those or open follow-ups?

for #6.2

interdiff-3-8

1. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
   @@ -40,6 +44,8 @@ class Rss extends RssPluginBase {
   +  protected $language_manager;
   
   @@ -53,9 +59,20 @@ class Rss extends RssPluginBase {
   +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
   ...
   +      $container->get('entity.manager'),
   

   Missing comments on these new variables.

2. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
   @@ -53,9 +59,20 @@ class Rss extends RssPluginBase {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityManagerInterface $entity_manager, LanguageManagerInterface $language_manager) {
   

   Let's assign $this->entityType in the constructor.

3. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
   @@ -163,4 +182,32 @@ public function render($row) {
   +    return $this->entity_Type;
   ...
   +    return $this->language_manager;
   

   Let's not mix snake_case and camelCase in the same class. Use camelCase, since that is was is already in use.

for #13

+++ b/core/modules/node/src/Plugin/views/row/Rss.php
@@ -53,9 +65,21 @@ class Rss extends RssPluginBase {
+    $this->nodeStorage = $entity_manager->getStorage($this->entity_Type);

rename variable to $this->entityType

Rerolled the patch.

This fix makes sense. Some quick review comments:

1. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
   @@ -42,6 +46,16 @@ class Rss extends RssPluginBase {
   +  /**
   +   * Set the entity type
   +   */
   +  protected $entityType;
   

   We already have protected $entityTypeId; for this

2. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
   @@ -52,10 +66,24 @@ class Rss extends RssPluginBase {
   +    $this->languageManager = $languageManager;
   ...
   +      $container->get('language_manager')
   

   Like heddn said in #7, wouldn't we run into similar problems for other entities? So wouldn't this be better added to the base class?

And most importantly, this still needs test

Applied the patch #19 but the language of the title was still in English.
I found that the reason was that the current language was not selected correctly. It should be like

$render_langcode = $this->languageManager->getCurrentLanguage()->getId();

Here's an interdiff.

Adding a hasTranslation validation to prevent errors when the listed content isn't translated for the current language.

that needs tests and #20 makes sense about base class

Thanks for the patch @piggito . It is still needed.

I am testing patch 24.

Patch 24 works to bring the title into the correct language in my view
and also brings the other fields into the correct language in my view

However I have a date field and that one stays in the default locale and default language even when viewing the rss in another language.
curiously.

Bumping this to 8.8.x

ah sorry this is a bug, it should be in the 8.7.x branch.

Re-roll for 8.8, seems to also apply against 8.7.

Missed a hunk in the re-roll.

to get the correct link for other language in an rss feed I also needed another patch:
#2877994-88: Entity Links fields does not have translation support

We're still using this patch with 8.7.11

I didn't understand why we used EntityTranslationRenderTrait in the first place, if we weren't really using it, so I took it into use in query() like they do in EntityRow, and then calling getEntityTranslation() in render().

Also, I moved most of the changes to RssPluginBase as suggested in #20 and #25. In order not to replicate the base class constructor changes in Rss.php, I removed the constructor and replaced the functionality with create().

Still needs tests.

#36 applied and works well with 8.8.5. Fixed a problem of not getting translated title and link.

Nitpicks. Needs work mostly for adding tests

1. --- a/core/modules/node/src/Plugin/views/row/Rss.php
   +++ b/core/modules/node/src/Plugin/views/row/Rss.php
   
   @@ -43,22 +42,12 @@ class Rss extends RssPluginBase {
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, EntityDisplayRepositoryInterface $entity_display_repository = NULL) {
   ...
   +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
   

   If we use create then either nodeStorage needs to be public or we need a setter for it, otherwise this isn't settable in Unit tests. So not sure about this, I would just stick with using __construct(). The pattern of using create always feels more like a contrib solution to me.

2. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
   @@ -106,6 +95,10 @@ public function render($row) {
   +    $node = $this->getEntityTranslation($node, $row);
   +
   +
   +
   

   Extra newlines are not needed

3. +++ b/core/modules/views/src/Plugin/views/row/RssPluginBase.php
   @@ -47,8 +65,12 @@
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, EntityDisplayRepositoryInterface $entity_display_repository = NULL, LanguageManagerInterface $language_manager, EntityRepositoryInterface $entity_repository) {
   

   We need a BC layer for the new added arguments (see something like \Drupal\views\Form\ViewsExposedForm::__construct in de 8.9.x branch)

Addressing #39 here. Also, rerolled the patch against 9.0.x-dev.

1. Ok, let's just use __construct() and add the needed properties to both Rss.php and RssPluginBase.php, right?
2. Oops, fixed.
3. Tried to do this. What about the versions, do we need to keep this until D10? Do we need a change record?

Not sure I'm allowed to set this to RTBC, but I can confirm (a) it does work and (b) patch #40 addresses all the remaining issue mentioned so far.

I agree with that! We tested the patch on v9.1.3 and it applies and works fine.

Rerolled to core version 9.2.0

I tested this on Drupal 9.5, standard install, using the steps in the IS. I reproduced the problem and then applied the patch and the problem was fixed. Moving to 9.5 since that is what I used for testing.

This still needs a test

- Adding test cases and test only patch.

Tested the patch #49. Thank you for the clear testing steps in the description.
Confirmed I saw the issue without the patch
Confirmed the issue was resolved with the patch

Tested the patch #49. Issue is solved after applying the patch. +1 to RTBC.

The issue summary is incomplete. There is no proposed resolution so how can a reviewer know that the desired fix has been made. Tagging for an Issue summary update.

Changes to the patch were requested for in #39 and there has not been a review of the patch since then, nor has the new test been reviewed.

Based on the above, setting to NW.

Updated the summary proposed resolution.

Since #39, several people have tested the patches. #49 also has been verified. So I propose we set this back to RTBC, so this issue can be closed finally.

Re-rolled #49 by removing the first code block which is completely unrelated and got removed from core by #3162827: Do not instantiate entity storages in constructors of services that do not always need them - this should not be brought back and isn't required for this issue either. I guess this got in accidentally.

If the tests pass, this should be re-marked as RTBC

Seems to have some failures.

Created MR with patch from #58.
Let's see if the tests fail or not.

MR with patch from #58 and fixed remaining issues. Please review.

Added patch and interdiff.

Left comments on MR.

Fixed remarks.

Feedback appears to be addressed.

Thanks for fixing this issue.

@kriboogh, can you comment in the issue the reasoning for the changes you make? Sometime to commit message is enough to provide a clue, but here the message is always the same. It does help reviewer to know what it being done and why.

I left comments in the MR that need attention. This is adding deprecations so there should be a change record and the URL for that change record is to be in the deprecation message. And 10.4 is a maintenance minor so I think this will be deprecated in 11.1.

I've tested the MR and it fixes the issue. Once MR comments are addressed it can go into RTBC.

rebased the MR to bring it up to date with 11.x

Patch for Drupal 11.3

Rebased the open MR to bring it up to date with main

Also fixed some code analysis flags that were highlighted in the pipeline - happy to be guided if they are slightly out of scope or not.

I've spent some time on this today and think I've addressed the outstanding issues:

• In #7 @heddn raised the point that the same pattern/bug exists in comment and aggregator. No subsequent comments addressed that, so I've taken a look and fixed the comment bug here. Aggregator is no longer part of core so we don't have to deal with it here.
• Drafted a change record at https://www.drupal.org/node/3618490
• Addressed the deprecation comments on the MR by updating the versions to 12 and 13 respectively; the wording now matches the core policy and links to the newly created change record
• Fixed testFeedTranslation() which was passing but not actually testing anything useful because it was 404ing and not getting detected.
• Removed the unrelated formatting changes to buildOptionsForm_summary_options() noted in a previous code review
• Removed the disused .phpstan-baseline.php entries that were causing the phpstan pipeline job to fail
• Reverted the unrelated calculateDependencies() try/catch back to the plain load() call (unrelated scope creep flagged in previous code review)

Setting to Needs Review.

Can deprecations be updated for 12.1 and removed/fixed in 13 please.