Skip to content
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

assets:precompile should not append asset digests when config.assets.digest is false #2782

Merged
merged 1 commit into from
Sep 1, 2011

Conversation

spohlenz
Copy link
Contributor

See my comment on #2768.

Basically, given that config.assets.digest can be set to false in production mode, rake assets:precompile should respect this configuration and not append asset digests to the filenames when set to false.

@rhulse
Copy link

rhulse commented Sep 1, 2011

Sorry, but allowing compilation without digests would be a Bad Thing. Unless I am missing something, this is not something that should ever be switched off in production.

:-)

It is assumed that you will have far-future headers set for files compiled to /assets. If you do not have digests on then it is impossible to get remote clients to refetch the assets. If you precompile without digests then you should not have far-future headers set, which kind of negates the whole point of the feature, IMHO.

@spohlenz
Copy link
Contributor Author

spohlenz commented Sep 1, 2011

Agree to an extent, but config.assets.digest has been added as a config option (see production.rb), and this option is useless (and will break assets) if assets:precompile does not respect it.

I still need to fix/add some tests for this.

@rhulse
Copy link

rhulse commented Sep 1, 2011

Sure, yes it does break in that case. It just doesn't make sense to turn it off :-)

@josevalim
Copy link
Contributor

I believe we should respect the user configuration, regardless if it makes sense or not. +1 for this pull request. @spohlenz, could you please rebase it? After you rebase it, please ping/mention me.

@spohlenz
Copy link
Contributor Author

spohlenz commented Sep 1, 2011

Squashed and rebased. Thanks @josevalim.

@josevalim
Copy link
Contributor

/cc @spastorino @guilleiguaran

@guilleiguaran
Copy link
Member

@josevalim, agree. I discourage do it but is a user choice. +1

josevalim added a commit that referenced this pull request Sep 1, 2011
assets:precompile should not append asset digests when config.assets.digest is false
@josevalim josevalim merged commit 8b049a1 into rails:3-1-stable Sep 1, 2011
@josevalim
Copy link
Contributor

Merged, can we have the same for master?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants