Destination url query param affects on form translation delete submission

Converted the button into action link in Claro and changed to use property promotion.

Hate to be that guy but can the IS be updated with the proposed solution.

Following the steps though I can confirm the issue and that patch #31 fixes it.

Besides the IS update should it be mentioned why

protected RequestStack $requestStack is protected when the rest aren't?

Added the proposed solution to the IS.

protected RequestStack $requestStack is protected when the rest aren't?

That's because it's using PHP constructor property promotion. 😊

Gotcha!

1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -119,8 +121,10 @@ class ContentTranslationHandler implements ContentTranslationHandlerInterface, E
   +  public function __construct(EntityTypeInterface $entity_type, LanguageManagerInterface $language_manager, ContentTranslationManagerInterface $manager, EntityTypeManagerInterface $entity_type_manager, AccountInterface $current_user, MessengerInterface $messenger, DateFormatterInterface $date_formatter, EntityLastInstalledSchemaRepositoryInterface $entity_last_installed_schema_repository, protected RequestStack $requestStack) {
   

   We need backward compatibility on the $requestStack argument, in the case a subclass calls the parent constructor without this argument.

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -772,23 +780,31 @@ public function entityFormDelete($form, FormStateInterface $form_state) {
   -  public function entityFormDeleteTranslation($form, FormStateInterface $form_state) {
   

   This is a public method so we should leave it in place and deprecate it for removal in 11.0.0.

3. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -772,23 +780,31 @@ public function entityFormDelete($form, FormStateInterface $form_state) {
   +      $options['query']['destination'] = $request->query->get('destination');
   

   As we only need destination here, can/should we use the redirect.destination service, instead of injecting the entire request stack?

Thanks @longwave! This should address all of the feedback from #36. 🤞

Screenshots before and after. This not only fixes an issue, but it's a major UX improvement.

1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -130,6 +134,10 @@ public function __construct(EntityTypeInterface $entity_type, LanguageManagerInt
   +      @trigger_error('Calling ContentTranslationHandler::__construct() without the $redirectDestination argument is deprecated in drupal:10.2.0 and will be required in drupal:11.0.0. See https://www.drupal.org/node/3375487', E_USER_DEPRECATED);
   

   Verified the link points to the right change record 👍🏾

2. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
   @@ -379,7 +379,7 @@ protected function doTestTranslationDeletion() {
   -    $this->submitForm([], 'Delete translation');
   +    $this->clickLink('Delete translation');
   

   Our existing test didn't find the bug here. So should we have a new test that actually adds a redirect destination query fragment, to ensure this works?

3. +++ b/core/themes/claro/claro.theme
   @@ -414,6 +414,10 @@ function claro_form_alter(array &$form, FormStateInterface $form_state, $form_id
   +  if (isset($form['actions']['delete_translation']['#type']) && $form['actions']['delete_translation']['#type'] === 'link' && !empty($build_info['callback_object']) && $build_info['callback_object'] instanceof EntityForm) {
   +    $form['actions']['delete_translation'] = _claro_convert_link_to_action_link($form['actions']['delete_translation'], 'trash', 'default', 'danger');
   +  }
   

   Does this need specific claro tests?

Paired with @penyaskito to add test case for #40.2.

#40.3: We don't usually add test coverage for UI enhancements.

Congrats! You earned the "I can write a test for that in 10 minutes badge!"

RTBC, thanks!

The last submitted patch, 41: 3108102-41.patch, failed testing. View results →

1) Drupal\Tests\workspaces\Functional\PathWorkspacesTest::testPathAliases
Failed asserting that a boolean is not empty.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/modules/workspaces/tests/src/Functional/PathWorkspacesTest.php:109
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

2) Drupal\Tests\workspaces\Functional\PathWorkspacesTest::testPathAliasesUserSwitch
Failed asserting that a boolean is not empty.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/modules/workspaces/tests/src/Functional/PathWorkspacesTest.php:152
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

FAILURES!
Tests: 3, Assertions: 126, Failures: 2.

Looked pretty random to me, lauriii requeued.

#43/#44 this is a new random fail: 🐛 [random test failure] Random failure in PathWorkspacesTest Fixed

The last submitted patch, 41: 3108102-41.patch, failed testing. View results →

Saving issue credits, and back to RTBC after some random fails.

Committed and pushed c61d707ed4 to 11.x. Thanks!

Unfortunately not eligible for backport because of the addition to the ContentTranslationHandler constructor.

Automatically closed - issue fixed for 2 weeks with no activity.

The change records for this issue is the same as the deprecation message. I suggest someone add more explanation and perhaps a code example.

The patch https://git.drupalcode.org/project/drupal/commit/c61d707ed4.patch applies cleanly against 10.1

    https://git.drupalcode.org/project/drupal/commit/c61d707ed4.patch (3108102 - Destination url query param affects on form translation delete submission)
patch '-p2' --no-backup-if-mismatch -d 'web/core' < '/tmp/652fa878032e9.patch'
patching file modules/content_translation/src/ContentTranslationHandler.php

patching file modules/content_translation/tests/src/Functional/ContentTranslationRevisionTranslationDeletionTest.php

patching file modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php

patching file themes/claro/claro.theme

Hunk #1 succeeded at 419 (offset 5 lines).

See everyone over in 🐛 Operations links on the translations overview page do not link to the correct route Needs work .

This should be closed since it has been committed https://git.drupalcode.org/project/drupal/-/commit/c61d707ed4 to 10.2+

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.)

At this point I guess better to change this to Fixed. We can always update the change records, but even short they are enough as contain the direct method replacement.

Automatically closed - issue fixed for 2 weeks with no activity.