Patch to make asset_timestamp respect public_folder setting

[AtoxIO]

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]

@padrino/core-members ?

[skade]

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

[nesquena]

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]

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

[skade]

settings.public_folder is.

[AtoxIO]

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]

@AtoxIO

No environment-dependent code please :).

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

[AtoxIO]

@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]

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

[nesquena]

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

[nesquena]

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

[AtoxIO]

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]

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]

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]

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]

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]

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

[AtoxIO]

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]

Ok, thanks again.

[wentzt]

Looks good on my end. Thanks!