Fatal Error on empty Redirect Path

Patch attached.

What did the last patch change? They appear to be identical.

Doesn't look like that patch changed anything. If it did - an interdiff would be a really helpful to show your work. Hiding that patch for now.

Even if the redirect is setup, it can use tokens, and the related tokens may be empty for any reason; e.g. using a field which is empty.
I would suggest to alternative or additionally check validity during redirection handling.

In specific, the mentioned exception is thrown from Drupal\Core\Url::fromUserInput(), and I would suggest to handle that exception at the call time and do nothing, i.e. do not redirect.
I avoided adding a log entry since this could potentially fill logs.

Adding a patch doing that to avoid the php fatal error.

This:

Even if the redirect is setup, it can use tokens, and the related tokens may be empty for any reason; e.g. using a field which is empty.

is a good point and should be accounted for. However, I don't think just taking no action is the right way to go. If someone set a redirect they were likely expecting the user to not hit that page. I think throwing an access denied error would be preferable to potentially allowing a user to access a page they weren't meant to.

I'm attaching a patch that throws an access denied instead. I've also brought my changes (to make the field required in the first place) back in - I think those probably weren't left out intentionally?

@Dylan Donkersgoed, thanks for the feedback and the new patch.

About #8, the idea was to focus on the main problem, without caring on UI; so it was provided as an alternative approach.
I agree that we should care on UI too, so thanks for trying to integrate the change!

Triggering access denied may not be the right thing to do in all cases, but it sounds like a good default.
Debugging the problem may be harder in that case, so I would suggest to at least log watchdog message about it.

Even better, I would suggest to instead make a new interaction point, e.g. a new module hook, to allow changing the behavior from a custom module if needed; and provide the default of logging a message about the problem and triggering access denied.

Moving to NW for at least logging a message, but even better if a new hook is also introduced.

I would agree that making an assumption of either access denied or access granted when the redirect path is empty might not work in all cases. We could expose the fallback option in the UI instead:

Fallback behavior:
- Page not found
- Display the page
- Access denied

Here's a first stab at making the fallback configurable. Update hook provided to supply new default config value.

The configurable fallback worked for me. I tried all three options and they appear to work as intended.

After using this for a bit, I realized that some AJAX operations were failing on my site, especially related to IEF and entity browsers.

After some investigation, I found that the AJAX call was returning this error:

call_user_func_array() expects parameter 1 to be a valid callback, 
non-static method Drupal\rabbit_hole\Plugin\RabbitHoleBehaviorPlugin\PageRedirect::validateRedirectSettingsForm() should not be called statically in /Users/martin/Sites/knowewell/web/core/lib/Drupal/Core/Form/FormValidator.php on line 282

Declaring as static the validateRedirectSettingsForm method added by this page fixed the errors, and restored the AJAX functionality. I'll try to roll a new patch soon.

This patch implements a fix suggest by mandclu aboutn validation static method
and a tag support on validation it's related by issue #3061163: Allow using in redirect settings

Thank you all! Can we have an interdiff from #12 to #15?
Both look good to me so far. #15 even better, but I wonder why \Drupal::pathValidator()->isValid($redirect) from the referenced issue #3061163: Allow using <front> in redirect settings is not used?

Hello, it's not a particular reason but if this makes sense we can make an approach combine both of patches.

but I wonder why \Drupal::pathValidator()->isValid($redirect) from the referenced issue #3061163: Allow using in redirect settings is not used?

I think, that the validation of the url-field should not be part of this patch, because the '' route is handled by #3061163.

EDIT: #3060274 modifies the validator as well, so we will get some conflicts when both patches are applied.

Furthermore there should a default-default-fallback action. As long the default-fallback action is not configurated, the exception is still triggered. The savest action would be imho to deny the access.

I implemented the suggestions, I made in #19 and adjusted the codestyle errors injected from the previous patches in the files I touched.

Remarks to my patch

+++ b/src/Plugin/RabbitHoleBehaviorPlugin/PageRedirect.php
@@ -167,7 +174,21 @@ class PageRedirect extends RabbitHoleBehaviorPluginBase implements ContainerFact
-      $target = Url::fromUserInput($target)->toString();
...
+        $target = Url::fromUserInput($target)->toString();
+      }
+      catch (\InvalidArgumentException $exception) {
+        switch ($fallback_action) {
+          case 'page_not_found':
+            throw new NotFoundHttpException();
+
+          case 'display_page':
+            return $current_response;
+
+          default:
+            throw new AccessDeniedHttpException();
+        }
+      }

