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
- Install Drupal 9.4
composer require drupal/fast_404 drupal/redirect
- Enable redirect and fast404 modules
- Copy web/modules/contrib/fast_404/example.settings.fast404.php file into web/sites/default folder
- Rename the file as settings.fast404.php
- Uncomment
$settings['fast404_path_check'] = TRUE;
in settings.fast404.php to enable path checking. - 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.
- Create a basic page, name its alias as /about-us
- Edit the same page, rename its alias as /about-you
- 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.
Comment | File | Size | Author |
---|---|---|---|
#56 | 2838359-56--fast_404-redirect-multilingual.patch | 1.62 KB | douggreen |
| |||
#55 | 2838359-55-multilingual-compatible-with-2820923-37.patch | 1.51 KB | seanB |
#55 | interdiff-54-55.txt | 981 bytes | seanB |
#54 | 2838359-54-multilingual-compatible-with-2820923-37.patch | 1.34 KB | seanB |
#54 | interdiff-50-54.txt | 885 bytes | seanB |
Comments
Comment #2
paulmckibbenThe attached patch will check for redirects if the redirect module is enabled.
Comment #4
paulmckibbenPutting back in needs review: I don't think automated tests are working for this project.
Comment #6
sonnykt@paulmckibben I have tested your patch. It works with 8.x-1.x. Thanks.
Comment #8
sonnyktI produced the patch that could be applied to 8.x-1.x-dev.
Comment #9
gg4 CreditAttribution: gg4 commentedLooking at #8
Probably should use
trim
to work with paths with trailing slashes.Query order should be adjusted to match others in this file.
Updated patch to consider the above.
Comment #10
gg4 CreditAttribution: gg4 commentedComment #11
jasonawantHi,
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.
Comment #12
gg4 CreditAttribution: gg4 commented@jasonawant do you mean that #10 is good and RTBC or that it is not needed?
Comment #13
jasonawantHi 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.
Comment #14
jasonawantHi,
Here's a re-roll of #10 against current dev..10 could not apply.
Comment #16
jasonawantI omitted a piece, here's another patch file.
Comment #18
gg4 CreditAttribution: gg4 commentedThank @jasonawant, can you also provide an interdiff against #10 or the last passing patch when you get this to go green?
Comment #19
jasonawantHi @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?
Comment #21
gg4 CreditAttribution: gg4 commentedComment #22
gg4 CreditAttribution: gg4 commentedAddressed required parameter $module_handler missing error.
Comment #24
gg4 CreditAttribution: gg4 commentedComment #26
gg4 CreditAttribution: gg4 commentedI think the testbot is having some serious issues. Moving that back to needs review.
Comment #28
gg4 CreditAttribution: gg4 commentedComment #29
gg4 CreditAttribution: gg4 commentedRe-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
Comment #31
gg4 CreditAttribution: gg4 commentedComment #32
gg4 CreditAttribution: gg4 commentedPatch updated to work with tests.
Comment #33
platinum1 CreditAttribution: platinum1 commentedThis seems like an important module from a performance perspective. I am curious why development has stalled?
Comment #34
gg4 CreditAttribution: gg4 commentedComment #35
joseph.olstadWe 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
Comment #36
marfillaster CreditAttribution: marfillaster as a volunteer commentedI 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.
Comment #37
dorficus CreditAttribution: dorficus at Nerdery for Nestle Purina PetCare - United States commentedFor 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.
Comment #38
bbombachiniThis 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.
Comment #39
Kristen PolThanks for the issue, patches, and reviews!
Confirmed the patch in #31 (filename uses "30") still applies cleanly against the
8.x-2.x
branch: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.
Comment #40
Kristen PolIt would be good to have very explicit steps to reproduce so it can be manually tested more easily.
Comment #41
g-brodieiUpdated 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.
Comment #42
douggreen CreditAttribution: douggreen commentedWe need to urldecode that path otherwise the database query is missing some redirects, patch attached
Comment #43
Martijn de WitChange the status for new attached patch, so it triggers tests
Comment #44
Martijn de WitTest seems to be ok. Very small change, see diff. setting it back to RTBC
Comment #45
azinck CreditAttribution: azinck at Forum One commentedThis 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).
Comment #46
azinck CreditAttribution: azinck at Forum One commentedI'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.
Comment #47
azinck CreditAttribution: azinck at Forum One commentedWhoops, got my logic inverted. Here's another try.
Comment #48
Kristen PolComposer require failure so back to needs work.
Comment #49
joshua1234511Reviewed 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.
Comment #50
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedUpdating the patch with a comment in example.settings.fast404.php regarding the setting 'fast404_respect_redirect'
Comment #51
joshua1234511Reviewed 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
Comment #52
Kristen PolThanks @ameymudras and @joshua1234511! This is ready to be merged by a maintainer :)
Comment #53
alexharries CreditAttribution: alexharries commentedComment #54
seanBThe 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.
Comment #55
seanBFound 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 .
Comment #56
douggreen CreditAttribution: douggreen commentedThe latest patch broke our site. Attached is an updated language aware patch.
Comment #57
juanolalla CreditAttribution: juanolalla at Peak Digital, LLC commentedComment #58
joseph.olstadComment #60
joseph.olstadhttps://www.drupal.org/project/fast_404/releases/8.x-3.0-alpha1
Comment #61
Martijn de Wit