This repository was archived by the owner on Mar 27, 2023. It is now read-only.
Fix empty SCRIPT_NAME with partial match route bug #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
drewdeponte
added a commit
that referenced
this pull request
Jan 24, 2016
Fix empty SCRIPT_NAME with partial match route bug
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.