URIs are always joined with forward slash #11
Conversation
lib/pagy/frontend.rb
Outdated
| @@ -34,7 +34,7 @@ def pagy_info(pagy) | |||
|
|
|||
| # this works with all Rack-based frameworks (Sinatra, Padrino, Rails, ...) | |||
| def pagy_url_for(n) | |||
| url = File.join(request.script_name.to_s, request.path_info) | |||
| url = "#{request.script_name}/#{request.path_info}" | |||
There was a problem hiding this comment.
Actually, this is not even needed. According to Rack specification:
The
SCRIPT_NAME, if non-empty, must start with/
ThePATH_INFO, if non-empty, must start with/
One ofSCRIPT_NAMEorPATH_INFOmust be set.PATH_INFOshould be/ifSCRIPT_NAMEis empty.SCRIPT_NAMEnever should be/, but instead be empty.
And in fact Rack provides a handy #path method for exactly this purpose:
There was a problem hiding this comment.
So why don't we use just url = request.path?
There was a problem hiding this comment.
That would be my recommendation as well. I’ll update the PR, perhaps add a few tests.
`File.join` might use backslash on e.g. Windows - URL paths are not File paths. According to Rack specification: - The `SCRIPT_NAME`, if non-empty, must start with `/` - The `PATH_INFO`, if non-empty, must start with `/` - One of `SCRIPT_NAME` or `PATH_INFO` must be set. `PATH_INFO` should be `/` if `SCRIPT_NAME` is empty. `SCRIPT_NAME` never should be `/`, but instead be empty. And in fact Rack provides this handy `#path` method for exactly this purpose: https://github.com/rack/rack/blob/42e48013dd1b6dbd/lib/rack/request.rb#L406-L408
bquorning
force-pushed
the
uri-is-not-a-file
branch
from
c0bab60
to
8543cd1
Compare
| @@ -23,6 +23,7 @@ Gem::Specification.new do |s| | |||
| s.add_development_dependency 'bundler', '~> 1.16' | |||
| s.add_development_dependency 'rake', '~> 10.0' | |||
| s.add_development_dependency 'minitest', '~> 5.0' | |||
| s.add_development_dependency 'rack' | |||
There was a problem hiding this comment.
I don’t know if rack should in fact be considered a runtime dependency? You cannot use Pagy::Frontend without either having Rack loaded, or overriding Pagy::Frontend#pagy_url_for.
There was a problem hiding this comment.
You cannot use Pagy::Frontend without either having Rack loaded, or overriding Pagy::Frontend#pagy_url_for
True, but...
The pagy_url_for uses rack just to give a default usable on most apps out of the box. Adding an explicit dependency would cause the installation of rack even if you never intended to use rack in your app and you are fine with just overriding the pagy_url_for.
On the other hand not adding it would cause the method to fail until the user will provide his/her own way to create the url, with no feedback added.
The best (puristic) approach would probably be conditionally defining the method in presence of rack, and defining the method raising a NotImplementedError exception if Rack is not present.
Something similar to how the pagy_t is implemented.
There was a problem hiding this comment.
Yeah, that sounds like a valid strategy. Except, just use NoMethodError instead of NotImplementedError: http://chrisstump.online/2016/03/23/stop-abusing-notimplementederror/ (as in: just leave the method not implemented, or raise it with a message about using Rack)
There was a problem hiding this comment.
I improperly used NotImplementedError till now :/.
If we leave it not implemented, there will be no feedback, if we implement it and raise the NoMethodError with a message, that would work, however, respond_to? would return true, but I can live with that :).
I think we should raise the NoMethodError with a clear self-explanatory feedback.
There was a problem hiding this comment.
You want it implemented in this PR, or a new one?
There was a problem hiding this comment.
I am fine with both options. Whatever is better for you. Thanks.
There was a problem hiding this comment.
I wrapped the method in an if block.
But then I thought of another thing: the method does not only require Rack, in order to call #GET and #path on the Request. It also requires a #request method in the class where the module is included…
There was a problem hiding this comment.
Yeah... there is also the params method used by the Pagy::Backend#pagy_get_variables...
I think we should just drop this attempt to be so smart, over-complicating our life, and try to be just practical :). I think that that "problems" could be addressed in a "Requirements" or similar section where we specifically say what is needed to make it work.
For now, I would focus on fixing the bug and eventually adding the tests (we could add rack as a development dependence), because the bug could affect users right now.
Then, if we will find some better way than the "Requirement" section, we could implement it.
There was a problem hiding this comment.
I agree. I have removed the last commit, so this PR is back at just using request.path and adding a few tests where Rack needs to be a development dependency.
bquorning
force-pushed
the
uri-is-not-a-file
branch
from
8543cd1
to
fa52fa8
Compare
|
@ddnexus Ready for review. |
ddnexus
force-pushed
the
dev
branch
2 times, most recently
from
1765527
to
92260b5
Compare
bquorning
force-pushed
the
uri-is-not-a-file
branch
from
c04859d
to
fa52fa8
Compare
File.joinmight use backslash on e.g. Windows - URL paths are not File paths.According to Rack specification:
SCRIPT_NAME, if non-empty, must start with/PATH_INFO, if non-empty, must start with/SCRIPT_NAMEorPATH_INFOmust be set.PATH_INFOshould be/ifSCRIPT_NAMEis empty.SCRIPT_NAMEnever should be/, but instead beempty.
And in fact Rack provides this handy
#pathmethod for exactly this purpose: https://github.com/rack/rack/blob/42e48013dd1b6dbd/lib/rack/request.rb#L406-L408