[PATCH] Fix zero length stylesheet cache files bug in case of missing CSS files

We actually have File.atomic_write for this.

    #   File.atomic_write("/data/something.important", "/data/tmp") do |f|
    #     file.write("hello")
    #   end

the js and css caching should both probably be using that

OK. Cool.

I'll have a look at atomic_write and update the patch as required

You shouldn't use File#readable? because it doesn't make sense on Windows, in fact that method is used to check if the file is readable by the effective user id of the current process.

I suggest File#exist? instead.

I've had a go using atomic_write and it only ensures that the asset cache file is not created. It will not take care of any exceptions raised when trying to read non-existent source asset files.

I think it boils down to these two options:

1) A missing source asset file always raises an exception and never creates a partial asset cache file.

2) Missing source asset files are ignored and the asset cache file is created only from the available source asset files.

If we went with option #1 Migrations will not run on a fresh database there is the additional complication of "should the exception be raised always, whether we are caching asset files or not?"

That way you can spot the missing file(s) during development and not end up with a production-only exception.

My vote is for solution #1 Migrations will not run on a fresh database, but I wouldn't mind some additional input. I have the patch for solution #1 Migrations will not run on a fresh database written already, just adding some tests.

@Luca Thanks for pointing that out. The definitive patch will use .exists? for Windows compatibility.

While trying to write tests for solution 1 I've discovered that a lot of the asset_tag_helper test make references to non-existent stylesheets/js files that break the tests because they only check for the return tags, not the actual files.

Writing the tests for solution 1 would require a large amount of changes to the actual test suite for asset_tag_helper

Is it worth the effort?

I definitely prefer option 1, have a go at what the test changes will look like.

Done. New patch attached.

An exception will be raised if a local javascript/stylesheet file included by the stylesheet_link_tag or javascript_include_tag can not be found.

When caching is enabled, we use atomic_write to ensure that the cache file is not created with zero length.

As I had to rework a large chunk of the asset_tag_helper test suit, I merged into it my related patch #2739 [PATCH] Fix (and strengthen) a couple of cache related stylesheet_link_tag te... which improves the test suite. Feel free to close that ticket.

Christos, in your patch, #ensure_stylesheet_sources! is declared twice, please can you clean the code?
Applied, all tests pass.
+1

Oh sorry, I'm wrong. It isn't declared twice, but hey have very similar code..

I don't think it makes sense to have stylesheet_link_tag and javascript_include_tag assert the presence of the files when they are not being cached or concatenated.

My reason for this, is that doing so makes it impossible to have the css or javascript be generated dynamically through an action or middleware.

Am I alone in thinking that the call to ensure_stylesheet_sources! should be removed?

Another situation where this breaks is in situations where a document root other than RAILS_ROOT/public is used (e.g. for multi-site apps).

A less desirable alternative would be to expose the "dumb" versions of these helpers (javascript_src_tag, stylesheet_tag) as public.

@Sam,

I'd take a patch which moved that check to be solely there when :cache
or :concatenate is true

The idea of my original patch was to catch the missing asset files in development mode, so that you don't deploy to production and discover a missing asset file (either by way of the exception, or by way of the messed up CSS).

Wouldn't it make more sense to explicitly declare dynamic stylesheets as such?

stylesheet_link_tag('css_controller_name/dynamic.css', :dynamic => true)

We can cover both of these cases by raising when :cache is true,
irrespective of the value of perform_caching

Here's the patch.

Missing source files will be still be caught if the cache or concat options are given.

This bit us too. We had a nonexistent javascript file that was generated outside of javascript_include_tag.

This has been applied already, in a modified way. Can close this ticket.