Rails5 Support

[vlymar]

Adds rails 5 to fortitude's testing matrix, fixes specs that break with rails 5.

To run the specs on your machine:
FORTITUDE_SPECS_RAILS_VERSION=4.2.5.1 bundle exec rspec spec/
FORTITUDE_SPECS_RAILS_VERSION=5.0.0.1 bundle exec rspec spec/
Note: we've forked the oop_rails_server gem with some minor changes, so you may need to re-bundle to install it.

See dobtco/oop_rails_server#1 for oop_rails_server PR

Rails 5 Changes that affect fortitude (or its specs):

• autoloading is disabled by default after the initialization phase in production
  Rails falls back on non-thread-safe autoloading even when eager_load is true rails/rails#13142
• implicit render behavior for actions with no templates defined is to return a 204 No Content
  Lock down new ImplicitRender behavior for 5.0 RC rails/rails#23827
• exceptions without handlers are propagated up slightly differently. fortitude raises a number of custom exceptions during the render stage, which rails then wraps in an ActionView::Template::Error. In rails 4, as the exception propagated up, the fortitude original_exception was then extracted and propagated. It is no longer extracted, so we get the ActionView::Template::Error wrapping the fortitude error.
  rails/rails@e35b98e#diff-81f758fb88f738604ba750f9dde1f6bbL9

TODO:

• How should we approach the error handling change?
  a) ignore it - An error is raise regardless, and the original error message is contained within it. oop_rails_server already implements a handler in rescue_from, so we can unwrap the exception there and leave existing error checking specs as is. The disadvantage here is if someone is already explicitly handling/rescuing from fortitude specific errors anywhere, they will now suddenly get Template::Errors instead.
  b) prevent rails from wrapping fortitude's exceptions somehow. I have an idea on how to do this, will update this PR in a few minutes if successful.
• Why does fortitude rely on autoloading, and can that requirement be removed? Otherwise we need to re-enable autoloading in production
• alias_method_chain has been deprecated

[ajb]

@vlymar, hope you don't mind -- I added dobtco/oop_rails_server as a GitHub dependency in the Gemfile, so your install instructions are no longer needed :)

[vlymar]

Here's the plan for handling exceptions:

1. oop_rails_server has a rescue_from handler that unwraps ActionView::Template::Errors, so the specs will continue to test for the specific fortitude exception.
2. We will update the README with notes for rails 5 users. The notes will mention that if you were explicitly handling Fortitude::Error::* exceptions before, you will not have to handle ActionView::Template::Error. the original exception can easily be obtained by calling err.cause
3. We'l comment the specs that test for specific exceptions with a note that explains that the server would have returned a Template::Error if not for the rescue_from handler unwrapping the exception

[vlymar]

im guessing this spec is testing that rails/fortitude doesn't autoload and use a widget defined outside of app/views, but im not 100% sure. Will investigate this along with the rest of the autoloading questions

[vlymar]

yeah, that seems to be it. the controller action for this spec loads in a rails file that defines Views::ClassLoadingSystemSpec::WidgetDefinedOutsideAppViews. This spec is testing to make sure that implicit rendering does not render that widget, since demands that all templates are under app/views

[ajb]

I was thinking about this question:

Why does fortitude rely on autoloading, and can that requirement be removed?

Do you have any theories about the answer? To me, I'm just seeing one failing spec when commenting-out ::Rails.application.config.enable_dependency_loading = true.

I'm almost more comfortable removing the spec instead of continuing to enforce autoloading.

[vlymar]

re. the spec that fails if we disable autoloading: i think in the absence of any new info, we can disable it. I'm just made uncomfortable by the fact that I don't understand why it is there. That spec is basically testing that if you have an empty directory like views/some_namespace/some_other_namespace/, you'l be able to reference the constant Views::SomeNamespace::SomeOtherNamespace without a nameerror. You're able to reference it because fortitude overrides const_missing to look for matching file paths under views/, and if the path exists (even if theres not a ruby file at the end of it), it'l generate the modules for you. This is useful when there is indeed a widget at the end of the path, because it lets you write Views::SomeNamespace::MyWidget without having had to explicitly define the Views and SomeNamespace modules, but, I don't get why someone would want to reference constants that didn't correspond with actual widgets.

All the actual real widgets get eager loaded, so before initialization finishes they already have constants defined.

[vlymar]

Important note about this PR:

I've replaced alias_method_chain with prepend, but this makes fortitude not backwards compatible with ruby < 2.0. Awaiting ageweke's input on the matter.

It would be possible to preserve backwards compatibility and remove the deprecation warning by unpacking alias_method_chain into the two alias_methods that make it up, or by doing some extra conditions by rails version to include if rails < 5, prepend otherwise, but both of these are ugly and problematic in their own ways.

[karlhe]

Ruby 1.9 is already EOL, and JRuby 1.7.x (1.9 equivalent) will follow shortly at the end of this year: jruby/jruby#4112

IMO trying to maintain 1.9 support is somewhat of a waste of effort, especially since Rails 5 doesn't even support 1.9, and the current version of Fortitude works fine for Rails 4.2.

[ageweke]

This has now been fixed in 0.9.5, which I just released. It’s completely compatible with all Rails versions from 3.0 through 5.0.

While I didn’t end up merging this PR exactly, I did take a look at many of the fixes from this branch as models — thank you so much, @vlymar, for the PR!