(This is the second of two interesting problems I've run into when
integrating Erector into our large, existing ERb-based Rails app --
see my previous message for more info...)
We're also trying to enable conversion of partials to Erector,
piecemeal -- i.e., you can make a partial use Erector while letting
the calling view stay in ERb. As far as I can tell, right now Erector
passes all controller instance variables to the partial unilaterally,
and omits locals -- 'quux' is unavailable to the widget
Views::Foo::Bar::Baz when called like so:
1. Partials can't use "needs" in any reasonable fashion, because
they get passed all of the controller's instance variables and will
blow up when extras come through;
2. Partials don't get locals at all.
So I added the following behavior:
A. If partials have a 'needs' clause and are rendered via
"render :partial", they silently ignore extra controller variables;
B. Locals are passed through to partials;
C. Locals override controller instance variables with the same name
(*)
(*) ERb has two namespaces for variables in partials: controller
variables ('@foo') and local variables ('foo'). Erector only has one;
IMHO this is much nicer. But if there's a collision you have to pick,
and this seemed clearly the right choice.
This helped a whole lot. However, there's one remaining issue of
hygiene: partials can still have access to any controller instance
variables they want; there's still this global namespace sitting
around, waiting for dependency-at-a-(long)-distance to be added
accidentally. So I added a single additional Erector::Widget class
method: controller_assigns_propagate_to_partials=. Iff you set this to
false (it's true by default), then "render :partial" *only* passes
through variables specified in :locals; controller variables are
ignored entirely. It's inherited, so you can set it on branches of
your view hierarchy easily and it "just works". It only affects the
"render :partial" case; direct widget instantiation, Erector's
'widget' command, and everything else are not affected.
This lets you choose whether to allow the messiness of letting
controller instance variables propagate down arbitrarily to partials
(probably more compatible with existing apps) or to choose a more
maintainable, hygienic route, and do it on arbitrary swaths of your
view hierarchy.
Same deal as the last message -- please let me know if any of this
seems crazy, or I'm missing something totally obvious that handles
this problem better. I'm trying to solve these problems in a
compatible, non-intrusive manner, but one that also helps Erector
evolve towards fitting in more and more smoothly with existing, large- scale Rails apps that weren't built with Erector in mind (and which
were built with less-than-perfect discipline...which we all have ;).
I'll happily work towards making this as clean and good as possible.
Just realized I didn't send my last message to the whole list. And now
that I've read Andrew's other message, I'll use this opportunity to
send a more informed reply!
<john.fireba...@gmail.com> wrote:
> On Sun, Nov 22, 2009 at 1:03 PM, Andrew Geweke <agew...@gmail.com> wrote:
>> So I added the following behavior:
>> A. If partials have a 'needs' clause and are rendered via
>> "render :partial", they silently ignore extra controller variables;
>> B. Locals are passed through to partials;
>> C. Locals override controller instance variables with the same name
>> (*)
>> (*) ERb has two namespaces for variables in partials: controller
>> variables ('@foo') and local variables ('foo'). Erector only has one;
>> IMHO this is much nicer. But if there's a collision you have to pick,
>> and this seemed clearly the right choice.
> This sounds workable to me. Another option would be to store the
> locals off somewhere other than instance variables and autogenerate an
> attr_accessor for each of them. Since the syntax for local variables
> is the same as local methods (providing you don't use a 'self'
> prefix), this would give you essentially the same dual namespaces as
> the ERb partial. But I think I agree that having a single namespace is
> nicer.
>> This helped a whole lot. However, there's one remaining issue of
>> hygiene: partials can still have access to any controller instance
>> variables they want; there's still this global namespace sitting
>> around, waiting for dependency-at-a-(long)-distance to be added
>> accidentally. So I added a single additional Erector::Widget class
>> method: controller_assigns_propagate_to_partials=. Iff you set this to
>> false (it's true by default), then "render :partial" *only* passes
>> through variables specified in :locals; controller variables are
>> ignored entirely. It's inherited, so you can set it on branches of
>> your view hierarchy easily and it "just works". It only affects the
>> "render :partial" case; direct widget instantiation, Erector's
>> 'widget' command, and everything else are not affected.
> I've been finding that autopropagation of controller assigns is
> problematic even in the non-partial case. I think it might make sense
> to use this to replace my NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES
> hack and have it work for direct widget instantiation and the 'widget'
> command as well. I.e. if it's set, controller assigns propagate only
> if they are needed or passed explicitly via :locals.
It looks like this is exactly what your
ignore_extra_controller_assigns= option does -- in which case +1, I
wholeheartedly support it. I would just drop
NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES, I think this can replace it
entirely.
On Nov 23, 2009, at 9:43 AM, John Firebaugh wrote:
>> I've been finding that autopropagation of controller assigns is
>> problematic even in the non-partial case. I think it might make sense
>> to use this to replace my NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES
>> hack and have it work for direct widget instantiation and the
>> 'widget'
>> command as well. I.e. if it's set, controller assigns propagate only
>> if they are needed or passed explicitly via :locals.
> It looks like this is exactly what your
> ignore_extra_controller_assigns= option does -- in which case +1, I
> wholeheartedly support it. I would just drop
> NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES, I think this can replace it
> entirely.
Thanks for the support, Alex and John!
NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES is still needed, as far as I
can tell, in the case where you don't set
"Widget.ignore_extra_controller_assigns = true". Do you think we
should just make that always true for Erector when it's running as
part of Rails, and leave out the configurability? It would make things
simpler, to be sure; I just wanted to be as conservative as possible
with my original patch.
Alex: re: the naming issue -- I agree; the only issue with
"allow_undeclared_parameters" is that it sounds like it should apply
everywhere, and my patch only applies in the Rails case, and only to
the immediate view, not partials. (I really like it that Erector blows
up if I pass extra stuff to my widget, in general, just like Ruby
does...it's just this one case where it has issues.) I'd hate to see
someone say "Erector::Widget.allow_undeclared_parameters = true" and
then go on a wild-goose-chase for hours trying to figure out why it's
not doing what it claims. But I'm all for better names...just having
trouble coming up with one that's specific but less verbose. If we can
brainstorm one, I'll change it right away.
I also just added another commit (http://github.com/ageweke/erector/commit/1cb39a3eb0af22adcd0b2a117d3a... ) that makes Erector work with the Cells plugin -- it doesn't by
default. I totally understand if we don't want that upstream; it's
safe and generic, but specific to this one interaction.
Assuming you're happy with them, what's the next step to getting these
upstream? I'm a Github newbie (the shock!) but am happy to do whatever
makes things easiest for you, too.
On Mon, Nov 23, 2009 at 2:45 PM, Andrew Geweke <agew...@gmail.com> wrote:
> On Nov 23, 2009, at 9:43 AM, John Firebaugh wrote:
> Thanks for the support, Alex and John!
> NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES is still needed, as far as I
> can tell, in the case where you don't set
> "Widget.ignore_extra_controller_assigns = true". Do you think we
> should just make that always true for Erector when it's running as
> part of Rails, and leave out the configurability? It would make things
> simpler, to be sure; I just wanted to be as conservative as possible
> with my original patch.
I think ignore_extra_controller_assigns should default to true for any
widget that has needs, and NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES
should be removed. It was created for the same reason as
ignore_extra_controller_assigns -- the presence of stray instance
variables that cause problems once you start using needs.
The ignore_extra_controller_assigns getter and setter are
Rails-specific and should be in rails_widget.rb, no?
On Nov 23, 2009, at 3:28 PM, John Firebaugh wrote:
> I think ignore_extra_controller_assigns should default to true for any
> widget that has needs, and NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES
> should be removed. It was created for the same reason as
> ignore_extra_controller_assigns -- the presence of stray instance
> variables that cause problems once you start using needs.
Totally agreed. I'll make it that way.
> The ignore_extra_controller_assigns getter and setter are
> Rails-specific and should be in rails_widget.rb, no?
Right now, they aren't, because RailsWidget has the following comment
right above it:
# RailsWidget and InlineRailsWidget are for backward compatibility.
# New code should use Widget, InlineWidget, or Erector.inline.
I can (1) leave them in widget.rb, (2) put them in RailsWidget and we
can "un-deprecate" it, or (3) make them a new module that you can add
to your widget (Erector::RailsCompatibility or something?) if you need
them. I vote for (2) (as (3) just would be confusing -- two separate
"rails widget" classes/modules?), but I don't know the history behind
the deprecation of RailsWidget...
>> The ignore_extra_controller_assigns getter and setter are
>> Rails-specific and should be in rails_widget.rb, no?
> Right now, they aren't, because RailsWidget has the following comment right
> above it:
> # RailsWidget and InlineRailsWidget are for backward compatibility.
> # New code should use Widget, InlineWidget, or Erector.inline.
RailsWidget, the class, is deprecated. rails_widget.rb, the file,
isn't -- it contains the majority of the Rails-specific erector code.
(To reduce the confusion, the non-backward compatibility stuff should
probably be migrated out of there.)
The ignore_extra_controller_assigns stuff should specifically be in
the Erector::Rails::WidgetExtensions module, which is included in
Erector::Widget when erector is used from rails.
On Nov 23, 2009, at 4:14 PM, John Firebaugh wrote:
> RailsWidget, the class, is deprecated. rails_widget.rb, the file,
> isn't -- it contains the majority of the Rails-specific erector code.
> (To reduce the confusion, the non-backward compatibility stuff should
> probably be migrated out of there.)
> The ignore_extra_controller_assigns stuff should specifically be in
> the Erector::Rails::WidgetExtensions module, which is included in
> Erector::Widget when erector is used from rails.
OK, that was embarrassing...sorry about that. I managed to miss that
completely the first time.
506663e7fdbbeb3c4706a4cfb7cb0839d7224ad5 (in my Erector fork) moves
the methods to Erector::Rails::WidgetExtensions, eliminates
NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES, and makes
ignore_extra_controller_assigns default to true in Rails widgets.
I also committed 6294e5ad1bc468bb4d52050121264e02064767ab, which fixes
three (related) spec failures from the parent -- inline widgets didn't
work if you had the Rails code loaded. It'd be awesome if someone can
double-check that change (http://github.com/ageweke/erector/commit/6294e5ad1bc468bb4d5205012126... ) -- I think it's pretty safe, but I always like getting feedback from
those who understand the code base better than I do.
I just reviewed all your patches (sorry for the delay) and I'm OK with
them all. I didn't totally follow the ignore_extra_controller_assigns
discussion between you and John, but I'll assume you guys worked it
all out and do a bit of merging this afternoon.
On Mon, Nov 23, 2009 at 6:33 PM, Andrew Geweke <agew...@gmail.com> wrote:
> On Nov 23, 2009, at 4:14 PM, John Firebaugh wrote:
>> RailsWidget, the class, is deprecated. rails_widget.rb, the file,
>> isn't -- it contains the majority of the Rails-specific erector code.
>> (To reduce the confusion, the non-backward compatibility stuff should
>> probably be migrated out of there.)
>> The ignore_extra_controller_assigns stuff should specifically be in
>> the Erector::Rails::WidgetExtensions module, which is included in
>> Erector::Widget when erector is used from rails.
> OK, that was embarrassing...sorry about that. I managed to miss that
> completely the first time.
> 506663e7fdbbeb3c4706a4cfb7cb0839d7224ad5 (in my Erector fork) moves
> the methods to Erector::Rails::WidgetExtensions, eliminates
> NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES, and makes
> ignore_extra_controller_assigns default to true in Rails widgets.
> I also committed 6294e5ad1bc468bb4d52050121264e02064767ab, which fixes
> three (related) spec failures from the parent -- inline widgets didn't
> work if you had the Rails code loaded. It'd be awesome if someone can
> double-check that change (http://github.com/ageweke/erector/commit/6294e5ad1bc468bb4d5205012126... > ) -- I think it's pretty safe, but I always like getting feedback from
> those who understand the code base better than I do.
> What does everyone think? Looking better?
> Andrew
> --
> You received this message because you are subscribed to the Google Groups "erector" group.
> To post to this group, send email to erector@googlegroups.com.
> To unsubscribe from this group, send email to erector+unsubscribe@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/erector?hl=en.
On Tue, Dec 1, 2009 at 1:17 PM, Alex Chaffee <ale...@gmail.com> wrote:
> Andrew -
> I just reviewed all your patches (sorry for the delay) and I'm OK with
> them all. I didn't totally follow the ignore_extra_controller_assigns
> discussion between you and John, but I'll assume you guys worked it
> all out and do a bit of merging this afternoon.
> On Mon, Nov 23, 2009 at 6:33 PM, Andrew Geweke <agew...@gmail.com> wrote:
>> On Nov 23, 2009, at 4:14 PM, John Firebaugh wrote:
>>> RailsWidget, the class, is deprecated. rails_widget.rb, the file,
>>> isn't -- it contains the majority of the Rails-specific erector code.
>>> (To reduce the confusion, the non-backward compatibility stuff should
>>> probably be migrated out of there.)
>>> The ignore_extra_controller_assigns stuff should specifically be in
>>> the Erector::Rails::WidgetExtensions module, which is included in
>>> Erector::Widget when erector is used from rails.
>> OK, that was embarrassing...sorry about that. I managed to miss that
>> completely the first time.
>> 506663e7fdbbeb3c4706a4cfb7cb0839d7224ad5 (in my Erector fork) moves
>> the methods to Erector::Rails::WidgetExtensions, eliminates
>> NON_NEEDED_CONTROLLER_INSTANCE_VARIABLES, and makes
>> ignore_extra_controller_assigns default to true in Rails widgets.
>> I also committed 6294e5ad1bc468bb4d52050121264e02064767ab, which fixes
>> three (related) spec failures from the parent -- inline widgets didn't
>> work if you had the Rails code loaded. It'd be awesome if someone can
>> double-check that change (http://github.com/ageweke/erector/commit/6294e5ad1bc468bb4d5205012126... >> ) -- I think it's pretty safe, but I always like getting feedback from
>> those who understand the code base better than I do.
>> What does everyone think? Looking better?
>> Andrew
>> --
>> You received this message because you are subscribed to the Google Groups "erector" group.
>> To post to this group, send email to erector@googlegroups.com.
>> To unsubscribe from this group, send email to erector+unsubscribe@googlegroups.com.
>> For more options, visit this group at http://groups.google.com/group/erector?hl=en.
I'm getting two failures in the Rails suite post-merge, both in render_spec:
1)
'ActionController::Base#render should raise when rendering a widget
class with implicit assigns and too many variables' FAILED
expected RuntimeError with message matching /Unknown parameter.*baz/,
got #<RuntimeError: Unknown parameter 'template'. NeedsWidget only
accepts foo, bar>
2)
'ActionController::Base#render should raise if rendering a #needs
template with excess controller variables' FAILED
expected ActionView::TemplateError with message matching /Unknown
parameter.*barfoo/, got #<ActionView::TemplateError:
ActionView::TemplateError (Unknown parameter 'template'.
Views::Test::Needs only accepts foobar, foobar) on line #1 of
app/views/test/needs.html.rb:
1: class Views::Test::Needs < Erector::Widget
2: needs :foobar
3:
4: def content
It seems that at one point you were omitting _template by force, and
then took that out in favor of the ignoring_extra_controller_assigns
setting, which these tests are now not setting. But I'm confused on
where to fix it -- should the tests be setting it themselves, or
should they be calling different controller methods, or should they
actually be expecting this new failure mode?
Andrew, could you see if you're getting these failures and if so give
me a hint on the right way to fix them?
> I'm getting two failures in the Rails suite post-merge, both in
> render_spec:
> 1)
> 'ActionController::Base#render should raise when rendering a widget
> class with implicit assigns and too many variables' FAILED
> expected RuntimeError with message matching /Unknown parameter.*baz/,
> got #<RuntimeError: Unknown parameter 'template'. NeedsWidget only
> accepts foo, bar>
> 2)
> 'ActionController::Base#render should raise if rendering a #needs
> template with excess controller variables' FAILED
> expected ActionView::TemplateError with message matching /Unknown
> parameter.*barfoo/, got #<ActionView::TemplateError:
> ActionView::TemplateError (Unknown parameter 'template'.
> Views::Test::Needs only accepts foobar, foobar) on line #1 of
> app/views/test/needs.html.rb:
> 1: class Views::Test::Needs < Erector::Widget
> 2: needs :foobar
> 3:
> 4: def content
> It seems that at one point you were omitting _template by force, and
> then took that out in favor of the ignoring_extra_controller_assigns
> setting, which these tests are now not setting. But I'm confused on
> where to fix it -- should the tests be setting it themselves, or
> should they be calling different controller methods, or should they
> actually be expecting this new failure mode?
> Andrew, could you see if you're getting these failures and if so give
> me a hint on the right way to fix them?
I don't get these failures, but I'm running at the head of my tree
(6294e5ad1bc468bb4d52050121264e02064767ab). Are you merging my entire
tree, or just individual commits?
I'd make sure you're merging at least these commits:
If you'll let me know which specific commits you're merging, I'd be
happy to give it a shot myself and fix any failures that are
happening. There's enough going on here that it can be a little
tricky, I think...
Bizarre! This isn't changing anything outside the Form class...
I think I'll keep it reverted for now and do a release. There's some
good stuff in here, including a block-related bug fix of mine. 0.7.3,
here we come!
> Bizarre! This isn't changing anything outside the Form class...
> I think I'll keep it reverted for now and do a release. There's some
> good stuff in here, including a block-related bug fix of mine. 0.7.3,
> here we come!
On Tue, Dec 1, 2009 at 7:11 PM, Alex Chaffee <ale...@gmail.com> wrote:
> Now CI is failing, even though it passes on my machine, and with the
> suspicious commit reverted. WTF?
Thanks for the tip. That was fun but didn't really help much, since
the thing happened and also didn't happen on my laptop at different
times. The CI box is on 1.8.6 btw -- shouldn't it be on 1.8.7 by now?
On Tue, Dec 1, 2009 at 6:39 PM, Chad Woolley <thewoolley...@gmail.com> wrote:
> On Tue, Dec 1, 2009 at 7:11 PM, Alex Chaffee <ale...@gmail.com> wrote:
>> Now CI is failing, even though it passes on my machine, and with the
>> suspicious commit reverted. WTF?
I just upgraded to rails 2.3.5... and now the 2 tests are failing for
me locally again. That's good news, right? Andrew, can you try to
'sudo gem update rails' and see what happens?
On Wed, Dec 2, 2009 at 7:00 AM, Chad Woolley <thewoolley...@gmail.com> wrote:
> On Tue, Dec 1, 2009 at 11:05 PM, Alex Chaffee <ale...@gmail.com> wrote:
>> The CI box is on 1.8.6 btw -- shouldn't it be on 1.8.7 by now?
> Probably. I'd have to do some research on what would break, or at
> least maybe bug people to get things green on 1.8.6 first. It's still
> a shared box.
> BTW, getting on Bundler is a great thing to do if you haven't yet...
but this list does not include @real_format or @_request, so they trip
up the 'needs' alert
so I removed them myself from inside Erector's monkey patchy
ActionView::Base#instance_variables_for_widget_assignment_for
Seems like a Rails bug. Anyone feel like submitting a patch?
- A
P.S. Make sure to 'sudo gem update rails' or just run 'sudo
geminstaller' to get Rails 2.3.5 all installed on your system before
doing more erector+rails development.
On Wed, Dec 2, 2009 at 11:21 PM, Alex Chaffee <ale...@gmail.com> wrote:
> I just upgraded to rails 2.3.5... and now the 2 tests are failing for
> me locally again. That's good news, right? Andrew, can you try to
> 'sudo gem update rails' and see what happens?
> On Wed, Dec 2, 2009 at 7:00 AM, Chad Woolley <thewoolley...@gmail.com> wrote:
>> On Tue, Dec 1, 2009 at 11:05 PM, Alex Chaffee <ale...@gmail.com> wrote:
>>> The CI box is on 1.8.6 btw -- shouldn't it be on 1.8.7 by now?
>> Probably. I'd have to do some research on what would break, or at
>> least maybe bug people to get things green on 1.8.6 first. It's still
>> a shared box.
>> BTW, getting on Bundler is a great thing to do if you haven't yet...
Sorry it took me a bit to respond -- my day yesterday was totally
shot, and trying to avoid flooding the list with too much email. Feel
free to IM me (agew...@gmail.com) if anybody wants a quicker ping at
any point.
So, the mystery continues re: specs. On my laptop, using the pivotal/ erector tree, I get two failures (attached as 'spec-failures.txt') at
the head, e352ca9d3a1ce789. I've checked in my patch for these at http://github.com/ageweke/erector/commit/a1c142738b89dbde1ad949c128e5... ; it seems like more missing Rails variables, although I'm at a loss
to explain why I get them and the CI box doesn't. Maybe it's 1.8.6 vs.
1.8.7? I'm on Snow Leopard and thus fixed to 1.8.7 (I'm unaware of a
1.8.6 Ruby that compiles and runs correctly on SL). The patch seems
like it should be safe everywhere, though, so maybe we still want it?
Another issue: 'rake install_dependencies' doesn't work for me, and
never has. I've checked in a patch for this at http://github.com/ageweke/erector/commit/4a2a254be8db6602f0f0d64972f2... ; the comment I added explains what's going on and why. (It's an
unfortunate interaction with the 'git' gem that Jeweler uses.) I'm
really curious why others aren't seeing this -- I've got gems: git =>
1.2.5, technicalpickles-jeweler => 1.2.1.
I will dig into the Rails bug you mention (@real_format, @_request,
and now @template and @request, as well), see what's going on, and
send in a patch if appropriate. We're trying to put a tighter
interface between Rails and the views than ERb uses, so they have no
tests that would cover this.
I've also merged the head of pivotal/erector into ageweke/erector, so
I believe we should be in sync completely. Onwards!
On Thu, Dec 3, 2009 at 2:18 PM, Andrew Geweke <agew...@gmail.com> wrote:
> Sorry it took me a bit to respond -- my day yesterday was totally shot, and
> trying to avoid flooding the list with too much email. Feel free to IM me
> (agew...@gmail.com) if anybody wants a quicker ping at any point.
> So, the mystery continues re: specs. On my laptop, using the pivotal/erector
> tree, I get two failures (attached as 'spec-failures.txt') at the head,
> e352ca9d3a1ce789. I've checked in my patch for these at
> http://github.com/ageweke/erector/commit/a1c142738b89dbde1ad949c128e5... > it seems like more missing Rails variables, although I'm at a loss to
> explain why I get them and the CI box doesn't. Maybe it's 1.8.6 vs. 1.8.7?
> I'm on Snow Leopard and thus fixed to 1.8.7 (I'm unaware of a 1.8.6 Ruby
> that compiles and runs correctly on SL). The patch seems like it should be
> safe everywhere, though, so maybe we still want it?
> Another issue: 'rake install_dependencies' doesn't work for me, and never
> has. I've checked in a patch for this at
> http://github.com/ageweke/erector/commit/4a2a254be8db6602f0f0d64972f2... > the comment I added explains what's going on and why. (It's an unfortunate
> interaction with the 'git' gem that Jeweler uses.) I'm really curious why
> others aren't seeing this -- I've got gems: git => 1.2.5,
> technicalpickles-jeweler => 1.2.1.
> I will dig into the Rails bug you mention (@real_format, @_request, and now
> @template and @request, as well), see what's going on, and send in a patch
> if appropriate. We're trying to put a tighter interface between Rails and
> the views than ERb uses, so they have no tests that would cover this.
> I've also merged the head of pivotal/erector into ageweke/erector, so I
> believe we should be in sync completely. Onwards!
> Andrew
> On Dec 3, 2009, at 7:30 AM, Alex Chaffee wrote:
>> Okay, I worked it out.
>> ActionController::Base defines the following
>> (action_pack/lib/action_controller/base.rb line 256)
