Merged
Conversation
Contributor
Author
|
Looks like this may be difficult to do compatibly from Rack 2 through 3.2 🙁 |
Contributor
You can always check the rack version and add a small compatibility layer. |
hmcguire-shopify
force-pushed
the
hm-muwxvywlrxnkwqlx
branch
5 times, most recently
from
6702a97 to
b678ae9
Compare
Contributor
Author
Done 👍 I think this should be good now |
byroot
reviewed
Dec 8, 2025
lib/pitchfork.rb
Outdated
Comment on lines
+196
to
+198
| rescue LoadError | ||
| warn 'rack not available, functionality reduced' | ||
| end |
Contributor
There was a problem hiding this comment.
Pitchfork has a runtime dependency on rack. So this shouldn't be needed.
byroot
reviewed
Dec 8, 2025
lib/pitchfork.rb
Outdated
Comment on lines
+18
to
+19
| singleton_class.attr_accessor :path_info_requires_leading_slash | ||
| self.path_info_requires_leading_slash = true |
Contributor
There was a problem hiding this comment.
I'd just make this a constant.
Rack is a dependency in the gemspec
hmcguire-shopify
force-pushed
the
hm-muwxvywlrxnkwqlx
branch
from
b678ae9 to
e0e1f3d
Compare
byroot
approved these changes
Dec 9, 2025
lib/pitchfork/http_parser.rb
Outdated
| e['pitchfork.socket'] = socket | ||
| e['rack.hijack'] = self | ||
|
|
||
| if Pitchfork::PATH_INFO_REQUIRES_LEADING_SLASH && e['PATH_INFO'] == '*' |
Contributor
There was a problem hiding this comment.
Suggested change
| if Pitchfork::PATH_INFO_REQUIRES_LEADING_SLASH && e['PATH_INFO'] == '*' | |
| if PATH_INFO_REQUIRES_LEADING_SLASH && e['PATH_INFO'] == '*' |
nitpick
One of the changes in Rack 3.2 was a fix to ensure at least one of
PATH_INFO or SCRIPT_NAME are non-empty. This causes a Rack::Lint error
in Pitchfork for `OPTIONS *` requests because Pitchfork has a `nil`
`PATH_INFO` for these.
This commit fixes the issue by setting `PATH_INFO` to `*`. It also undoes
some of the previous changes from the [commit][1] to support Rack 3.1
because they appear to have no effect.
The undone change is the moving of `PATH_INFO = "*"` and `REQUEST_PATH =
"*"` to the `fragment` action. From what I can tell, this part of the
state machine is not involved with the request uri, and so this if
statement is not evaluated in `OPTIONS *` requests:
```
Request_Line = ( Method " " Request_URI ("#" Fragment){0,1} " " HTTP_Version CRLF ) ;
```
Then, the actual fix is manually setting the `PATH_INFO` to `*` when
that's the given `REQUEST_URI`. Normally, `PATH_INFO` is handled in the
`request_uri` action, but that action is only run for URIs starting with
`/` (and explicitly excludes `*`).
[1]: 65725c5
hmcguire-shopify
force-pushed
the
hm-muwxvywlrxnkwqlx
branch
from
e0e1f3d to
9e5308e
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
One of the changes in Rack 3.2 was a fix to ensure at least one of PATH_INFO or SCRIPT_NAME are non-empty. This causes a Rack::Lint error in Pitchfork for
OPTIONS *requests because Pitchfork has anilPATH_INFOfor these.This commit fixes the issue by setting
PATH_INFOto*. It also undoes some of the previous changes from the commit to support Rack 3.1 because they appear to have no effect.The undone change is the moving of
PATH_INFO = "*"andREQUEST_PATH = "*"to thefragmentaction. From what I can tell, this part of the state machine is not involved with the request uri, and so this if statement is not evaluated inOPTIONS *requests:Then, the actual fix is manually setting the
PATH_INFOto*when that's the givenREQUEST_URI. Normally,PATH_INFOis handled in therequest_uriaction, but that action is only run for URIs starting with/(and explicitly excludes*).