Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow all query parameters #37

Open
dtspma opened this issue Oct 15, 2024 · 6 comments
Open

Allow all query parameters #37

dtspma opened this issue Oct 15, 2024 · 6 comments

Comments

@dtspma
Copy link

dtspma commented Oct 15, 2024

Hello Tim

For different Ad Services I need to configure the extension to forward all query parameters. I can't list them all in forwardRedirectParameters. Is there an option for this?

Thank you for help
Pascal

@lochmueller
Copy link
Owner

Hey @dtspma ,
currently you have to add all parameters and there is no other option. But you can handle this also in the Ad services. The a is (i think so) in one specific language (and in a specific region), so the ad target could be directly the right language? This would avoid a separate redirect for the user, you do not have to configure all the parameters and the Ad service will love this, because the Ad target is a 20x and not a 30x code anymore.
Regards,
Tim

@dtspma
Copy link
Author

dtspma commented Oct 15, 2024

Hmm, thank you for your quick feedback.

Unfortunately a 307 temporary redirect occures when I request https://www.helvetictours.ch/?utm_source=test&utm_medium=test2 although the language is not changed to French. Did I configure something wrong? As soon as I add utm_source and utm_medium to the list the redirect does not happen anymore. But I don't know all the query parameters of all ad services or they may change once in a while.

@lochmueller
Copy link
Owner

Do you try to debug in the Event process https://github.com/lochmueller/language_detection/blob/main/Classes/Handler/LanguageDetectionHandler.php#L25 to check which event cause the problem in your case? Do you call the page with active backend session and "BackendUserCheck" will avoid the redirect? https://github.com/lochmueller/language_detection?tab=readme-ov-file#checklanguagedetectionevent

@dtspma
Copy link
Author

dtspma commented Oct 17, 2024

Hi Tim

I tried to debug it with a requested /?test=test which results in a 307 redirect to /

It seems to me that if the request contains a query param which is not listed in forwardRedirectParameters the targetUri created by the DefaultResponse https://github.com/lochmueller/language_detection/blob/main/Classes/Response/DefaultResponse.php#L23 does not have the query param because $target is created by $language->getBase() and therefore $targetQuery https://github.com/lochmueller/language_detection/blob/main/Classes/Response/DefaultResponse.php#L44 is empty.

Since the targetUri (without query param) is not the same as the event request uri (with query param) the response is set.
https://github.com/lochmueller/language_detection/blob/main/Classes/Response/DefaultResponse.php#L33

Since the response is set and not null a redirect occurs. https://github.com/lochmueller/language_detection/blob/main/Classes/Handler/LanguageDetectionHandler.php#L53

We have these unecessary redirects in TYPO3 v11 and v12 instances.

@lochmueller
Copy link
Owner

Hey @dtspma ,
thanks for your debugging. So I think this check is wrong https://github.com/lochmueller/language_detection/blob/main/Classes/Response/DefaultResponse.php#L24 https://github.com/lochmueller/language_detection/blob/main/Classes/Response/DefaultResponse.php#L55
We have to compare the generated URI (only Host + Path) with the calling URI (also only Host + Path). But the code looks like the request URI is checked and compared without removing the query arguments.
Could you confirm this... then I will check and change that. Perhaps we should also add some test cases: https://github.com/lochmueller/language_detection/blob/main/Tests/Unit/Response/DefaultResponseTest.php#L21
Regards,
Tim

@dtspma
Copy link
Author

dtspma commented Oct 18, 2024

Hi Tim yes, I guess it would work as expected in my use case (no language switch = no redirect) if checkSameUri() would compared the request without the query arguments to the build targetUri which may not have all query arguments but only the ones listed in forwardRedirectParameters.

Method buildRedirectUri() adds existing query parms to the target URI if they are included in forwardRedirectParameters and that's correct and required if a redirect to another language should happen. So the fix has to be done in checkSameUri().

Regards Pascal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants