-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix url_rewriter.hosts not used for output_add_rewrite_var() #13294
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
Fix url_rewriter.hosts not used for output_add_rewrite_var() #13294
Conversation
|
…tput_add_rewrite_var
1f56c6f
to
3cd1a02
Compare
After rebasing to trigger checks again, MACOS fails three Zend tests unrelated to this PR. |
I know this is not a priority as this functionality never worked correctly, but could I ask for a review? |
I will try to find some time on this in the next few weeks. Ping me in case there is no review in the next 3 weeks. |
@bukka pinging you as it has been a little over 3 weeks. |
I have done a bit of testing and it all looked good to me. I also noticed that it was incorrectly using session for url-rewriter:
(this was previous master run without the patch applied) So it was actually other way round in some cases, right? I'm thinking that I should maybe extend upgrading note (maybe check if the current info makes sense to you) Due to potentially BC break I picked only master for this which this PR also targeted. |
Yes, the end result of the rewriting was the other way around in some cases (not the mix-up of host lists though). I don't remember the exact details but I think this had something to do with the code below where php-src/ext/standard/url_scanner_ex.re Lines 747 to 752 in 6615476
|
) Document the bugfix of php/php-src/pull/13294. This is a part of #3872. Co-authored-by: haszi <haszika80@gmail.com>
Fixes the bug where
output_add_rewrite_var()
is using the hosts set insession.trans_sid_hosts
instead of theurl_rewriter.hosts
which was added specifically for this purpose.