upgrade to 2.3.2: render started matching vim-patchmode *.orig files

rubyonrails-core
1

As of 2.3.2 render started loading vim patch mode files ( '*.erb.orig' instead of '*.erb' ):

How to reproduce: create *.erb.orig files in the appropriate view directory:

rails 2.3.2 loads:

app/views/articles/new.html.erb.orig

instead of:

app/views/articles/new.html.erb

This is probably because of the way ActiionView::ReloadableTemplate::ReloadablePath is implemented. The keys it contains come from the relative path without the extension, so it gets:

@paths[ 'sessions/logout.html.erb' ] => template # from login.html.erb

and the same hash value gets overwritten:

@paths[ 'sessions/logout.html.erb' ] => template # from login.html.erb.orig

The quick hack I made just reverses the file listing (Dir.glob.sort.reverse), so the *.erb files come after the *.erb.orig files, and replace those created from .orig files (patch below).

NOTE: The problem appeared in RSpec, which may load templates differently than production/non rspec/unit test/console - that is why I changed all 3 Dir.glob instances.

--- reloadable_template.rb.orig 2009-03-18 12:27:45.000000000 +0100 +++ reloadable_template.rb 2009-03-19 18:48:31.109227986 +0100 @@ -69,7 +69,7 @@

         # get all the template filenames from the dir          def template_files_from_dir(dir) - Dir.glob(File.join(dir, '*')) + Dir.glob(File.join(dir, '*')).sort.reverse          end      end

--- template.rb.orig 2009-03-18 12:27:45.000000000 +0100 +++ template.rb 2009-03-19 18:51:29.281239443 +0100 @@ -78,7 +78,7 @@

       private          def templates_in_path - (Dir.glob("#{@path}/**/*/**") | Dir.glob("#{@path}/ **")).each do |file| + (Dir.glob("#{@path}/**/*/**") | Dir.glob("#{@path}/ **")).sort.reverse.each do |file|              yield create_template(file) unless File.directory?(file)            end          end

2

Update to my own post:

There are two paths depending on whether you have config.action_view.cache_template_loading or not.

(Either EagerPath.load! or ReloadablePath.register_template)

EXAMPLE: Given the files:     views/main/index.html.erb     views/main/index.html.erb.backup

RESULT: get '/main' renders 'views/main/index.html.erb.backup'

PROBLEM:

In both cases (Eager, ReloadablePath) "@path[path]" is assigned twice, setting "index.html.erb.backup" as the final template.

This is not relevant for production mode, since you don't usually have backups of editor files there.

Backup files such as: "index.html.erb~" are handled correctly, BTW - added to the list, but never used because of unique extension ("erb~").

QUESTION: Why does ActionView consider the *.backup files anyway?

ANSWER: They pass the ActionView::Template.split test:

      elsif valid_extension?(m[1]) # format and extension

The thing is, index.html.erb.orig has 3 extensions m = ['html', 'erb', 'orig'] and m[2] is ignored. Idea: join m[1] and m[2] and return as extension?

QUESTION: shouldn't both implementations(Eager/Reloadable) be identical, except for handling _when_ to load the templates?

The differences may cause some problems to come up only in one case (w/wo caching). E.g. my guess is that broken symbolic links will be handled differently - ReloadablePath uses File.file?, which returns false for the broken symlinks that the Dir.glob returns. EagerPath doesn't seem to check that.

SOLUTION; Only assign a template once, don't clobber the previous value. This works, because Dir.glob returns a sorted list, where 'index.html.erb' is at the top.

PATCH: The patch becomes trivial now:

--- template.rb.orig 2009-03-18 12:27:45.000000000 +0100 +++ template.rb 2009-03-20 12:15:14.698483807 +0100 @@ -64,7 +64,7 @@           templates_in_path do |template|             template.load!             template.accessible_paths.each do |path| - @paths[path] = template + @paths[path] ||= template #don't clobber previous entry with *.erb.backup files             end           end           @paths.freeze

--- reloadable_template.rb.orig 2009-03-18 12:27:45.000000000 +0100 +++ reloadable_template.rb 2009-03-20 13:13:43.866482672 +0100 @@ -41,7 +41,7 @@

          def register_template(template)             template.accessible_paths.each do |path| - @paths[path] = template + @paths[path] ||= template #don't clobber previous entry with *.erb.backup files             end           end

Seems to work fine now. But does this solve/create other problems with templates and caching?

Hope this helps and possibly prevents some headaches in the future.

czarek wrote: