Confirmation cancel links are incorrect if installed in a subdirectory

That related issue might be helpful, since having the base path only *sometimes* is exactly the problem.

From the RedirectResponseSubscriber - if the destination has a leading slash we should treat it as a URL that includes the base path already. If it doesn't have a leading slash the code would be correct.

@pwolanin suggested that we assume every link that begins with a leading slash includes the base path.
This will break the test coverage we added in #2501655: ConfirmFormHelper::buildCancelLink() incorrectly handles ?destination URLs with a leading slash

So, this is not really the right approach and I don't see a trivial fix fix here.

In #2418219: Deprecate destination URLs that don't include the base path and in the code in \Drupal\Core\EventSubscriber\RedirectResponseSubscriber::getDestinationAsAbsoluteUrl() we assume if the destination has a leading slash is includes the base path already.

There is currently no way to construct a Url object and tell it to strip the base path.

So, we either need a special case here, or directly construct the markup, or ?

Here's more of a gedanken patch

Yeah I think the gedanken Url::fromUri('generated://this-is-any-previously-generated-url')

Yes, so basically we might need another scheme that tells the unrouted url assember to use the value as if it already includes any necessary pas path/script.

This would just be a couple small tweaks to \Drupal\Core\Utility\UnroutedUrlAssembler::buildLocalUrl()

If this is only case handling this sort of urls I would strip base path right there instead of creating another scheme.
$options['path'] = substr($options['path'], Unicode::strlen($request->getBasePath()));

Remove basepath from the path only if it starts with basepath.

Oh, forgot to import Unicode class.

Hi julia.klimovsky,

If you will try to pass the destination value without drupal sub directory name, It will work properly. No need to patch inside ConfirmFormHelper.php file.

instead of /drupaldemo/node/{nid}/delete?destination=/drupaldemo/admin/content
you have to find a way to pass the destination like below
/drupaldemo/node/{nid}/delete?destination=/admin/content

@bapi_22, I have to disagree :)

While system URLs like ?destination=admin/content indeed work with buildCancelLink, but...

The destination value is generated by the RedirectDestinationInterface and Core decided to change its behavior in #2417645: Change destination query string handling to break dependence on system path to generate destinations WITH base_bath. Long time ago. So, I still can't see better way of fixing the cancel links than strip the base path. This way, we still support both ?destination=/subdir/admin/content and legacy ?destination=admin/content

PS. I will look into failing tests asap...

I've updated the patch and tests so it covers "subdirectory" situation properly

I have tested the patch #26 manually and it seems to be working fine for confirmation pages. Please refer the attached screenshots.

Test case 1:

Created a new content and trying to delete the content confirmation page. Previously it shows page not found. After applying the patch it's redirecting to the content management page.

Testcase 2:

Created new custom block using Block layout and created a new test block content. After that, I'm trying to delete the content and it's redirecting properly to block content management page. Previously, it redirects to page not found.

