[bug?] Haml engine local assigns modifies Object.

[Nick Howell]

In Haml::Engine#render:
<pre>
176 def render(scope = Object.new, &block)
177 @scope_object = scope
178 @buffer = Haml::Buffer.new(@options)
179
180 local_assigns = @options[:locals]
181
182 # Get inside the view object's world
183 @scope_object.instance_eval do
184 # Set all the local assigns
185 local_assigns.each do |key,val|
186 self.class.send(:define_method, key) { val }
187 end
188 end
</pre>

Notice that scope and @scope_object are, by default, Objects. When
local_assigns are...assigned...the Object class has new methods
defined on it. This is *bad*.

I suggest that we replace it with this:
<pre>
m = Module.new
locals_assigns.each { |k, v| m.send(:define_method, k) { m } }
@scope_object.send(:extend, m)
</pre>

Thoughts?

Nick

[Nathan Weizenbaum]

You're right, it should be like that. I don't know how it snuck through. I'll fix it later today.

- Nathan

[Nick Howell]

Or, if you want an uber-one-liner:

@scope_object.send(:extend, @options[:locals].inject(Module.new) { |m,
k, v| m.send(:define_method, k) { v } })

Is it just me, or could Haml use some code-cleanup?

Nick

On Apr 12, 6:34 pm, "Nathan Weizenbaum" <nex...@gmail.com> wrote:
> You're right, it should be like that. I don't know how it snuck through.
> I'll fix it later today.
>
> - Nathan
>

[Nathan Weizenbaum]

That actually breaks... k ends up with a two element key/value array and
v ends up with nil. Also, inject takes the return value of the last
block as the memo value for the next block, so you'd have to do

@scope_object.extend(@options[:locals].inject(Module.new) { |m, k| m.send(:define_method, k[0]) { k[1] }; m })

As to the messiness of the codebase, I think to some extent it's
inevitable. As we add more features, write more stuff to handle edge
cases, etc. it's not always possible to keep it elegant. Not to mention
that, as humans, we tend to make stupid mistakes from time to time (like
this one... heh).

As part of the 2.0 release, I'm planning to do some serious reworking of
the Engine/Buffer rendering system. This is primarily to increase
efficiency by making as much rendering as possible happen before the
template is cached, but it'll also offer a good opportunity to take a
good look at the code that's currently doing the rendering and clean it
up as much as possible.

That's not to say that it wouldn't be worthwhile to do some cleaning
now, though. If you're interested, by all means go for it.

- Nathan

[Nick Howell]

Doh. 'spose I shoulda tested it. Especially since that was the first
time I'd ever used inject. I feel like an ass now.

Thanks for getting that in so quick; I post and the next time I update
the fix is in.

I'll keep my eyes out for anything that looks like an easy fix, and
post up if I find anything interesting.

I've actually been kinda interested in modifying the Engine through
redefining certain methods; it would be nice if the compile and render
methods could be factored into sub-methods (like an assign_locals
method, in this case), to allow for easier overrides. If you're gonna
be reworking it pretty significantly for 2.0, I won't bother messing
with it now.

I'll definitely be looking forward to performance improvements, though
I've been pleasantly surprised by the current implementation (I'd read
in a few places that Haml was a lot slower than ERB).

Nick