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

Patch to make asset_timestamp respect public_folder setting #1090

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@AtoxIO

AtoxIO commented Mar 3, 2013

Currently asset_timestamp just uses Padrino.root + "public" + appname as the asset folder. Normally this is equal to the public_folder setting, unless someone overrides it in their app.rb. This fixes that issue by combining the source path with public_folder instead.

NOTE: This fixes the asset_folder bug in my previous pull request. I am aware this fails some tests which it shouldn't. Could someone please look into that for me, I am short on time.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero
Contributor

dariocravero commented Mar 11, 2013

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 11, 2013

Member

I think this is worth considering. Its a bug and a only a small, reasonable fix.

Member

skade commented Mar 11, 2013

I think this is worth considering. Its a bug and a only a small, reasonable fix.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 11, 2013

Member

OK moved back to 0.11.0 then if you guys want to review and apply it. I just saw he mentioned it was failing tests.

Member

nesquena commented Mar 11, 2013

OK moved back to 0.11.0 then if you guys want to review and apply it. I just saw he mentioned it was failing tests.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Mar 11, 2013

Member

self.class.public_folder does not work. Is it the right way to retreive public folder from within helpers?

Member

ujifgc commented Mar 11, 2013

self.class.public_folder does not work. Is it the right way to retreive public folder from within helpers?

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 11, 2013

Member

settings.public_folder is.

Member

skade commented Mar 11, 2013

settings.public_folder is.

@AtoxIO

This comment has been minimized.

Show comment
Hide comment
@AtoxIO

AtoxIO Mar 11, 2013

Hey everybody, I added a two fixes. One for the tests and one for absolute files.

Now I am finding it very annoying to spend most of my code on working around the tests, seems silly to me. Sure the tests pass, but I have considered simply adding return if PADRINO_ENV == "test". Isn't there a better solution to this madness?

AtoxIO commented Mar 11, 2013

Hey everybody, I added a two fixes. One for the tests and one for absolute files.

Now I am finding it very annoying to spend most of my code on working around the tests, seems silly to me. Sure the tests pass, but I have considered simply adding return if PADRINO_ENV == "test". Isn't there a better solution to this madness?

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 11, 2013

Member

@AtoxIO

No environment-dependent code please :).

A better solution would be to include a default setting for public_folder

Member

skade commented Mar 11, 2013

@AtoxIO

No environment-dependent code please :).

A better solution would be to include a default setting for public_folder

@AtoxIO

This comment has been minimized.

Show comment
Hide comment
@AtoxIO

AtoxIO Mar 11, 2013

@skade
No environment-dependent code please :).

Yeh, that's why I didn't do it, still, it's annoying not to have common methods in the tests.

Also, the build failure is not my doing, I only modified padrino-helpers, and padrino-gen is failing. So I guess my code is good to merge.

AtoxIO commented Mar 11, 2013

@skade
No environment-dependent code please :).

Yeh, that's why I didn't do it, still, it's annoying not to have common methods in the tests.

Also, the build failure is not my doing, I only modified padrino-helpers, and padrino-gen is failing. So I guess my code is good to merge.

@wentzt

This comment has been minimized.

Show comment
Hide comment
@wentzt

wentzt Mar 16, 2013

Just ran into this. I would love to see this fix get in 0.11.0

wentzt commented Mar 16, 2013

Just ran into this. I would love to see this fix get in 0.11.0

@ghost ghost assigned nesquena Mar 16, 2013

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 16, 2013

Member

OK, I'll take a look at this for 0.11.0

Member

nesquena commented Mar 16, 2013

OK, I'll take a look at this for 0.11.0

nesquena added a commit that referenced this pull request Mar 16, 2013

@nesquena nesquena closed this Mar 16, 2013

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 16, 2013

Member

OK, I merged a version of this into latest edge, can you guys try it and confirm? /cc @AtoxIO @wentzt

Member

nesquena commented Mar 16, 2013

OK, I merged a version of this into latest edge, can you guys try it and confirm? /cc @AtoxIO @wentzt

@AtoxIO

This comment has been minimized.

Show comment
Hide comment
@AtoxIO

AtoxIO Mar 16, 2013

I'm sorry, but I think this will not work, since this has my original bug in it. The problem is that uri_root_path does not simply append to public_folder. You need File.join(asset_folder, source).

Example:

javascript_include_tag "example"
# public_folder = "../public_html/admin"

# File.join(asset_folder, source) = "javascripts/example.js"
# asset_timestamp's final path: "../public_html/admin/javascripts/example.js"

# uri_root_path(asset_folder, source) = "/admin/javascripts/example.js"
# asset_timestamp  final path: "../public_html/admin/admin/javascripts/example.js"

