Add test case for issue #24

[ajb]

This is a test case for #24, but the tests are passing where they should be failing.

[ageweke]

First off: thank you for being awesome and figuring out how to contribute a test in the absence of docs. I really appreciate it!

Complete coincidence — I was just looking at this in the last couple of days. (I have a test case that looks extremely similar to this.) I have the same problem: I'm unable to reproduce the issue. It looks like it “all works”. If anybody can figure out how to reproduce the issue, please let me know. I absolutely will fix it if I can reproduce it…but I just can’t seem to make it happen right now, no matter what I do.

I’m still thinking deeply about the YieldedObjectOutputter. I think it needs an overhaul, and I think I’m starting to figure out how to do it. The fundamental issue is that some helpers (e.g., link_to) semantically “should” output in Fortitude (you want to write link_to …, not rawtext(link_to …)), while others semantically shouldn’t (writing number_with_delimiter shouldn’t output, just return the string), but there’s zero programmatic difference between them. So there will always be some custom code required to specify which should be which. In Fortitude’s core, for Rails, this is this file.

I’m thinking of first making YieldedObjectOutputter more robust underneath, so it can easily know where it is in a stack of yielded blocks, and decide what to output and what not to. Then, creating some syntactic sugar so it’s really easy to define new complex helpers (like form_for, fields_for, and so on) that do the right thing.

My goal here is that when people want to use something like simple_form_for, it’s very easy to tell Fortitude what should be output and what shouldn’t — and allow the building of either a very general fortitude-contrib or tie-specific gem like fortitude-simple_form_for or whatever that shares this little piece of code. I don’t think it makes sense to put integration code like that for a gazillion third-party gems into Fortitude’s core, but I can do a much better job of making it really easy to create and share that code.

[ajb]

With regard to figuring out how to write a test: my pleasure!

I've also pinged Karl and Harlan in #24, so hopefully they can chime in and we can figure out if there's actually a bug here. I know that there's a reason I used the monkeypatch originally... but it might have had to do with simple_form.

Anyway, I've been thinking about some of the same stuff -- I was going to ask you about your preferred pattern for creating a compatibility gem like fortitude-simple_form.

I agree that the YieldedObjectOutputter is probably one of the more... perplexing parts of Fortitude, but that said, it's already an improvement over Erector, and I think some more documentation could go a long way towards making it less opaque. 😄

[karlhe]

Memory isn't too good on this one, I'll try and see if I can reproduce it again.

In the meantime, I did notice that in my code the only time this situation pops up is with a hash argument, e.g. label :for => :foo do ...

[ajb]

Weird, so the build failed on CI.

[karlhe]

So I finally got my environment working (turns out it's because I didn't have nodejs installed...), and your test does fail for me, here's a snippet:

   -/oop_rails_server_base_template/i
   +"{\"exception\":{\"class\":\"ArgumentError\",\"message\":\"wrong number of arguments (0 for 1)\",\"backtrace\":[\"/vagrant/lib/fortitude/rails/yielded_object_outputter.rb:9:in `block in wrap_block_as_needed'\",\"/var/lib/gems/1.9.1/gems/actionpack-4.0.13/lib/action_view/helpers/capture_helper.rb:38:in `block in capture'\",\"/var/lib/gems/1.9.1/gems/actionpack-4.0.13/lib/action_view/helpers/capture_helper.rb:200:in `with_output_buffer'\",\"/var/lib/gems/1.9.1/gems/actionpack-4.0.13/lib/action_view/helpers/capture_helper.rb:38:in `capture'\",

Which I think is exactly what we would expect?

If it helps, I set up a Vagrant box with the following config to test it:

Vagrant.configure("2") do |config|

  config.vm.box = "ubuntu/trusty64"

  config.vm.provider :virtualbox do |vb|
    vb.customize ["modifyvm", :id, "--natdnshostresolver1", "on"]
    vb.customize ["modifyvm", :id, "--memory", 4096]
    vb.customize ["modifyvm", :id, "--cpus", 4]
  end

  config.vm.provision "ruby", type: "shell" do |s|
    s.inline = <<-SCRIPT
      sudo apt-get -y install git libsqlite3-dev nodejs ruby-dev make
      su - vagrant -c "sudo gem install bundler"
    SCRIPT
  end
end

[ajb]

It looks like the failures are conditional on the version of Rails. I can give this another shot when I get a chance...

[ageweke]

Finally getting back to this. Notes to myself:

The problem is triggered by code like this:

    form_for :person do |f|
      f.label(:name) do
        text 'Foo'
      end
    end

When Fortitude sees the call to form_for, it properly wraps the f being returned in an instance of Fortitude::Rails::YieldedObjectOutputter. The job of the YOO is to intercept calls like f.label, which normally just return text (i.e., in ERb, you say <%= f.label(:name) … %>, not just <% f.label(:name) … %>, and instead cause them to output text instead. And this works just fine.

The issue is the inner block. There is a bug in Fortitude without this patch that causes it to fail when trying to wrap the inner block — it’s expecting blocks to always have at least one argument, and that fails, which this PR fixes. But the much more important bug is deeper: when we call that inner block, text 'Foo' immediately outputs text. But this is wrong, because we’re still capturing the return value of f.label, and will output it only after that call is done. So we end up with Foo<label for="person_name"></label>, which, of course, is completely incorrect. What we need is <label for="person_name">Foo</label>".

What needs to happen is that the inner call to text doesn’t immediately output text, but, instead, appends it to whatever buffer f.label is going to return, so that it gets properly nested. I haven’t yet figured out how to do that, but that’s what needs to happen. I’ll tackle that as soon as I get a little more time…

[ageweke]

So, after further digging — there were two issues here:

1. The YieldedObjectOutputter assumed that all methods that took blocks took at least one argument. This is not, in fact, true (and #label is a great example). I’ve fixed that.
2. Text from the label block showing up outside the label. This is actually a Rails bug in 3.0/3.1. A vanilla Rails install of either of these versions, without Fortitude even installed at all, when given the following code:

<%= form_for :person do |f| %>
  <%= f.label :name do %>
    Foo
  <% end %>
<% end %>

…ends up emitting something like this:

<form accept-charset="UTF-8" action="/foo/bar" method="post">

     Foo
<label for="person_name">

     Foo
</label></form>

…which is clearly wrong.

Yes, this is wrong in a different way than Fortitude’s output is wrong, and that might make me suspect Fortitude. But (a) Fortitude isn’t doing anything particularly weird under the covers here, and (b) both Fortitude and ERb are broken on Rails 3.0/3.1, while (c) both Fortitude and ERb work perfectly on Rails 3.2 and above. As such, I don’t think there’s anything to fix in Fortitude here. I’ve added this example to the spec suite, with a skip for Rails 3.0/3.1.

The fix for the first issue outlined here is in 0.9.5, which I just released.