Problem/Motivation

It'd be nice if users chould have a redirect regex such as: ^/foo/(\d+)$ and have the redirect reference replace it with /baz/$1/target

Such that /foo/122 redirects to /baz/122/target.

Proposed resolution

Add appropriate preg_replace() call to ensure it updates regex references.

Add some kind of flag into the regex: prefix, signifying that the regex needs to match against the query strings too. e.g. :regex:qs:

So that regex such as this is possible:

regex:qs:page(?:\?)(?:.*?(\&))?(param)=([^&]+)

to match against page?extraparam=foo&param=123

Remaining tasks

Provide MR.

Issue fork redirect_regex-3577966

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

codebymikey created an issue. See original summary.

codebymikey opened merge request !2

codebymikey’s picture

codebymikey commented
Issue summary: View changes
Status: Active » Needs review
codebymikey’s picture

codebymikey commented

One limitation at the moment with the module is with regards to how the ? character is handled.

I would ideally like to be able to use named regexes capturing groups like:

^/foo/(?'id'\d+) -> /baz/$id

But due to how the RedirectForm currently works, the presence of the ? will cause it to think it's a query string and mess the original URL up.

Simple non-named regex groups will still work for now, just that more complex regexes will no longer be supported.

I think to combat this, the "default" and "edit" form handlers will need to be overridden/altered so it switches Drupal\redirect\Form\RedirectForm for a different version.

Ideally, the new regex functionality should be done in a way that doesn't hack the redirect_source.path to work.

Given, how easy the implementation seems to be, it would be nice if this was actually supported in the original redirect module as a first class citizen. e.g.
1. with a new add regex redirects permission to restrict who can create/update regex redirects.
2. introduce a new boolean field to flag if a URL is a regex based one or not.

codebymikey’s picture

codebymikey commented
Assigned: Unassigned » codebymikey

I've got something sandboxed locally with the ? fixes and permission restriction, just assigning to myself so no-one else works on it in the meantime.

It'll keep the regex: approach, without introducing a new boolean base entity field (as that's out of scope for the issue)

Will push up once the codes cleaned up.

o'briat’s picture

o'briat
Primary language French
Location Nantes
commented

Seems fine to me.
You could remove the Escape forward slashes as \/ from _redirect_regex_add_help_text() in the module file, since you use braces ({ }) as delimiter

o'briat’s picture

o'briat
Primary language French
Location Nantes
commented

Here is a MR with add the support of querystrings.

codebymikey’s picture

codebymikey commented
Title: Provide the ability to replace regex references » Provide the ability to replace regex references and support query strings
Issue summary: View changes

You could remove the Escape forward slashes as \/ from _redirect_regex_add_help_text() in the module file, since you use braces ({ }) as delimiter

That's all done already in my local version. Was just finalizing an integration test with the #2879648: Redirects from aliased paths aren't triggered feature since the module already had code trying to support it.

Here is a MR with add the support of querystrings.

I don't think the commit addresses the original issue I mentioned with regards to using ? directly within a regex.

I've already addressed the original encoding issue locally, and will push the changes up soon.

I do think adding support for matching against query strings is also useful, so will try and incorporate some of that into my version too.

I also like the addition of the views field filter, as I believe that's the probably the preferred way to integrate the module without messing with the site's existing configuration - that way the redirect_regex module can be easily uninstalled if needed without any lingering side-effects or broken configurations.

codebymikey’s picture

codebymikey commented
Assigned: codebymikey » Unassigned
Related issues: +#3578020: Introduce permission to restrict who can add/update regex permissions

Pushed the updated code up including functional and kernel tests addressing all the issues found during my tests.

Overview of current functionality:
1. Replace regex references.
2. Support query strings.
3. Provide permissions so only certain users can work with the regexes - #3578020: Introduce permission to restrict who can add/update regex permissions
4. Avoid manually specifying the regex: or regex:qs prefix in the UI, there's now checkboxes to provide the functionality.

Longer goals:
1. Migrate away from the regex: prefix, and use fields (low priority)
2. Try and optimize the repository so that the regex match happens on the database level. As it can be potentially very inefficient when the site has a lot of regex redirects.

Just need to see what the maintainer thinks. But I think I'm done with the issue for the time being as it does everything I'd like it to do.

But feel free to provide any feedback!

o'briat’s picture

o'briat
Primary language French
Location Nantes
commented
Status: Needs review » Needs work

The query part is not working as expected:
Redirection: formation\?([^=]*).*$ + querystring + /formations/$1/
For: /formation?foo
Expected: /formations/foo/
Observed: /formations/foo/?foo= the querystring is appended to destination url

For: /formation?foo=bar
Expected: /formations/foo
Observed: /formations/foo/?foo=bar the querystring is appended to destination url

I finally remember what I did to get rid of the querystring : uncheck the "redirect passthrough querystring" settings of the redirect module (global) or patch its file: /src/EventSubscriber/RedirectRequestSubscriber.php#L166

--- src/EventSubscriber/RedirectRequestSubscriber.php
+++ src/EventSubscriber/RedirectRequestSubscriber.php
@@ -163,7 +163,7 @@

       // Handle internal path.
       $url = $redirect->getRedirectUrl();
-      if ($this->config->get('passthrough_querystring')) {
+      if ($this->config->get('passthrough_querystring') && !str_starts_with($redirect->getSourcePathWithQuery(), '/regex:qs:')) {
         $url->setOption('query', (array) $url->getOption('query') + $request_query);
       }
       $headers = [

I believe there is a more elegant way to override \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber::onKernelRequestCheckRedirect()

codebymikey’s picture

codebymikey commented
Status: Needs work » Needs review

Initially planned to leave this for the specific sites to implement, but I've addressed the feedback from #10 as it does seem reasonable enough

Extra query strings added by the passthrough_querystring setting will be removed if the regex explicitly supports query strings. Then those requiring even more special handling of the query string can do so in their own hook_redirect_response_alter() hooks.

o'briat’s picture

o'briat
Primary language French
Location Nantes
commented
Status: Needs review » Reviewed & tested by the community

Perfect: thanks!

baikho’s picture

baikho commented

Hey both, thanks a lot for the contributions here. I'm a bit busy lately, so I won't be able to follow up on this closely. If tests coverage is good I'm happy for this to go in.

  • codebymikey committed 9afbdc25 on 1.x 
    feat: #3577966 Provide the ability to replace regex references
codebymikey’s picture

codebymikey commented
Status: Reviewed & tested by the community » Fixed

Thanks for testing @obriat! Committed.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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