Also, there is a second bug that preexisted my patch, imagine the file http_get.js http_get.png. You should match against %r(^(/|http://)} instead.

Also, too bad that absolute files are no longer given timestamps with this patch, used to be a useful feature, even if it only worked for the default public_folder path.

AtoxIO commented Mar 16, 2013

I'm sorry, but I think this will not work, since this has my original bug in it. The problem is that uri_root_path does not simply append to public_folder. You need File.join(asset_folder, source).

Example:

javascript_include_tag "example"
# public_folder = "../public_html/admin"

# File.join(asset_folder, source) = "javascripts/example.js"
# asset_timestamp's final path: "../public_html/admin/javascripts/example.js"

# uri_root_path(asset_folder, source) = "/admin/javascripts/example.js"
# asset_timestamp  final path: "../public_html/admin/admin/javascripts/example.js"

Also, there is a second bug that preexisted my patch, imagine the file http_get.js http_get.png. You should match against %r(^(/|http://)} instead.

Also, too bad that absolute files are no longer given timestamps with this patch, used to be a useful feature, even if it only worked for the default public_folder path.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 16, 2013

Member

Also, too bad that absolute files are no longer given timestamps with this patch, used to be a useful feature, even if it only worked for the default public_folder path.

It added a lot of complexity to the methods which I wanted to simplify. Also what's an example use case for specifying an absolute path into the app's public folder? If you want an asset_timestamp, why specify the absolute path over the more traditional methods?

Member

nesquena commented Mar 16, 2013

Also, too bad that absolute files are no longer given timestamps with this patch, used to be a useful feature, even if it only worked for the default public_folder path.

It added a lot of complexity to the methods which I wanted to simplify. Also what's an example use case for specifying an absolute path into the app's public folder? If you want an asset_timestamp, why specify the absolute path over the more traditional methods?

@AtoxIO

This comment has been minimized.

Show comment
Hide comment
@AtoxIO

AtoxIO Mar 16, 2013

Easy, third party software. Say you're adding a javascript/php file manager, and you don't want to break up the original library paths for easier upgrading. Saves a ton of bandwidth to provide a never-expires on the javascripts. I know there are better deployments possible, but sinatra makes quick-and-dirty hacking so much fun!

I was thinking, since rack manages to find the location of absolute files, can't we use a rack method to resolve an uri to an absolute path?

AtoxIO commented Mar 16, 2013

Easy, third party software. Say you're adding a javascript/php file manager, and you don't want to break up the original library paths for easier upgrading. Saves a ton of bandwidth to provide a never-expires on the javascripts. I know there are better deployments possible, but sinatra makes quick-and-dirty hacking so much fun!

I was thinking, since rack manages to find the location of absolute files, can't we use a rack method to resolve an uri to an absolute path?

nesquena added a commit that referenced this pull request Mar 16, 2013

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 16, 2013

Member

Made some more tweaks that I think might have fixed the bugs, can you confirm? fe32c00...master Let me know if any other tweaks need to be made regarding the bug.

I don't understand your third party software example, why not just load that using a relative path? What makes the absolute path useful in that case?

Member

nesquena commented Mar 16, 2013

Made some more tweaks that I think might have fixed the bugs, can you confirm? fe32c00...master Let me know if any other tweaks need to be made regarding the bug.

I don't understand your third party software example, why not just load that using a relative path? What makes the absolute path useful in that case?

@AtoxIO

This comment has been minimized.

Show comment
Hide comment
@AtoxIO

AtoxIO Mar 16, 2013

You are still using uri_root_path result_path, instead of source for the timestamp path, check my previous example (#1090 (comment)) to understand what problem that causes.

Also, about the relative/absolute path. Relative paths get /javascripts/ prepended to .js files. If you don't want that, but do want a timestamp, it becomes a useful feature.

AtoxIO commented Mar 16, 2013

You are still using uri_root_path result_path, instead of source for the timestamp path, check my previous example (#1090 (comment)) to understand what problem that causes.

Also, about the relative/absolute path. Relative paths get /javascripts/ prepended to .js files. If you don't want that, but do want a timestamp, it becomes a useful feature.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 16, 2013

Member

I see, thanks for reviewing the patch, I believe that last commit fixes the issue with uri_root being used to calculate timestamp.

Member

nesquena commented Mar 16, 2013

I see, thanks for reviewing the patch, I believe that last commit fixes the issue with uri_root being used to calculate timestamp.

@AtoxIO

This comment has been minimized.

Show comment
Hide comment
@AtoxIO

AtoxIO Mar 16, 2013

thumbs up

The absolute file path stuff is not that important. I will write a new uri_to_absolute_path helper in the future for those use cases.

AtoxIO commented Mar 16, 2013

thumbs up

The absolute file path stuff is not that important. I will write a new uri_to_absolute_path helper in the future for those use cases.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 16, 2013

Member

Ok, thanks again.

Member

nesquena commented Mar 16, 2013

Ok, thanks again.

@wentzt

This comment has been minimized.

Show comment
Hide comment
@wentzt

wentzt Mar 16, 2013

Looks good on my end. Thanks!

wentzt commented Mar 16, 2013

Looks good on my end. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment