http_router
http_router copied to clipboard
joshbuddy
Fix empty SCRIPT_NAME with partial match route bug
Why you made the change:
I made this change so that SCRIPT_NAME and PATH_INFO would be set appropriately in the scenario where a partial match route is used.
How the change addresses the need:
There is an issue with the current version of rewrite_partial_path_info where it does not set SCRIPT_NAME appropriately. When the request environments PATH_INFO is set, it causes the request.rack_request.path_info helper return the updated version, not the original. This is requst.rack_request.path_info eventually just reads the PATH_INFO value out of the request environment. As a side effect of this, the original code would always set the SCRIPT_NAME to "" because it would be slicing from request.rack_request.path_info[0, 0] in actuality.
This change resolves this issue by first temporarily storing a dup of the original path_info and using that to determine the end index for the SCRIPT_NAME. This change also handles setting SCRIPT_NAME correctly for the somewhat unique but common case of matching the partial match exactly.
I have added tests for each of these cases as I couldn't find any tests covering this before.
@joshbuddy is there any chance you could review this PR as it fixes a bug in http_router? @jodosha do you have any pull with getting this in? It is the fix that resolves an actual bug in http_router, but also makes http://hanamirb.org work properly with http://sidekiq.org.
@joshbuddy, please review this PR we need this on only for sidekiq ❤️💛💜💗💙
Will take a look!
On Mar 1, 2017, at 14:44, Anton Davydov [email protected] wrote:
@joshbuddy, please review this PR we need this on only for sidekiq ❤️💛💜💗💙
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
@joshbuddy you're my hero 🎉
P.S.: WDYT maybe it'll good to add someone from hanami core for helping with maintaining the project? Because we have some errors with this gem and we will be happy to fix it ASAP.
/cc @jodosha