+++ b/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
@@ -31,6 +32,10 @@ class ConfirmFormHelper {
+        if ($base_path && strpos($options['path'], $base_path) === 0) {
+          $options['path'] = substr($options['path'], Unicode::strlen($base_path));
+        }

could the argument be made that this logic belongs in Url::fromUserInput instead of in ConfirmFormHelper?

It seems to be embedding intimate knowledge of the routing system in this form, when you could easily argue that it would belong better in the Uri::fromUserInput factory method

I'd like to get some input from @pwolanin, @dawehner and @tim.plunkett here as there are threads of an alternate approach being discussed but then the patch takes a right turn and goes with the strpos approach

+1 to adding this logic to Url::fromUserInput! Updated the issue title to make this more generic. We have similar issues in:
#2990664: Media library does not work when Drupal is installed into a sub-directory
#3002805: Media Widget Cannot Select Media - Page not Found/ReWrite Base Conflict
#3042038: Allow UrlGenerator to process action URLs in FormBuilder::buildFormAction()

#2646744: \Drupal\Core\Url does not accept root-relative (file) URLs, making it impossible to let LinkGenerator create root-relative file URL links is also closely related, aka the same but for base: aka file URLs, if I understand this correctly.

Here is a first attempt to fix this in Url::fromUserInput().

This should fix the tests.

+++ b/core/lib/Drupal/Core/Url.php
@@ -206,9 +206,9 @@ public static function fromUserInput($user_input, $options = []) {
     // base path.
-    $base_path = \Drupal::requestStack()->getCurrentRequest()->getBasePath();
-    if ($base_path && strpos($user_input, $base_path) === 0) {
-      $user_input = substr($user_input, mb_strlen($base_path));
+    $current_request = \Drupal::requestStack()->getCurrentRequest();
+    if ($current_request && strpos($user_input, $current_request->getBasePath()) === 0) {
+      $user_input = substr($user_input, mb_strlen($current_request->getBasePath()));
     }
 

I haven't tested it yet, but I think the flaw with this approach is that it is going to do pretty horrible things if the base path matches the drupal internal URL, e.g. imagine the base path is /admin/, so you have /admin/admin/...

I thought about that. Not really sure how to deal with it though. The URL object currently seems to be inconsistent in that UnroutedUrlAssembler::buildLocalUrl() adds the basepath when calling Url::toString(), but if you feed that same URL to Url::fromUserInput(), it is not stripping the base path off again.

This would indeed break for the situation where the subdirectory is /admin, but the Url::fromUserInput() method does not receive the subdirectory when called. On the other hand, it currently breaks for every case we the subdirectory IS passed to Url::fromUserInput(). If anyone has an idea to have it both ways I'm open to suggestions?

I haven't tested it yet, but I think the flaw with this approach is that it is going to do pretty horrible things if the base path matches the drupal internal URL, e.g. imagine the base path is /admin/, so you have /admin/admin/

That is totally true. After discussing this with berdir on slack, I think we agreed that changing this in Url::fromUserInput() is probably not a good idea. There is no reliable way to detect a subdirectory if the subdirectory is also a valid Drupal path. I guess only the calling code would know if a subdirectory is expected or not.

Url::fromUserInput() already mentions that it only accept valid Drupal paths relative to the root directory. We could add a helper function to strip subdirectories from paths though. Created #3044656: Add a helper method to strip subdirectories from URL paths to fix that.

Let's change this issue back to its original purpose. It could be postponed on #3044656: Add a helper method to strip subdirectories from URL paths, or not :)

Any update on this.
Issue still present on Drupal 8.8.1

For Drupal 8.8.x

For Drupal 8.8.x

For Drupal 9.2.x

Unicode::strlen() used in the #46 patch have been removed from version Drupal 9. Use mb_strlen() instead.

The last patch failed to apply on 9.2, so re-rolled it for 9.2

Re-rolled patch for 9.3, Attached interdiff for same. Please review.

Updated

+++ b/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
@@ -31,6 +31,10 @@ public static function buildCancelLink(ConfirmFormInterface $form, Request $requ
+        $base_path = $request->getBasePath();
+        if ($base_path && strpos($options['path'], $base_path) === 0) {
+          $options['path'] = substr($options['path'], mb_strlen($base_path));
+        }
         $url = Url::fromUserInput('/' . ltrim($options['path'], '/'), $options);

I tried patch #52 and found two cases where the cancel link was not what I expected:

1. At /subsite/node/2/delete?destination=/subsite/fi/admin/content the cancel link is missing the language prefix /subsite/admin/content.
   At /subsite/fi/node/2/delete?destination=/subsite/fi/admin/content the language prefix is included.
2. At /subsite/node/2/delete?destination=/subsites/admin/content the cancel link points to /subsite/s/admin/content.

The first case seems to happen also on a site without subdirectory but should the second case be fixed in this issue?

Tagging for bugsmash.

Possible duplicate with a different scenario for steps to reproduce.

#3179708: When Drupal is installed in a sub-path, the cancel link adds the sub-path twice

It's a bug. I don't think it needs "Needs subsystem maintainer review".

-    $data[] = ['baz'];
-    $data[] = ['/baz'];
+    $data[] = ['baz', ''];
+    $data[] = ['/baz', ''];
+    $data[] = ['baz', '/base/path'];
+    $data[] = ['/base/path/baz', '/base/path'];

Think we should still be able to cover when nothing is based in 2nd.

Here is a 10.1.x patch for composer.json. Please do NOT give me commit credit for this patch.

Thank you to everyone above, I just spent 90 minutes trying to figure out this issue and then determined that it had to be a core bug. And sure enough, you all have done my work !
Thank you!

Elevating to framework manager

There is outstanding feedback on the MR, it needs work, not a framework manager review.

I added changes suggested by Mohammed Nassar to the MR 4160.
It would be great to fix this issue, it will be more issues, because many contrib modules moving to Gitlab CI and it runs webserver with the prefix.

I don't understand @smustgrave's suggestion:

Think we should still be able to cover when nothing is based in 2nd.

Can someone please explain?

Is it possible to hide patch files, and branches that not in use?

I think this is ready for review.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

MR 4160 is mergeable, update status back to needs review.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

I have tested MR https://git.drupalcode.org/project/drupal/-/merge_requests/4160
Looks good, can be moved to RTBC.

Hiding the irrelevant MRs and patches... someone needs to fix the 11.x MR as it has merge conflicts.

Rebased MRs.

was going to post this issue for a weekly review but issue summary could use some work before reaching out to maintainers to take a look.

Fix the confirm URL in the node/translation delete confirm form.

Don't see anything in the MR that seems to just be targeting that form but more general Confirmation Form. S

User interface changes usually should contain before/after sceenshots if it's a UI change, probably will need a CR too.

Left some comments on the MR as well.