If drupal is installed in a subdirectory accessing directly admin/modules/uninstall/confirm causes redirection to localhost/admin/modules/uninstall instead of localhost/drupal8/admin/modules/uninstall

This because \Drupal\system\Form\ModulesUninstallConfirmForm line 113 uses relative path instead of route name for generate the redirect url:

return new RedirectResponse('/admin/modules/uninstall');

URLs should be generated from route names. It should be: 

return $this->redirect('system.modules_uninstall');
CommentFileSizeAuthor
#5 uninstall_redirect-2112895-5.patch1.39 KBwillzyx
#4 uninstall_redirect-2112895-4.patch675 byteswillzyx
#3 2112895-3--uninstall_redirect.patch687 bytesdrunken monkey
#1 2112895-1.patch735 bytesswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

swentel CreditAttribution: swentel commented
FileSize
2112895-1.patch735 bytes

Are you sure, because that is in the confirm buildForm method, not in the submit. None the less, that path is wrong indeed.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Active » Needs work

I think URLs should now be generated from route names.

A quick grep for use of this class shows code like this:

  public function redirect($route_name, array $route_parameters = array(), $status = 302) {
    $url = $this->urlGenerator()->generate($route_name, $route_parameters, TRUE);
    return new RedirectResponse($url, $status);
  }
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2112895-3--uninstall_redirect.patch687 bytes

Ran into the same problem just now, the attached patch seems to be the right way to fix it.

@swentel: I don't understand it either, the code doesn't look like it, but this is definitely where the problem is coming from. And without the patch, uninstalling also didn't work (at least for me, though that might have been a fluke).

willzyx’s picture

willzyx CreditAttribution: willzyx commented
FileSize
uninstall_redirect-2112895-4.patch675 bytes

I think it should be

// Prevent this page from showing when the module list is empty.
if (empty($this->modules)) {
  return $this->redirect('system.modules_uninstall');
}

as is done in ModulesListConfirmForm::buildForm()

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Issue summary: View changes
FileSize
uninstall_redirect-2112895-5.patch1.39 KB

Added test and update IS

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Title: Uninstalling module doesn't work for drupal installed in subdirectory » Wrong redirection in admin/modules/uninstall/confirm if drupal is installed in a subdirectory
Issue summary: View changes

Update IS

joachim’s picture

joachim CreditAttribution: joachim commented 
+++ b/core/modules/system/src/Tests/Module/UninstallTest.php
@@ -106,5 +106,10 @@ function testUninstallPage() {
+
+    // Make sure confirmation page is accessible only during uninstall process.
+    $this->drupalGet('admin/modules/uninstall/confirm');
+    $this->assertUrl('admin/modules/uninstall');
+    $this->assertTitle(t('Uninstall') . ' | Drupal');

That doesn't seem related to me.

willzyx’s picture

willzyx CreditAttribution: willzyx commented

That doesn't seem related to me.

why do you think is not related? we should test that, by accessing directly to the confirm page, you will be redirected properly

joachim’s picture

joachim CreditAttribution: joachim commented

Ah ok fair enough. That's a property of confirm forms I wasn't aware of.

aburrows’s picture

aburrows CreditAttribution: aburrows as a volunteer commented
Status: Needs review » Reviewed & tested by the community

This can be RTBC now #5 works, just tested by creating sub directory and moving site into that directory. Downloaded token 8.x and installed then uninstalled and returns user to modules page

The last submitted patch, 1: 2112895-1.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 487de77 and pushed to 8.0.x. Thanks!

  • alexpott committed 487de77 on 8.0.x 
    Issue #2112895 by willzyx, swentel, drunken monkey: Wrong redirection in...

Status: Fixed » Closed (fixed)

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