Cannot delete file when using language negotiation domains

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

Changing to RTBC after the last comment.

catch → closed merge request !6848

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

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 browser 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 request sanitize function removes the destination URL and thus it ends up in getCancelUrl and subsequently throws the error.

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

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?

smustgrave → credited agentrickard → .

smustgrave → credited alexpott → .

smustgrave → credited Krzysztof Domański → .

smustgrave → credited parisek → .

smustgrave → credited Wim Leers → .

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

The test case testBlockPlacementIndicator in BlockUITest.php has the issue described in 🐛 Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error Closed: duplicate . 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: Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error).

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