Destination url query param affects on form translation delete submission

Set it to needs review so this gets a few more views. The patch seems to work.

I saw that drupal 9 also has this problem

My bad. Tested with the wrong version in #8

Did a quick re-roll of the last patch for drupal 9.1

Should've done a reroll for drupal 9.2 ... ah well, here it is :)

It looks like I made a mistake when re-rolling the patch. Here's a working on for 9.2.x

#12 rerolled to 9.3

Patch #12 doesn't need to reroll as it's getting applied cleanly on Drupal 9.3.x.

I'm not able to apply patch #16 on a Drupal 9.3.9
Thought it was because of other patches applied to Drupal Core, but I've been testing to apply only this patch, and it's not applicable, even alone.

A re-rolled patch against the latest 9.3.x.

Still valid on Drupal 10.1.x, standard install.

Re rolling for 10.1.0, couldn't generate an interdiff

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fix as per #24
thanks

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!

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: #3375584: [random test failure] Random failure in PathWorkspacesTest

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.

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 #2451693: Operations links on the translations overview page do not link to the correct route.

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.

The updates to the CR did not happen and now that 10.2 is no longer supported, I am removing the tat.