Domain-based language negotiation should retain "destination" URL query argument

Support for delete-form is required. I've tested this and it works fine.

Changing to RTBC after the last comment.

It's not clear to me whether this will fix the original bug report, would be good to add some test coverage if the lack of this is causing user-facing fatal errors. Could also use an issue summary update.

- Issue summary updated. error screenshot added.
- New test case added
- Screenshot post fix added

The MR has failed at test case testUnmanagedGitIgnoreWhenGitNotAvailable which seems unrelated to the code fix. what should be done?

Appears to have test failures

Issue summary appears to be updated with solution in #13 so removing that tag

Ran test only feature and got

1) Drupal\Tests\file\Functional\FileListingTest::testFileDeleteWithMultipleDomain
Behat\Mink\Exception\ResponseTextException: The text "Are you sure you want to delete the file image-test.png?" was not found anywhere in the text of the current page.
/builds/issue/drupal-3424701/vendor/behat/mink/src/WebAssert.php:907
/builds/issue/drupal-3424701/vendor/behat/mink/src/WebAssert.php:293
/builds/issue/drupal-3424701/core/tests/Drupal/Tests/WebAssert.php:956
/builds/issue/drupal-3424701/core/modules/file/tests/src/Functional/FileListingTest.php:331
/builds/issue/drupal-3424701/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 3, Assertions: 187, Errors: 1.

So removing that tag

Seems additional condition for deletion seems to be there.

This seems like it would come from lib/Drupal/Core/Entity/EntityListBuilder.php but that passes in 'delete-form' to ::toUrl() so it can't be that.

I think we need a backtrace here with what is calling ::toUrl() with NULL, and fix it there, or at least understand what's going wrong a bit more. Seems weird to default to deleting just because there's no canonical or edit link. Also the exception message would need to be updated which is not done in the MR.

Is it this in the actual delete form?

 /**
   * {@inheritdoc}
   */
  public function getCancelUrl() {
    $entity = $this->getEntity();
    if ($entity->hasLinkTemplate('collection')) {
      // If available, return the collection URL.
      return $entity->toUrl('collection');
    }
    else {
      // Otherwise fall back to the default link template.
      return $entity->toUrl();
    }
  }

If so, I think the problem is file module missing a 'collection' link for the cancel link to point to, it shouldn't point to the delete page, that's what we're on.

- IS updated
- fix as per #20 done

Sorry I don't think that fix is right either because views is an optional module and therefore the route may or may not exist.

Couple of questions:

1. What happens if we remove the exception from EntityBase::toUrl() and fall back to <front> for the route there - I don't think that's the correct fix but I would like to see what happens.

2. It would be useful to manually test this in 10.1 or 10.0.x from before the regression was introduced, and see what the cancel button actually did on this form, I have a feeling the button was broken but it rendered and no-one ever noticed.

If the views module does not exist, then this screen is not accessible and therefore it's not possible to reach the error.

I tried to add the collection route conditionally but the REST tests fail.

1. If the exception is commented out, it tries to create URL with entity.file.collection and ends up throwing an exception. If that is commented out too and URL to is created, then the dialog pops up. Click on cancel, it remains on the same page.
2. The problem occurs only when the language module is enabled and multiple languages are configured. If it's a single language site then the functionality works fine. The cancel stays on the same page, which would be the expected behavior. If multiple languages are configured then the delete confirmation popup doesn't open, it ends up with exception.

If the original fix of adding 'delete-form' is enabled then the delete confirmation pops-up and clicking on cancel, it remains on the same page.

Proposed solution shouldn't point to a comment, for ease of review please update issue summary with actual proposed solution.

Find saying "Go to this link" the same as when people put "see patch"

@smustgrave the correct solution needs to be confirmed by @catch. once done can update the same

putting it back to NR for @catch attention.

If the views module does not exist, then this screen is not accessible and therefore it's not possible to reach the error.

I think it would still be possible to browse directly to the URL, and definitely possible to set up an alternative view at a different path to do the same thing (or just change the path on the default view).

I think someone should do this:

