Rubies in the Rough

Let's Patch Rails

In celebration of my first ever trip to RailsConf next week I wanted to be a good open source citizen and contribute a patch.

I'm giving a presentation at the conference on various random features that the framework provides. I selected features from many places like blog posts, books, and some that I just remembered from years of working with the software.

One of the features I decided to show was an old feature that I never see anyone use. It turns out that there's a good reason for that. When I tried it on a modern Rails, it didn't work anymore. Rails has undergone some changes under the hood and this feature was likely removed in that process. I figured out a workaround so I could still show it in my presentation, but it would be nice if I contributed the fix back to Rails so others could use it.

In this article, I will walk through the entire process of doing that.

What's Missing?

ERb has always had an under appreciated syntax tweak. Everyone knows you can write code like this:

$ erb
<% %w[This is ERb].each do |w| %>
* <%= w %>
<% end %>
^D
* This

* is

* ERb

If you pass the proper flag though, ERb can also understand this shorthand syntax:

$ erb -T -
% %w[This is ERb].each do |w|
* <%= w %>
% end
* This
* is
* ERb

As you can see, just starting a line with % is enough to put the entire line in Ruby mode (with the result not inserted into the document). This makes for very clean iterators and if statements, in my opinion.

Rails supported this syntax in the past. You could set config.action_view.erb_trim_mode to a String containing a % to enable it. Rails still supports the trim mode setting, but now it only supports the - mode for trimming whitespace around ERb tags. (Confusingly, the - argument to the command-line program I used above does both % and -, but ERb doesn't behave that way internally.)

I believe the main reason this feature got dropped is that Rails has switched over to using Erubis on the backend. Erubis is ERb compatible, but it looks like the Rails team only managed to support the - trim mode in the change and dropped the support for %.

That's what I intend to restore.

Getting a Working Install

I have contributed to Rails some in the past, but that was years ago now. Because it has been so long, I don't even have a checkout of the code anymore. That seems like a good place to start.

I know where Rails lives on GitHub, but if I didn't I would have tried to find it by Googling Rails GitHub. I'll begin by visiting the Rails repository, forking a copy of my own to mess around with, grabbing the URL for my copy, and cloning it locally. That last step amounts to running this command:

$ git clone git@github.com:JEG2/rails.git
Cloning into rails...
remote: Counting objects: 296204, done.
remote: Compressing objects: 100% (74450/74450), done.
hooks/post-upload-pack:5:in `require': no such file to load --
        /data/repositories/3/nw/39/14/config/basic (LoadError)
        from hooks/post-upload-pack:5
remote: Total 296204 (delta 230167), reused 283494 (delta 218965)
Receiving objects: 100% (296204/296204), 40.50 MiB | 1.75 MiB/s, done.
Resolving deltas: 100% (230167/230167), done.

I seem to have a copy now, but notice that there was an error reported in the process.

Sidetracked

I debated whether or not I should let this distract me from my goal, but it does seem worth a quick look since it seems to affect cloning the project and a lot of programmers do that. My goal is to be good citizen and fixing broken windows is a great way to do that.

So I peaked inside of that hook file… well, I tried. I assumed the hook would be in the .git/hooks directory, but it wasn't:

$ cat .git/hooks/post-upload-pack
cat: .git/hooks/post-upload-pack: No such file or directory

When it wasn't where I expected it to be, I Googled for the error: from hooks/post-upload-pack. The first link showed me that others have this issue with non-Rails repositories. The second link confirmed my suspicion that this isn't related to Rails. It's actually a GitHub issue in this case.

Good to know. I'll let that go and get back to patching now.

Getting it to Run

Now that I have the Rails source, I need to get it setup for development. Many projects include some information about how to contribute, often in their README, and that usually gives you some steps for getting started. Sure enough, the Rails README did have a section for that and it actually linked me to a detailed guide on how to contribute.

Though it takes a little time, you really should read through this content. It sucks to dump a bunch of effort into a patch only to have it rejected because you didn't do it the way the maintainers for that project like things done. That usually ends up wasting even more time than just reading the documentation.

So I read it. It reminded me to search for my issue in their tracker, to make sure it wasn't there already. I tried searches for erb_trim_mode and just erb but none of the results seemed relevant.

Next, the guide told me to get the test suite running. That sounded like a great idea to me. I started working through those steps.

I already had Git installed and I had checked out the Rails source, so I skipped over those steps.

The next step talks about installing some dependencies, but only gave instructions for an operating system I don't use. I thought I had some of them anyway, maybe even all though I wasn't sure. Given that, I decided to just make sure I had the latest version of Bundler, as the guide recommends, bundle, and try the tests to see what happens:

$ gem install bundler
Successfully installed bundler-1.1.3
1 gem installed
Installing ri documentation for bundler-1.1.3...
Building YARD (yri) index for bundler-1.1.3...
Installing RDoc documentation for bundler-1.1.3...
$ gem update bundler
Updating installed gems
Nothing to update
$ bundle install --without db
…
Your bundle is complete! Use `bundle show [gemname]` to see
where a bundled gem is installed.
…
$ bundle exec rake test
…

That last command dumped a metric ton of output. I saw a lot of tests flow by, some errors, and plenty of warnings. I had to decide how I should handle this.

One option would be to start addressing these issues until I can cleanly run the entire test suite. That could take quite a bit of time though. The truth is that I only need to touch one small part of Rails and I know that part isn't the database (a complex portion of the Rails test suite that gets its own section in the guide). Because of that, I decide to focus in on the subsection of Rails I want to manipulate and try to get that section running cleanly.

First, I need to find the right spot. I searched through the code for the broken setting:

$ ack erb_trim_mode
actionpack/lib/action_view/base.rb
155:  delegate :erb_trim_mode=, :to => 'ActionView::Template::Handlers::ERB'

actionpack/lib/action_view/template/handlers/erb.rb
44:   class_attribute :erb_trim_mode
45:   self.erb_trim_mode = '-'
82:       :trim => (self.class.erb_trim_mode == "-")

OK, it looks like the code I care about is in ActionPack, so I'll focus in on that. That went better, though still not perfect:

$ cd actionpack/
$ bundle exec rake test
…
… warning: loading in progress, circular require considered harmful - …
…
… warning: method redefined; discarding old blank?
… warning: previous definition of blank? was here
…
Skipping MemCacheStoreTest tests. Start memcached and try again.
…
Finished tests in 22.599275s, 160.8459 tests/s, 723.3418 assertions/s.

  1) Error:
test_parameters(RequestTest):
ArgumentError: wrong number of arguments (1 for 0)
…

3635 tests, 16347 assertions, 0 failures, 1 errors, 0 skips
rake aborted!
…

There are a few things happening here. First, I'm seeing some warnings. I wish they weren't there, but they don't really apply to what I am doing here and I'm going to ignore them for now.

I also noticed that some tests were skipped because I didn't have memcached running. That's fine. I'm not going to touch anything related to that, so skip away.

Finally the test suite does seem to have a single error. If it were several errors I might be concerned that I don't have something setup right, but this is just one error that looks like a Ruby issue nowhere near the code I'm going to work on. I note that it's there, but try not to worry about it for now.

I'll want to check this full suite again before I'm done to make sure I haven't made anything worse, but it would be nice if I could zero in even more on just the part I care about. The guide told me about a feature to run just one directory of tests. It also told me how to silence the warnings. I would like to try combining both of those tricks to see if I can get down to a small clean run of tests I can experiment with.

My previous search showed that the code I need to modify is the ActionView::Template::Handlers::ERB. I didn't match any code in the tests though, so I think we can assume that this feature is probably untested. That would explain how it got removed in the first place. Still, I hope the handler itself is tested and I hunt for that:

$ ls test
abstract              assertions            lib           ts_isolated.rb
abstract_unit.rb      controller            routing
active_record_unit.rb dispatch              template
activerecord          fixtures              tmp

That template directory looks promising. I wanted to see if it was possible to get a clean run of just those tests. Unfortunately, it didn't go as well as I hoped. I was able to silence the warnings, but Rails seemed to ignore the TEST_DIR feature mentioned in the guide.

I poked around some more. I couldn't find any tests for the specific feature I am interested in. I did find some semi-related tests for ERb functionality in the erb subdirectory. With a little fiddling around I figured out how to run one of those files individually:

$ bundle exec ruby -I test test/template/erb/tag_helper_test.rb
Run options: --seed 6234

# Running tests:

.....

Finished tests in 0.011093s, 450.7347 tests/s, 540.8816 assertions/s.

5 tests, 6 assertions, 0 failures, 0 errors, 0 skips

I plan to add a file in this directory for testing the handlers. I can run just that file as I work to see how things are going, then close with a full run of the of the ActionPack suite to confirm that I haven't made anything worse.

The Boy Scout Rule

I want to add another possible trim mode, but it looks like what's there isn't even tested. That's problematic because I won't know if I break that feature.

Given that, I would like to add a super trivial set of tests just to verify the current behavior. Next, I'll add some tests for the behavior I want and then add the feature.

This means I'll be leaving things in slightly better shape than I found them. There will be a new feature, it will be tested, and the existing feature will gain some coverage.

To me, this is the ideal strategy for approaching any open source contribution. If we all did this, things would always improve with every commit.

I start by adding the new test file I need as test/template/erb/handlers_test.rb and ape a little of the setup code I saw from other files in that directory:

require "abstract_unit"

class HandlersTest < ActiveSupport::TestCase
  extend ActiveSupport::Testing::Declarative

end

Then I added a little code to make it easy to temporarily change the trim mode and to render some content using a handler:

require "abstract_unit"

class HandlersTest < ActiveSupport::TestCase
  HANDLER  = ActionView::Template::Handlers::ERB
  Template = Struct.new(:source)

  extend ActiveSupport::Testing::Declarative

  private

  def with_erb_trim_mode(mode)
    @old_erb_trim_mode    = HANDLER.erb_trim_mode
    HANDLER.erb_trim_mode = mode
    yield
  ensure
    HANDLER.erb_trim_mode = @old_erb_trim_mode
  end

  def render(template)
    eval("output_buffer = nil; " + HANDLER.call(Template.new(template)))
  end
end

The with_erb_trim_mode() helper just sets the mode for me then runs a block of code. It wraps that block in some code to save and restore the old setting. We need to do that because this is effectively a global variable that affects all of Rails. It could break other tests if we left it on the wrong setting.

It took me a bit to puzzle out a proper render() for these handlers. You can see that I had to add a Template just so I could put the ERb content in an expected source() method. I also had to set a variable before the eval() because the template code expects to have that from the context it gets run in. I figured these quirks out by reading the handler source and noodling with the code until I had figured out how to make it happy.

Next I added two simple tests for the existing functionality:

require "abstract_unit"

class HandlersTest < ActiveSupport::TestCase
  # ...
  test "content is not trimmed without a trim mode" do
    with_erb_trim_mode nil do
      assert_equal("   \ntest", render(" <% 'IGNORED' %>  \ntest"))
    end
  end

  test "content around tags is trimmed if the trim mode includes a dash" do
    with_erb_trim_mode '-' do
      assert_equal("test", render(" <% 'IGNORED' %>  \ntest"))
    end
  end

  # ...
end

Those are passing:

$ bundle exec ruby -I test test/template/erb/handlers_test.rb
Run options: --seed 9634

# Running tests:

..

Finished tests in 0.009449s, 211.6626 tests/s, 211.6626 assertions/s.

2 tests, 2 assertions, 0 failures, 0 errors, 0 skips

That gives me a touch more confidence for adding some new code, so I add some more tests for the functionality I want:

require "abstract_unit"

class HandlersTest < ActiveSupport::TestCase
  # ...

  test "percent lines are normal content without a trim mode" do
    with_erb_trim_mode nil do
      assert_equal( "% if false\noops\n% end\n",
                    render("% if false\noops\n% end\n") )
    end
  end

  test "percent lines count as ruby if trim mode includes a percent" do
    with_erb_trim_mode "%" do
      assert_equal("", render("% if false\noops\n% end\n"))
    end
  end

  test "both trim modes can be used at the same time" do
    with_erb_trim_mode "%-" do
      assert_equal( "test", render( "% if false\noops\n% end\n" +
                                    " <% 'IGNORED' %>  \ntest" ) )
    end
  end

  # ...
end

I now have a couple of failing tests to fix:

$ bundle exec ruby -I test test/template/erb/handlers_test.rb
Run options: --seed 15711

# Running tests:

F...F

Finished tests in 0.027155s, 184.1282 tests/s, 184.1282 assertions/s.

  1) Failure:
test_both…(HandlersTest) [test/template/erb/handlers_test.rb:36]:
--- expected
+++ actual
@@ -1 +1,4 @@
-"test"
+"% if false
+oops
+% end
+test"


  2) Failure:
test_percent…(HandlersTest) [test/template/erb/handlers_test.rb:30]:
--- expected
+++ actual
@@ -1 +1,4 @@
-""
+"% if false
+oops
+% end
+"


5 tests, 5 assertions, 2 failures, 0 errors, 0 skips

The Big Finish

The setup and tests were definitely the hard part. The actual code needed is tiny. First, I introduce a new handler:

require 'action_dispatch/http/mime_type'
require 'active_support/core_ext/class/attribute'
require 'erubis'

module ActionView
  class Template
    module Handlers
      class Erubis < ::Erubis::Eruby
        # ...
      end

      class ErubisWithPercentLine < Erubis
        include ::Erubis::PercentLineEnhancer
      end

      # ...
    end
  end
end

The old ERb code took its setting from a parameter to the constructor, but Erubis tells you to mix in a module that handles this feature. To do that, I just subclass the existing handler and add the module. Now we can have it either way, depending on which handler we delegate to.

Next I edit a small chunk of implementation code for the ERB handler to take it from this:

require 'action_dispatch/http/mime_type'
require 'active_support/core_ext/class/attribute'
require 'erubis'

module ActionView
  class Template
    module Handlers
      # ...

      class ERB
        # ...

        def call(template)
          # ...

          self.class.erb_implementation.new(
            erb,
            :trim => (self.class.erb_trim_mode == "-")
          ).src
        end

        # ...
      end
    end
  end
end

to:

require 'action_dispatch/http/mime_type'
require 'active_support/core_ext/class/attribute'
require 'erubis'

module ActionView
  class Template
    module Handlers
      # ...

      class ERB
        # ...

        def call(template)
          # ...

          mode           = self.class.erb_trim_mode.to_s
          implementation = self.class.erb_implementation
          if mode.include?("%") && implementation == Erubis
            implementation = ErubisWithPercentLine
          end
          implementation.new(erb, :trim => mode.include?("-")).src
        end

        # ...
      end
    end
  end
end

Really all I do here is to trade out the handler if it is set to the normal Erubis and the trim mode includes %. In that case I switch to the handler that includes the extra behavior.

I was a little worried I might be slowing down the Rails rendering pipeline with this change. That would be bad, because we know it's rare and I would hate to impact the common case to support it. However, I checked around a bit for the code that invokes this handler and it turns out that templates are only compiled once, under normal usage. That's perfect. Even if I did add a tiny penalty, we won't be paying it with every render. That's what I was worried about.

I rerun my tests to ensure that change does the trick, then rerun the full ActionPack suite to verify that there's still just one failure. That's exactly how it goes, so the feature is restored.

The Polish

I assume this feature is never used because no one knows it exists. To increase the chances of people finding it I decide to add a CHANGELOG entry:

## Rails 4.0.0 (unreleased) ##

*   Restored support for the "%" ERb/Erubis _trim mode_.  This can be
    activated with:

        config.action_view.erb_trim_mode = "%"  # or "%-" whitespace trim

    With that mode active, you can use a single percent sign at the
    beginning of a line to engage Ruby mode (without inserting results).
    It allows for template code like this:

        % if current_user.try(:admin?)
          <%= render "edit_links" %>
        % end

    *James Edward Gray II*

There's other documentation that should be updated too. For example, I noticed that one of the Rails guides mentions this feature but gives the wrong default for it and claims that it supports a lot more modes than it does. I'll wait to see if the core team accepts my patch, then offer to fix that documentation to bring it inline.

Take note that most of my work on this patch was not about adding the feature. I invested a lot of time in the tests, I worried about whether or not I was slowing Rails down, and I took time to add some documentation. A patch is more politics than code and I'm trying to make it easy for the core team to say, "Yes."

Sending it Up

The work is done, so it's time to push it up to GitHub and send a pull request. The first part is a few simple commands:

$ git add --all
$ git commit -m "Restoring the '%' trim mode for ERb templates, …"
[master c734294] Restoring…
 3 files changed, 76 insertions(+), 4 deletions(-)
 create mode 100644 actionpack/test/template/erb/handlers_test.rb
$ git push origin master
Counting objects: 24, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (13/13), done.
Writing objects: 100% (13/13), 2.00 KiB, done.
Total 13 (delta 10), reused 0 (delta 0)
To git@github.com:JEG2/rails.git
   99eae3f..c734294  master -> master

I went to GitHub and filled out the pull request. Now I'll just have to wait and see if it gets accepted…

Get Involved

We should all remember to do our part. You can, and should, help out. Here are some ideas for things you could do:

• Try my patch. Whip up a little Rails application, point it at my fork and try the new feature. If it works for you, go add a comment saying so to my pull request. This encourages the Rails team to accept the patch.
• Work on those warnings in the tests. You know I love running with warnings mode on, so it really bugs me that Rails doesn't run clean in that mode. These are usually quite easy to fix too. The message generally sends you to the exact line and you usually just need to add some parentheses, initialize a variable, or switch away from a deprecated method. If you make any pull request fixing these, drop me an email. I'm happy to advocate for you.
• If you are feeling adventurous and want to be a real hero, take a look at the open Rails issues and see if you can help out with any of them. There's usually some in there that aren't too much trouble if you dig a little and it's an excellent way to get comfortable with the Rails source.