Each switch statement should have a default section, as the 'access denied' is default when entering the config-form. I used it here as well
Personally I don't like a case statement without a break, but I did not touch it, and there is no recommandation in the code-style :-(

I've seen it too late: When updating an existing installation with this patch, the new field `redirect_fallback_action` is not installed, so a update_hook is missing:

This patch adds the new field to existing entities.

I have been looking at this issue.

First, a minor point. I confirm that the patch in #4 is identical to the one in #2.

Second, most of the patches on this issue set the "Redirect path" field to required and also add a custom validator for that field. I guess the idea is that the field is set to required only when Behavior is set to "Page redirect"; this requires JavaScript; and we want to check the field even when JavaScript is disabled.

I tested this by removing the custom validator and then submitting the form with the "Redirect path" field empty. With JavaScript enabled, the form is not submitted, as expected, since the field is required. I did not test with JavaScript disabled, but I did test with the current version of the 8.x-1.x branch, and I get a validation error from FormManglerService::validateFormRedirect(). That code was added in #2922902: Relative Redirects paths should be Absolute paths, after the initial patch on this issue. At this point, we can remove the custom validator from this patch.

The attached patch removes the redundant validator. I am still working on this issue, so I am assigning it to myself.

Third, the patch in #25 adds an update function. I tested it, and it works as expected. However, I have also been working on the related issue #3060274: Tokens as redirect. I have git branches for that issue and this one. Git merges the two branches cleanly, but then the update function fails. So we will have to be careful when combining these two patches.

