InvalidArgumentException: Invalid translation language specified.

Did not review but seems there are test failures from last patch.

Tagging for issue summary update for the proposed solution.

Used the redirect approach from #2752345: Visiting translation adding page for existing translation throws an InvalidArgumentException → and updated the issue summary.

The last submitted patch, 83: 2137815-83.patch, failed testing. View results →

Rookie mistake on previous patch. Also the permissions needed a bit of tweaking. For some reason the redirect (302) statuscode was not possible to catch with $this->assertSession()->statusCodeEquals so used a bit different approach.

Fix for the typo.

The last submitted patch, 87: 2137815-87.patch, failed testing. View results →

Adjusting the tests to pass also on CI.

The last submitted patch, 89: 2137815-89.patch, failed testing. View results →

Rescoped the test for content_translation where the change is.

The last submitted patch, 91: 2137815-91.patch, failed testing. View results →

Maybe these where left off from #3232673: \Drupal\Core\Entity\EntityInterface::label() can return a NULL → .

🤪

The last submitted patch, 94: 2137815-94.patch, failed testing. View results →

Random test failures.

Go to content admin and use the filters to find the node I want to translate. Click its Translation operation link.
Add a French translation and save it. This takes me back to the content admin page.
Wait, I forgot to change something! But I can't see my node any more as it's not in the first page of results. I know, I'll just press Back and that'll get me back to the form I was just at, right?

Confirmed the issue following these steps.
Applying patch #94 resolved the issue for me.

Issue summary matches the solution where I'm redirected to translation overview
Patch also appears to have test coverage.

RTBC+1

The last submitted patch, 94: 2137815-94.patch, failed testing. View results →

Random test failure.

I'm triaging RTBC issues → . I read the IS and then read the patch. I did not read all the comments or review or test the patch.

Thanks everyone for working on this old bug!

I found some things that should be fixed before this is ready for commit.

1. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
   @@ -362,6 +362,12 @@ public function add(LanguageInterface $source, LanguageInterface $target, RouteM
   +    // If the entity already has the target translation,
   +    // redirect to the translation overview page.
   

   This is not wrapped correctly. It should be to 80 columns.

2. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
   @@ -120,6 +120,23 @@ protected function doTestBasicTranslation() {
   +    // Assert that once a translation exists, the recurring calls to route is
   +    // redirected to the translations overview page.
   

   I think we can remove the part about 'calls to route'. How about, "Asserts that once a translation exists the user is redirected to the translations overview page.".

3. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
   @@ -120,6 +120,23 @@ protected function doTestBasicTranslation() {
   +      $this->assertStringContainsString('/entity_test_mul_changed/manage/1/translations', $this->getUrl(), 'Correct path after redirect');
   ...
   +      $this->assertStringContainsString('/entity_test_mul_changed/manage/1/translations', $this->getUrl(), 'Correct path after redirect');
   

   Messages are no longer used in assertion, unless needed for clarification as in a for loop.

Also, this should also have a fail test. Therefore setting to NW. This is close!

The attached patch should handle the issues found on #100. Also included a patch that shows the failing.
Also tried to trim the summary of the issue to first phrase on the issue summary.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Re-roll & Convert Patch #101 into MR. Please have a look.

#101 applies cleanly, shows the test failures, and appears to have address.

The MR is a reroll of the test-only patch

Hiding patches

The MR shows fails https://git.drupalcode.org/issue/drupal-2137815/-/pipelines/56688/test_r... for content translation tests, so that feels like it might be a genuine issue

@larowlan as mentioned on #106, "The MR is a reroll of the test-only patch". The patch still applies and passes the tests.

#101 has 1800 fails

Can we move to an MR now for faster testing

Tests on https://git.drupalcode.org/project/drupal/-/merge_requests/5591 now pass so setting the status back to RTBC.

Added some review comments. The most important of which is:

Is this the correct behaviour? The original request for this behaviour mentioned at least adding a message telling the user what had happened. But I don't understand the rationale for going to the translation overview page instead of the edit page for the translation which is surely closer to the user intended.

@hswong3i there was already an existing MR please rebase that vs opening new ones. If doing different approach please note the difference