Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Allow redirect between two different domains.
Example:
When user enters 'example.site.org' redirect him to 'site.org/example'
Proposed resolution
Use similar functionality as the host redirect module.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#47 | allow_redirect_between-2833457-47.patch | 20.76 KB | Ginovski |
#47 | interdiff-2833457-46-47.txt | 1.95 KB | Ginovski |
#46 | form-domain-red.png | 37.49 KB | Ginovski |
#46 | allow_redirect_between-2833457-46.patch | 20.73 KB | Ginovski |
#46 | interdiff-2833457-44-46.txt | 1.8 KB | Ginovski |
Comments
Comment #2
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedInitial patch, added a form, some configuration for the redirects in install folder, and extended the function
RedirectRequestSubscriber::onKernelRequestCheckRedirect to check for domains redirect.
Comment #5
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedAdded schema
Comment #8
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commented1.Fixed schema
2. Added table in the form
3. Added ajax button for adding another redirect row (need to fix this)
4. Configured submitForm (need to fix this aswell, values from the text fields are not submitted).
Screenshot of the current form:
Comment #11
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedReroll, need to fix schema and ajax.
Comment #12
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedSome form and schema configuration, functioning good now.
Comment #14
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedReroll and added test coverage for the form functionality.
Comment #16
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedComment #19
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedFixed the schema and added dsm.
Comment #21
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedFixed the RedirectRequestSubscriberTest with adding a domain configration.
Comment #23
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedAdding an if to fix the foreach.
Comment #24
Berdirthis needs to go, default is an empty list.
Domain redirectS
this first if is always true as you assign it in the constructor.
this matches on the full path for the domain.
What you usually want to do with something like this is a redirect of anything that uses foo.org/bla to bar.org/bla. So only match on the domain and keep the path when redirecting.
Matching a certain path is probably still useful, especially if we support wildcards. Lets split this up into a separate "Path" field, so you could have a configuration like this:
From: example.org, Path: /blog/*, To: blog.example.org
if it contains either http:// or https:// then we should treat it to only match if you are actually using that protocol, so you need to set a flag in those ifs or check the protocol of the request.
For example, if you have from: http://example.org and to: https://example.org, then you need to redirect to https but if you're on https then it must not match.
That said, I'm not sure we should actually support that. Usually having this kind of configuration in Drupal is very annoying on local/test environments when you don't have https.
So maybe we should validate that it does *not* contain a protocol in the UI and it ourself here for he target domain.
missing docblock.
this is the same as the parent
use $this->config.
you could above just do $form['redirects'][] then you don't need this.
add the primary button flag so this one is highlighted
you could initialize maximum_domains in buildForm() to 1 if not set, then you don't need this special case.
as discussed above
again, $this->config(), that's why getEditableConfigNames() exists.
getValue('redirects')
Comment #25
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedAddressed changes from #24 comment.
Except for 3. (RedirectRequestSubscriberTest fail locally if I don't have this if) - added screenshot
Comment #27
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedFixed tests.
Comment #28
Berdiruse has()
coding style. Also, you can simply look for '://'
not what I said, $this->config()
4 isn't addressed yet I think, splitting up domain and path + wildcard support.
Comment #29
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commented1. Using has() instead of get() for the form_state
2. Fixed the if for the http:// check, now checking for ://
3. Changed Drupal::configFactory()->getEditable('redirect.domain') to $this->config('redirect.domain').
4. Added wildcard support (not sure if this I did this right)
Example for the wildcard usage:
Case 1:
Assuming there is a redirect domain in the configuration (foo.org/bar/* -> bar.foo.org)
When requesting foo.org/bar/post, it redirects to bar.foo.org/post
Case 2:
Assuming there is a redirect domain in the configuration (foo.org/first/* -> foo.org/first/second)
When requesting foo.org/first/third, it redirects to foo.org/first/second/third
Comment #30
Berdirthere is a path matcher service that we might be able to use. Maybe. If not then we'll figure out something. Also, doesn't the break above skip this then?
what we need todo is first get a list of matches for the domain and then apply the path rules.
As discussed, we can't do web tests easily (only for path, not domain).
but we should be able to write unit tests for this easily. Check the existing unit test and then lets define a number of scenarios that we want to support.
the point of this change is that you don't need to do it twice ;)
Comment #31
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commented1. Changed the schema structure of the domain redirects.
2. Changed the form and redirect function accordingly.
3. Added unit test with 1 basic redirect case:
Configuration: 'foo.com/*' => 'bar.com'
Test: Request to foo.com/example, redirects to bar.com/example.
Screenshot of the form:
Comment #32
Berdir* To Domain then isn't just the domain, its' the whole uri. Maybe we can call that Destination instead.
*
I've been thinking about this config and the separation of code from this and normal redirects.
I see 3 options for the config:
1. Existing settings file, then we only have to load one on each request
2. Separate config file, current patch
3. Separate, optional module for this feature.
For 1 and 2, we additionally have 3 options for where this code lives:
A. Same event subscriber, inlined (current patch)
B. A separate event method
C. A completely separate event subscriber (if we pick 3 above, then we need C here as it is a separate module anyway).
In #2704213: "Redirect from non-canonical URLs to the canonical URLs" not working with language code in url, we moved a lot methods to a separate service, as that will possibly eventually move to core.
I think we should at least move the logic to a separate method, that might also make it easier to test.
And I'm actually considering to move it into th same config file but not sure about that yet. Wait with that for now.
still want to get rid of that if condition. And if we go with a separate method, then that should just work.
did you try to use the PathMatcher service? I'd expect that to be a lot more flexible, someone might write something like /foo/*/bar
as in the UI, I'd rename this to destination in config and variables.
Comment #33
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commented1.Changed to_domain -> destination
2. Created new domain redirect request subscriber and test
3. Using path matcher for redirecting with wildcard.
Comment #35
Berdir$config_factory
redirectChecker, $redirect_checker.
requestContext, $request_contet.
this doesn't matter for us, we're not doing inbound path processing.
pathmatcher can do both, so you don't have to do your own check.
getRequestUri includes the query string, pretty sure what we want here is getPathInfo().
I don't think those redirects are ever routed, so we can possibly skip this.
also not sure what the context line does above, we don't seem to use that here?
lets make a separate test for this.
Comment #36
BerdirAlso, still thinking about making this a separate module like redirect_404, now that everything is in separate files. Lets discuss this.
A few more variations on the unit test would also be good, lets have a look at using a data provider for this.
Comment #37
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commented1. Renamed config -> config_factory, checker -> redirect_checker
2. Removed request_context since it wasn't used.
3. Removed comment for the request inbound path processing.
4. Using path matcher for the redirect check.
5. Using pathInfo instead of requestUri
6. Skipped the redirects in setResponse()
7. Made a separate test for the UI
8. Moved the whole domain redirect to a submodule redirect_domain
9. Added services and everything accordingly.
10. Added data provider in the unit tests and another case for simple redirect.
Comment #38
BerdirCan use RedirectChecker::class, same for other class/interface references.
not used here I think? Wondering if we actually should, though, but we can possibly implement that later. (Would be pretty easy to configure an endless redirect).
that's not really how a data provider is meant to work.
I'd keep the configuration static, what the data provider should provide is the incoming URL and the expected destination, including a NULL if it should not redirect. Then we test various different requests against a given configuration.
Those two things can combined into one call.
this snippet is then no longer repeated, instead you provide $request and $destination as arguments to your method. See \Drupal\KernelTests\Core\Cache\CacheCollectorTest::testCacheCollector() for an example.
probably copied, but we should fix the docs still on the new code.
no documentation.
Comment #39
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commented1. Addressed changes from #38 comment.
2. Also added the protocol to the destination, not sure if I did this correct
Comment #40
BerdirThat's asserting the wrong thing, you need to check if you have a response, not that the response_url is null, you know that already.
We have no example yet for..
* correct domain but wrong path
* A domain with multiple paths, e.g. first a fixed path and then a wildcard, you could add a fixed redirect to foo:com, if it matches that, it should use that destination and otherwise the wildcard.
Comment #41
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedEdit: #42 is the correct comment.
Comment #42
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedAdded the two cases and fixed assertion.
There is 1 thing though:
If the user writes first the wildcard in the configuration, then the fixed,
the wildcard will pick up the redirect before the fixed.
So not sure if it should check for the fixed first, and then for the wildcards? (that was in some previous patch with the manual check instead of pathmatcher)
Comment #43
BerdirAs discussed:
* move yml file
* ensure leading / in form
* add help text
Comment #44
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commented1. Moved yml file
2. Enforced a leading '/' in the subpath and added test assertion
3. Added hook_help.
Comment #45
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedAdding the sorting domain redirects as a related issue.
Comment #46
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedAdded help text for the domain redirect.
Screenshot of the form:
Comment #47
Ginovski CreditAttribution: Ginovski at MD Systems GmbH commentedFixed the subpath and added 1 more test assertion.
Comment #49
BerdirYay, finally :)
Committed.
Comment #51
ABaier CreditAttribution: ABaier commentedJust a quick question:
Would wildcards in the middle of a path work? I have to fix some strange patterns of an old site.
Example of a possible sub path:
/node/*/videos
Another thing:
If I had a path with more than one arguments, would one wildcard solve it or do I have to set one for each position?
Example:
/videos/335/some-alias
Sub path:
/videos/* OR /videos/*/*
Nginx should not be a problem, I hope.
Thanks for any hints!
Comment #52
ABaier CreditAttribution: ABaier commentedForget my questions in #51 … everything is working as expected, but I missed out that our server redirects the whole traffic from example.com to www.example.com, so my redirects with example.com in the "from domain" field did not catch. Maybe it would be a good idea to mention this in the example configuration above the form.
Comment #53
arakwar CreditAttribution: arakwar commentedI'm not sure I understood the documentation and this thread properly, but my assumption is that a config of :
FROM : example.com
SUB PATH : /*
DESTINATION : foo.com/en
should redirect http://example.com/product/first to http://foo.com/en/product/first
Am I right ? If so, it doesn't seems to work... Anything I throw at the site on example.com once the configuration is set as noted earlier will just land at http://foo.com/en
I could just set an entry for each redirection, but I have about 7 or 8 domains to redirect, and my marketing team misprinted some URL. It would be nice to be able to redirect entire domain to the new base domain, then have the base domain manage those wrong URL.
Most people would recommend at this point to manage it in the .htaccess file, but with our multisite configuration that would mean more than thousand rules... I need to figure out a way to have Drupal to manage it :( The Redirect module with the domain redirect feature seemed the best way, if it could work as I assumed it.
If it's not the plan, what would be a good procedure ? maybe adding a tag in the Destination URL to let know that we want the subpath to be repeated there ?
Comment #54
bkosborneLooks like that type of redirect is not supported, but there's work being done in #2904056: Option to keep original path on domain redirect to fix that.