Fourth and last point: the patches on this issue combine two different approaches, and I think this issue should really be split into two. One approach is to make the field required. The other approach is to provide a fallback for when it is empty. (The common use case is when, after #3060274, the field is a token but the token evaluates to empty.) If we have the fallback, do we really need to make the field required?

Here is a possible use case, but perhaps there are better ones. I want a different redirect for every node of some content type. So I want to configure the content type to redirect, but leave the redirect path empty. Then I can enter a different redirect path on every node. (A better solution, after #3060274, is to use a token.)

Because of that last point, I am adding the "Needs issue summary update" tag.

The attached patch does two things:

1. Remove an unused use statement.
2. Add an update function to update the rabbit_hole.behavior_settings config items.

I should have done (1) as part of the patch in #26.

As for (2), I noticed that the settings form for an existing content type (already configured to use the Redirect option) did not have any fallback selected. The new update function takes care of that. It also handles the default settings. (There are two defaults: one for per-bundle settings and the other for per-entity settings.)

I considered combining this update function with the one from #25. That is an option, but I decided that there may be sites that have already applied that patch, and it would be convenient for those sites if I created a separate update function.

I do not have time to continue working on this issue (unless I run into problems with further testing) but I do have some suggestions.

1. +++ b/src/FormManglerService.php
   @@ -320,6 +320,8 @@ class FormManglerService {
              ?: '',
              'redirect_code' => $form_state->getValue('rh_redirect_response')
              ?: BehaviorSettings::REDIRECT_NOT_APPLICABLE,
   +          'redirect_fallback_action' => $form_state->getvalue('rh_redirect_fallback_action')
   +            ?: 'access_denied',
   

   Why not use the default value from config? I think that would be rabbit_hole.behavior_settings.default_bundle.

2. +++ b/src/Plugin/RabbitHoleBehaviorPlugin/PageRedirect.php
   @@ -21,6 +21,8 @@ use Drupal\rabbit_hole\BehaviorSettingsManagerInterface;
   ...
   +use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
   +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
   @@ -167,7 +173,21 @@ class PageRedirect extends RabbitHoleBehaviorPluginBase implements ContainerFact
   ...
   +      catch (\InvalidArgumentException $exception) {
   +        switch ($fallback_action) {
   +          case 'page_not_found':
   +            throw new NotFoundHttpException();
   +
   +          case 'display_page':
   +            return $current_response;
   +
   +          default:
   +            throw new AccessDeniedHttpException();
   +        }
   +      }
   

   Instead of throwing these exceptions directly, I think it makes more sense to list all the available Rabbit Hole behavior plugins (or maybe just those that do not require configuration) and invoke the configured one here. That is, we should be calling the performAction() method from the access_denied or page_not_found plugin instead of throwing exceptions here.

3. +++ b/src/Plugin/RabbitHoleBehaviorPlugin/PageRedirect.php
   @@ -35,6 +37,7 @@ class PageRedirect extends RabbitHoleBehaviorPluginBase implements ContainerFact
   ...
   +  const RABBIT_HOLE_PAGE_REDIRECT_DEFAULT_FALLBACK_ACTION = 'access_denied';
   @@ -238,6 +260,9 @@ class PageRedirect extends RabbitHoleBehaviorPluginBase implements ContainerFact
   ...
   +      $redirect_fallback_action = isset($entity->rh_redirect_fallback_action->value)
   +        ? $entity->rh_redirect_fallback_action->value
   +        : self::RABBIT_HOLE_PAGE_REDIRECT_DEFAULT_FALLBACK_ACTION;
   

   Same idea as (1). Let's get rid of this constant and use the configured default instead.

In #26, I mentioned a problem with the update function after merging the patches from this issue and #3060274: Tokens as redirect. Probably that was a mistake on my part, or maybe I somehow fixed it as part of the patch in #27.

I am attaching two new patches here: one is a combined patch, with both #27 from this issue and #23 from #3060274. The second is a version of the patch from #27 that can be applied after applying #23 from #3060274. The only differences between the second patch and the one in #27 are metadata lines and one line of context.

The patch from #3060274-23: Tokens as redirect was committed to the dev branch, so the "after" patch in #28 is now the active patch on this issue. Unfortunately, the patch needs a reroll after #3115059: Can't enter redirect urls longer than 128 characters, so here is an updated version. The only differences between this patch and the "after" patch are context lines.

Do we care whether #states comes before or after #maxlength?

Thanks for the patch @benjifisher. Added a few more lines to ensure the config value does not get wiped out on export.

diff --git a/src/Entity/BehaviorSettings.php b/src/Entity/BehaviorSettings.php
index 76d15fa..d774020 100644
--- a/src/Entity/BehaviorSettings.php
+++ b/src/Entity/BehaviorSettings.php
@@ -21,7 +21,8 @@ use Drupal\rabbit_hole\Exception\InvalidBehaviorSettingException;
* "action" = "action",
* "allow_override" = "allow_override",
* "redirect" = "redirect",
- * "redirect_code" = "redirect_code"
+ * "redirect_code" = "redirect_code",
+ * "redirect_fallback_action" = "redirect_fallback_action"
* },
* config_export = {
* "id",
@@ -29,7 +30,8 @@ use Drupal\rabbit_hole\Exception\InvalidBehaviorSettingException;
* "action",
* "allow_override",
* "redirect",
- * "redirect_code"
+ * "redirect_code",
+ * "redirect_fallback_action"
* },
* links = {}
* )

I think it's time for me to take a look at this issue, so here we go :)

The last patches look good at a glance, but I'd like to apply some changes to make the code cleaner and easier to extend. I'm attaching is a re-roll against the latest dev version (we already have a bunch of new changes there) with some refactoring (I hope I didn't break it yet).
(This is a draft version, I'll try to get back to it this week)

I agree with @benjifisher that we should execute Rabbit Hole plugins instead of duplicating the job of the action inside the PageRedirect action; That's why the fallback action login was moved into BehaviorInvoker.php class.

Here is a new version based on #30.
A list of applied changes:

• Moved fallback action execution from \Drupal\rabbit_hole\Plugin\RabbitHoleBehaviorPlugin/PageRedirect to \Drupal\rabbit_hole\BehaviorInvoker;
• Replaced hardcoded list of fallback actions with the list of all available Rabbit Hole actions (except Page Redirect) and set default action to "bundle_default";
• Made some refactoring and added tests;

#27 1-3 are valid points, but it should be fixed in several places. I created a follow-up issue for that: #3180520: Use default values from default bundle configuration instead of hardcoded constants.

Removed some outdated asserts.

I think it's good enough to be committed.
If anyone has any suggestions - feel free to open a new issue or re-open this one.

Thanks, @all!