Issue

When the redirect module is enabled, and fast404 path checking is also enabled, fast404 will fail paths that are configured as redirect sources.
Based on 7.x functionality, fast404 should not fail paths that are configured as source paths in the redirect module.

Steps to Reproduce

  1. Install Drupal 9.4
  2. composer require drupal/fast_404 drupal/redirect
  3. Enable redirect and fast404 modules
  4. Copy web/modules/contrib/fast_404/example.settings.fast404.php file into web/sites/default folder
  5. Rename the file as settings.fast404.php
  6. Uncomment
    $settings['fast404_path_check'] = TRUE;
    in settings.fast404.php to enable path checking.
  7. Add
    if (file_exists($app_root . '/' . $site_path . '/settings.fast404.php')) {
      include $app_root . '/' . $site_path . '/settings.fast404.php';
    }

    at the end of settings.php to allow fast404 module to take effect.

  8. Create a basic page, name its alias as /about-us
  9. Edit the same page, rename its alias as /about-you
  10. An redirect entity should automatically generate when new alias is saved on the same node. Check /admin/config/search/redirect to confirm.

What to expect

Without the patch

Navigating to /about-us will not redirect but go into fast404 page.

With the patch

Navigating to /about-us shall redirect normally to /about-you.

CommentFileSizeAuthor
#56 2838359-56--fast_404-redirect-multilingual.patch1.62 KBdouggreen
#55 2838359-55-multilingual-compatible-with-2820923-37.patch1.51 KBseanB
#55 interdiff-54-55.txt981 bytesseanB
#54 2838359-54-multilingual-compatible-with-2820923-37.patch1.34 KBseanB
#54 interdiff-50-54.txt885 bytesseanB
#50 fast_404-honor_redirects-2838359-50.patch1.19 KBameymudras
#47 interdiff-46-47.txt523 bytesazinck
#47 fast_404-honor_redirects-2838359-47.patch726 bytesazinck
#46 interdiff-42-46.txt5.06 KBazinck
#46 fast_404-honor_redirects-2838359-46.patch727 bytesazinck
#42 interdiff--fast_404-redirect-2838359-42.txt109 bytesdouggreen
#42 fast_404-redirect-2838359-42.patch6.02 KBdouggreen
#41 2838359-after-anime.gif117.99 KBg-brodiei
#41 2838359-41-after.jpg81.19 KBg-brodiei
#41 2838359-41-before.jpg40.19 KBg-brodiei
#31 interdiff--2838359--28-30.txt1.46 KBgg4
#31 fast_404-redirect-2838359-30.patch6.01 KBgg4
#29 fast_404-redirect-2838359-28.patch4.36 KBgg4
#22 fast_404-redirect-2838359-22.patch4.35 KBgg4
#22 interdiff--2838359--19-22.txt643 bytesgg4
#19 fast_404-redirect-2838359-19.patch3.86 KBjasonawant
#16 fast_404-redirect-2838359-15.patch3.87 KBjasonawant
#14 fast_404-redirect-2838359-14.patch3.28 KBjasonawant
#10 interdiff--2838359--9-10.txt3.43 KBgg4
#10 fast_404-redirect-2838359-10.patch3.88 KBgg4
#9 fast_404-redirect-2838359-9.patch729 bytesgg4
#8 fast_404-redirect-2838359-8.patch730 bytessonnykt
#2 2838359-redirect-2.patch707 bytespaulmckibben
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

paulmckibben created an issue. See original summary.

paulmckibben’s picture

The attached patch will check for redirects if the redirect module is enabled.

Status: Needs review » Needs work

The last submitted patch, 2: 2838359-redirect-2.patch, failed testing.

paulmckibben’s picture

Status: Needs work » Needs review

Putting back in needs review: I don't think automated tests are working for this project.

Status: Needs review » Needs work

The last submitted patch, 2: 2838359-redirect-2.patch, failed testing. View results

sonnykt’s picture

Version: 8.x-1.0-alpha2 » 8.x-1.x-dev
Status: Needs work » Reviewed & tested by the community

@paulmckibben I have tested your patch. It works with 8.x-1.x. Thanks.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2838359-redirect-2.patch, failed testing. View results

sonnykt’s picture

Status: Needs work » Needs review
FileSize
730 bytes

I produced the patch that could be applied to 8.x-1.x-dev.

gg4’s picture

