padrino / padrino-framework

Padrino is a full-stack ruby framework built upon Sinatra.
http://www.padrinorb.com
MIT License
3.36k stars 508 forks source link

It's possible to call partials by name with extension (partial 'foo.haml'), but it would fail if :collection is passed #1636

Closed lolmaus closed 10 years ago

lolmaus commented 10 years ago

I encountered this bug in Middleman. Never tried it in pure Padrino.

Padrino allows calling partials with extension, e. g. partial 'foo.haml'.

This works fine, until you try to pass the :collection argument to partial. If you do so, Padrino crashes with an "invalid locals key" exception.

The problem here is that Padrino/Tilt would try in the partial to declare local variables with variable names based on partial names. E. g. for partial :foo, collection: [] there would be foo and foo_counter variables declared.

Now, if you call a partial with extension, it would try to declare invalid variable names and fail: partial 'foo.haml', collection: [] -- tries to declare variables with names foo.haml and foo.haml_counter.

Here's the line of code responsible for raising an exception for such variable names: https://github.com/rtomayko/tilt/blob/1.4.1/lib/tilt/template.rb#L222

PS You might consider forbidding to call a partial with extension, but there's at least one workflow when it's necessary.

It is technically possible to store partials with identical names yet different extensions in one folder, e. g.:

source/
  _foo.haml
  _foo.sass

You might consider that to be a bad practice, but hear me out. In my project, each module of a page is represented by a haml file and a sass file. I found it very convenient to store both files in one place, so that they appear next to each other. I no longer have to scroll my project directory tree all the way to and fro when theming a page module.

If there is more than one partial with the same name and you do a partial call without specifying the extension (e. g. partial :foo), Padrino will pick one of those partials and ignore others. So providing the extension is necessary in this use case.

ujifgc commented 10 years ago

Thank you for you thorough explanation. Fixed.

bhollis commented 10 years ago

I believe this will fail if there is a . somewhere else in the path. So a path like /more.partials/foo.html or /_my.awesome.partial.html. I'm not saying it's a great idea to name partials like that, but somebody's going to do it.

ujifgc commented 10 years ago

/more.partials/foo.html won't fail as there's template.to_s.rpartition(File::SEPARATOR) before. /_my.awesome.partial.html won't fail, it will register local variable my.

bhollis commented 10 years ago

OK, sounds good then.