It would be useful to manually test this in 10.1 or 10.0.x from before the regression was introduced, and see what the cancel button actually did on this form, I have a feeling the button was broken but it rendered and no-one ever noticed.

Once we know what the original behaviour was and whether it was broken already in a different way, it will help inform what the solution here should be.

Also, does this really require language negotiation to reproduce?

Results of manual testing

10.0.8 - View does not provide the option to delete.

10.1.0
- Without language module enabled, Delete confirmation is shown and clicking Cancel, closes the dialog
- With language module enabled, Error as per screenshot error-on-delete.png

After further analysis, I've found that the URL generated for delete without language module or language with url detection is

https://d10n.ddev.site/file/1/delete?destination=/admin/content/files&_wrapper_format=drupal_modal

and with language module with domain detection is

https://d10n.ddev.site/file/1/delete?destination=https%3A//d10n.ddev.site/admin/content/files&_wrapper_format=drupal_modal

the issue is that in the second case the destination is a fully qualified url and in the first case it's relative path. Since we have fully qualified URL, the RequestSanitizer removes the destination URL and thus it ends up in getCancelUrl and subsequently throws the error.

- IS updated
- Fix updated
- also fixes issues #2980527: Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error

removing "needs manual testing" tag. done in #28

Comment in MR.

without correcting the assertion, the test will fail. do confirm if you want this to be removed. will the merge happen with test failing?

So going to pivot since the todo and test update is needed here. Lets expand test coverage to make sure it includes scenarios in #2980527: Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error. Which I'll close and move over credit for.

The test case testBlockPlacementIndicator in BlockUITest.php has the issue described in #2980527: Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error. This has been fixed. I don't think we need additional test case for this.

Works for me (I had the issue described in #2980527).

Fine if we don't expand test coverage. Will let committers decide if needed though.

Thanks for the working on improving multi-lingual sites!

An issue summary update removed a large block of testing and explanation that I found useful to help understand this issue. Maybe that should be restored?

I read the comments and MR at #2980527: Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error the fix there is very different from this. That MR was created by a core committer and another core committer worked on this issue. I think we need to get confirmation of which solution is preferred. Apologies if I missed it. For now, I will tag for framework manager review. The other issue also has test and it would help to know if that should be kept or not.

There are also comments in the MR to resolve.

An issue summary update removed a large block of testing and explanation that I found useful to help understand this issue. Maybe that should be restored?

restored original proposed solution

solves the problem. All threads were resolved. The large block of testing was restored.

Looks like this needs work based on @quietone's review.

This same problem exists in Drupal 10 but wasn't sure how to document it?

Error thrown when trying to delete file entities at /admin/content/files

Turn off URL detection that uses domain at /admin/config/regional/language/detection, clear cache and file entities can be deleted error free.

Updated proposed solution to mention that the todo around https://www.drupal.org/project/drupal/issues/2980527 is also being removed.

Resolved the rest of the threads and believe all feedback has been addressed.

Only request I would make is if the MR can be rebased since it's 800+ commits back.

Rebased

Thanks believe all feedback has been addressed

Feels like the issue that was closed as a duplicate better (more generically) describes what this is about, while this issue title is just about one specific aspect of that? Maybe we should update it?

From #53, Trying for a better title.

One comment on the MR - I think the new logic needs an explanation as to why it's there.

From what I understand the current implementation add the domain to the destination uri so that the user after the POST get redirected to the domain is currently using.

For instance if I have a site in english and italian, each with its own domain, and I'm using the english one, if I try to delete the italian translation of a node the delete link is built with the italian domain, and then destination=english_domain/current_path is added to bring back the user to the current page with the current language. Otherwise he'll switch to the language of the page he has deleted.

Current implementaion seems to create problem:
1. delete file does not works
2. if i try to delete a translation of a node I currently get this error:

Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\TrustedRedirectResponse for it.

(on Drupal 10.5)

The proposed patch is not perfect (to user will switch to the language of the translation that need to edit / delete) but it's still better that the current implementation.

P.S. is a backport to 10.x planned?

A version compatible with Drupal 11.3.2 has been created.

Didn't https://www.drupal.org/project/drupal/issues/3424720 already closed and comitted to 11.1.x fix this problem?