Looking at #8

  1. +++ b/src/Fast404.php
    @@ -122,6 +122,16 @@ class Fast404 {
    +    if (\Drupal::moduleHandler()->moduleExists('redirect')) {
    +      $path_noslash = ltrim($path, '/');
    ...
    +        return;
    

    Probably should use trim to work with paths with trailing slashes.

  2. +++ b/src/Fast404.php
    @@ -122,6 +122,16 @@ class Fast404 {
    +      $sql= "SELECT rid FROM {redirect} WHERE :path = redirect_source__path";
    

    Query order should be adjusted to match others in this file.

Updated patch to consider the above.

gg4’s picture

jasonawant’s picture

Hi,

I think this is working as expected. I'm adding a related issue with the redirect module. The Redirect modules allows redirects with trailing slashes, however Redirect does not match, it strips leading and trailing slashes from path before matching.

gg4’s picture

@jasonawant do you mean that #10 is good and RTBC or that it is not needed?

jasonawant’s picture

Hi bonus,

Sorry, my previous response is confusing.

I think the patch in #10 is working as expected against the current Redirect 1.2 release. However, b/c Redirect allows redirects with trailing slashes to be saved to database, the trim logic in this patch may not find a Redirect match when querying the redirect table.

Redirect's RedirectRepository::findMatchingRedirect() uses a hash value, prepared with Redirect::generateHash(), to query the redirect table.

This would be the preferred lookup method, but I'm not sure fast404 can do this in a pre-bootstrap logic without including more of Redirect's files or duplicating much of the logic.

jasonawant’s picture

Hi,

Here's a re-roll of #10 against current dev..10 could not apply.

Status: Needs review » Needs work

The last submitted patch, 14: fast_404-redirect-2838359-14.patch, failed testing. View results

jasonawant’s picture

Status: Needs work » Needs review
FileSize
3.87 KB

I omitted a piece, here's another patch file.

Status: Needs review » Needs work

The last submitted patch, 16: fast_404-redirect-2838359-15.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gg4’s picture

Thank @jasonawant, can you also provide an interdiff against #10 or the last passing patch when you get this to go green?

jasonawant’s picture

Hi @bonus,

Here's a new patch using the short array syntax that fixes the code standard issue. interdiff failed, I suspect b/c #10 can not apply cleanly anymore. Do you have any suggestions about how else to produce the interdiff?

Status: Needs review » Needs work

The last submitted patch, 19: fast_404-redirect-2838359-19.patch, failed testing. View results

gg4’s picture

gg4’s picture

Addressed required parameter $module_handler missing error.

Status: Needs review » Needs work

The last submitted patch, 22: fast_404-redirect-2838359-22.patch, failed testing. View results

gg4’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: fast_404-redirect-2838359-22.patch, failed testing. View results

gg4’s picture

Status: Needs work » Needs review

I think the testbot is having some serious issues. Moving that back to needs review.

Status: Needs review » Needs work

The last submitted patch, 22: fast_404-redirect-2838359-22.patch, failed testing. View results

gg4’s picture

Status: Needs work » Needs review
gg4’s picture

Re-roll. Current patches do not consider that fast404.inc creates a new Fast404 object and will require and addition controller argument, which will require Drupal to be bootstrapped. fast404.inc has other issues in general, though. See #2961512: Using the fast404.inc approach throws errors

Status: Needs review » Needs work

The last submitted patch, 29: fast_404-redirect-2838359-28.patch, failed testing. View results

gg4’s picture

Status: Needs work » Needs review

Patch updated to work with tests.

platinum1’s picture

This seems like an important module from a performance perspective. I am curious why development has stalled?

gg4’s picture

Title: Incompatibility with Redirect module » Fast404 path checking is incompatibility with Redirect module
Version: 8.x-1.x-dev » 8.x-2.x-dev
joseph.olstad’s picture

We did not need this patch, but yet are using redirect module

see our configuration details here:
https://www.drupal.org/project/fast_404/issues/2961512#comment-14040732

marfillaster’s picture

I would prefer a configurable kernel.request event priority.

Reason why fast_404 happens before redirect is due to the former having higher priority.
https://git.drupalcode.org/project/fast_404/-/blob/8.x-2.x/src/EventSubs... vs https://git.drupalcode.org/project/redirect/-/blob/8.x-1.x/src/EventSubs....

This can be overridden using compiler pass but it would be nice to just have the priority as a $setting.

dorficus’s picture

For anyone attempting to patch to the 8.x-2.x dev branch at the latest commit as of June 17th, 2021, the patch from comment 30 applies and allows for d9 compatibility based on initial testing.

bbombachini’s picture

This won't apply for me as I'm also using the patch from https://www.drupal.org/project/fast_404/issues/2820923. Unfortunately we decided to disable the module until both issues get fixed.
Tried what joseph.olstad said on #35 but that also didn't work for us.

Kristen Pol’s picture

Thanks for the issue, patches, and reviews!

Confirmed the patch in #31 (filename uses "30") still applies cleanly against the 8.x-2.x branch:

KristenBackupMBP:fast_404 admin$ patch -p1 < fast_404-redirect-2838359-30.patch 
patching file fast404.services.yml
patching file src/EventSubscriber/Fast404EventSubscriber.php
patching file src/Fast404.php
patching file tests/src/Unit/Fast404EventSubscriberTest.php
Hunk #1 succeeded at 63 (offset 1 line).
patching file tests/src/Unit/Fast404Test.php

There is a mixture of feedback from "I didn't need this patch" to "this patch didn't apply" to "this patch did apply" :)

I did review the code and didn't see any obvious formatting/styling issues.

Kristen Pol’s picture

It would be good to have very explicit steps to reproduce so it can be manually tested more easily.

g-brodiei’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs steps to reproduce, -Needs manual testing
FileSize
40.19 KB
81.19 KB
117.99 KB

Updated IS with a Steps to Reproduce process.

Manually tested against core 9.4 and fast_404 2.x-dev, #31 applies cleanly and works with redirect for new alias to properly redirect instead of displaying a fast404 page.

Removing tag NSTR and NMT.

before patch
after patch applies

patch apply animation

douggreen’s picture

We need to urldecode that path otherwise the database query is missing some redirects, patch attached

Martijn de Wit’s picture

Status: Reviewed & tested by the community » Needs review

Change the status for new attached patch, so it triggers tests

Martijn de Wit’s picture

Status: Needs review » Reviewed & tested by the community

Test seems to be ok. Very small change, see diff. setting it back to RTBC

azinck’s picture

This approach isn't compatible with invoking fast404_preboot in settings.php. This patch now requires that we provide a ModuleHandler object to instantiate a Fast404 object but we don't have the ability to do that in settings.php (at least, I'm not aware of a way to do so) since we're not far enough along in the bootstrap process.

The only thing the ModuleHandler object is used for here is to check if the Redirect module is enabled on the site. What would people think about just checking for the existence of the redirect table and using that as a proxy for determining whether to do the redirect checking?

On a different topic, @joseph.olstad in 35:
Using fast404_not_found_exception instead of fast404_path_check "works" in as much as it preserves compatibility with the Redirect module, but it fails in that you then lose much of the benefits of the fast 404 module (time to deliver a 404 for a standard page with no file extension returns to approximately the same speed as it is without this module in my testing).

azinck’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
727 bytes
5.06 KB

I'm actually wondering about using another setting to control whether or not the redirect table is checked. This would avoid having to run another query on every page request just to see if the redirect table exists.

Patch attached.

If people like this we'll need to update the docs and tests a bit.

Kristen Pol’s picture

Status: Needs review » Needs work

Composer require failure so back to needs work.

joshua1234511’s picture

Reviewed and tested the patch provided in #47
- The Module is not yet compatible with D10, the test composer was failing due to that.
- Works with 9.4 PHP 8.0 & MySQL 5.7, D9.4 6 pass

Additional The patch required the
$settings['fast404_respect_redirect'] = TRUE;
to be set in the settings.fast404.php file

It would be best to include comment and variable in the example.settings.fast404.php file

Results
Without the patch
- Navigating to /about-us will not redirect but go into fast404 page.

With the patch
$settings['fast404_respect_redirect'] = TRUE;
- Navigating to /about-us shall redirect normally to /about-you.
$settings['fast404_respect_redirect'] = FALSE;
- Navigating to /about-us will not redirect but go into fast404 page.

ameymudras’s picture

Status: Needs work » Needs review
FileSize
1.19 KB

Updating the patch with a comment in example.settings.fast404.php regarding the setting 'fast404_respect_redirect'

joshua1234511’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the patch provided in #50
The comment reference is added.
Re tested the patch on v9.4 ( PHP 8.0 & MySQL 5.7, D9.4 6 pass )
Marking as RTBC

Kristen Pol’s picture

Thanks @ameymudras and @joshua1234511! This is ready to be merged by a maintainer :)

alexharries’s picture

Title: Fast404 path checking is incompatibility with Redirect module » Fast404 path checking is incompatible with Redirect module
seanB’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
FileSize
885 bytes
1.34 KB

The patch in #50 does not play nice with #2820923-37: Fast404 path checking is incompatibility with multilingual paths . Attached is a patch to check the language for redirects as well for people that need this.

This obviously needs the patch from #2820923: Fast404 path checking is incompatibility with multilingual paths to work.

Marking as NW since this could still use a test.

seanB’s picture

Found one extra issue in #54 for languages without a path prefix. As for #54. this patch also needs #2820923-37: Fast404 path checking is incompatibility with multilingual paths .

douggreen’s picture

The latest patch broke our site. Attached is an updated language aware patch.

juanolalla’s picture

Status: Needs work » Needs review
joseph.olstad’s picture

Version: 8.x-2.x-dev » 8.x-3.x-dev

joseph.olstad’s picture

Martijn de Wit’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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