[PATCH] Per-Request Multiple Controller View Paths

The Test

The Fix

I would welcome this. +1.

I have published the plugin that makes use of this patch at http://code.google.com/p/rails-multisite/ It works without the patch, but it doesn't really fulfill it's potential.

I would love to see this functionality adopted++

I've jotted down my use-case here: http://homes.msi.ucsb.edu/~cburt/ecosystem_dashboard/themes/2754

The #view_paths method seems unnecessary since #initialize takes a view_paths parameter. Also, ActionMailer doesn't have view_paths so this change would probably break it. I did things a little differently after seeing Julik's ticket on the same issue. Reopen the ticket if this doesn't work for you, but the test passes :)

(In [7034]) Make ActionView#view_paths an attr_accessor for real this time. Also, don't perform an unnecessary #compact on the @view_paths array in #initialize. Closes #8582 [dasil003, julik, rick]

Also, I too worked on a CMS in rails several years ago and ran into this same problem. The view_paths stuff is a huge step, but there's another wacky problem I ran into: http://weblog.techno-weenie.net/2005/11/4/more_on_per_request_template_roots_in_actionpack. Note my apparent lack of regard for what the core team thought about it. I just hacked my app (this was pre-plugins probably). Ha.

Replying to technoweenie:

Hmmm, thanks for the heads up on that. I took a look and the method looks like much the same in edge rails as it did 18 months ago when you posted that snippet (the line # is significantly different though :)

I will add a similar fix to my multisite plugin.

Actually this isn't fixed at all. The inheritance support of this feature is what was causing the breakage. Specifically, if a given controller doesn't have view_paths, it dup.freezes the superclasses view_paths. This duplication is what was causing ActionController::Base to have a separate copy of view_paths.

Of course inheritance will break if we remove the duplication, and since @template (ie. ActionView) is initialized early, the only viable fix it seems, is to provide late-loading/updating of the view paths in the case where a controller exists. This is similar to my previous patch, but this time much cleaner.

The original test passed because it wasn't inheriting from ActionController::Base.view_paths... it had its own explicitly provided in setup.

Attached are a patch for the test, and my improved patch to finally fix the problem.

I just don't like that solution, it seems a bit crufty to pass the @view_paths to the template and never use it. ActionPack is begging for for some refactoring love.

Yeah. Personally I don't even know why the view_paths is passed at all? I didn't want to mess with it because it looks like it's for cases where a controller is not passed (controller = nil by default), what exactly is that situation?

Likewise, what's up with [*view_paths].compact?

Why would there be nil in there? And if so, why would it be a problem? And also what is the purpose of [*view_paths] to begin with? Isn't that the same as view_paths.dup? And why should it be dup'ed anyway?

Unfortunately these are all questions beyond my familiarity with rails core. My patch was designed mostly just not to break things, and honestly I am using it in production, so it's pretty urgent to me. I'd love to help with a refactoring, but I think that requires someone with more familiarity than me.

I have uploaded a new version of this patch with && @controller.respond_to?(:view_paths)

I noticed breakage occurring with ActionMailers (specifically ExceptionNotifier) because they are passed in the controller parameter, but they do not have view_paths (maybe they should?).

Rick, I'm really interested in getting this working. Do you think there's any chance of getting this patch accepted even if it's a bit crufty? The passed-in view_paths seem to be for the use of ActionMailer, and refactoring that whole interaction seems way over my head. I think adding a single-line method doesn't really make things much worse than they are now, so I'm in favor of accepting this patch.

However if you really don't want to accept it, I think the better solution might be to revisit John Long's original semantics. The whole issue is the view_paths.dup.freeze. This is necessary for the inheritance test to pass, but I'm not totally sure on what his intentions were. For instance, if you want to set up search paths on a subclass of ActionController::Base then you can't append the paths anyway, you have to set them explicitly.

If it could somehow be done without the freeze, then patching ActionView wouldn't be necessary. However I'm sure I'm missing something as to why the dup'ed copy needs to be frozen. There may be a way to preserve his intentions without relying on the freeze.

Just wanted to post an update here for anyone following this patch. I'm using it in production and it works, but Rick doesn't like the look of it, and I can do better. The issue is to refactor John's original patch so it's not duping the array paths. I believe I can do so in such a way that preserves the spirit of what John was trying to accomplish but without requiring any changes to ActionView. I contact John last month about this, but never received any feedback. I'll give it my best shot and hopefully have something going in the next few weeks.

I know this might get labeled as herecy, but...

a) we know that class_inheritables lead to unexpected things. Depends on the weather. Particularly because their behavior is ambiguous (how much overrides can an array take preserving all of the upstream values?) b) we know that when overrides need to happen it's better to let the overrides happen at the proper sanctioned place (that's about what OOP is for. The problem that John was trying to solve is solvable, albeit at a much smaller cost IMO if you drop the class_inheritable_metamagic_programmablemation going on there.

Besides - why oh why would action controller need to store view paths in a classvar per s.e. if we can simply make it an instance method (all problems with inheritance are then resolved by wrapping a call to super as "resort to" paths). Do we need any template paths without instantiating the controller at all (who is going to render them?)

So the question is: are the classvars worth the hassle?

Just jotting down:

class ActionController::Base
   
   def view_paths
    # deduce view_paths based on my name
   end
end

class ApplicationController < Base
  before_filter do | c |
      # authentication fails here
      render :action => 'login_failed' # this will need to lookup a template
  end
end

class Specific < ApplicationController
   def view_paths
       # if @site does not exist here it's not something Rails should solve for The Guy 
       # if The Guy renders from a pre_filter without having his variables in place
      super + [@site.theme_path]
   end
end

Alternatively you can make view_paths something that gets called as the first before_filter, no exceptions (but with the request environment already in place). In short - dump the classvars.

alright... and now, for the most dreadful hacky workaround around town (fictitious example based on working code)

class ApplicationController < ActionController::Base
  before_filter :find_site
  around_filter :set_current_site_class_var_and_go_directly_to_hell
  layout :site_layout

  protected
    def find_site
      @site = Site.find_by_subdomain(request.subdomains.first)
    end
    def site_layout
      @site.layout_name
    end    
    def set_current_site_class_var_and_go_directly_to_hell
      @@current_site = @site
      yield
    ensure
      @@current_site = nil
    end
    # Override ActionController::Base::controller_path
    def self.controller_path
      "#{@@current_site.view_sub_directory}/#{super}"
    end
end

this view_paths hash class var business looks like some premature performance optimisation... definitely in need of a refactoring.

It wasn't premature. It brings the amount of file accesses way down for a single request and sped things up.

I haven't been able to get ahold of John Long, so I'm reluctant to massively refactor without understanding the whole story here. Basically my whole problem stems from the fact that ActionView is getting a COPY of the view_paths instead of the original view_paths.

I understand that having an inheritable view_paths makes it nice for plugins so they can modify the view_paths only for a single controller. I support that functionality, but the problem with the current implementation duplicates the original view_paths when I believe it should simply refer to it. The best I can figure, John was trying to prevent the scenario that modifying a specific controller's view_paths affects all the other controllers indirectly... that would indeed be confusing. But in my case I have the opposite problem, which is that I only want to have one view_paths apply to all my controllers, and it's impossible to achieve because by the time we reach the request-serving stage, ActionController::Base.view_paths refers to an array that is simply not used at all by ActionView.

So the question is how do we reconcile the need for per-request view_paths and per-controller view_paths? From my perspective the first step is removing the dup.freeze and pass the actual array to ActionView... that much is necessary to remove the need for the hacky patch I originally created. But then we run into some hairy issues that it seems like we can't have both ways. For instance, if we want to modify the base view_paths per-request, then we can't duplicate it because the changes won't percolate down automatically. But if we don't duplicate than we can't have per-controller view_paths.

In order for per-request and per-controller view_paths to be reconciled, it seems like the per-controller view_paths would have to inject the base view_paths every request. What would be good semantics for that? Whatever way you slice it seems some complexity will have to be added, but I think it's absolutely necessary, because the whole multiple controller view_path concept is castrated if it can't be used on a per-request basis. The current implementation is useful for separation of concerns for plugins (John's interest), but useless for productization (which was a big part of the rationale for accepting the original patch, at least according to the announcement in the official blog early this year).

Anyone have any bright ideas?

2 technoweenie: sure (globbing is expensive) but can't we use memoization for that? 2 dasil003: see above.

Here is the example of how request-based root paths can be detected (but it does that orthogonally to the core Rails):

http://julik.textdriven.com/svn/tools/rails_plugins/themer/

It works both with edge rails and gem rails.

The approach I took for this one is to use a "template finder" object and make this object responsible for finding views. The biggest problem with that is essentially munging the passed paths in such a way that ActionView agrees to accept and resolve them properly. What if we shift the responsibility for lookups (say everything before the template extension comes into the picture) into a lookup object, which will cache lookups but will be overridable? Even with a simplistic cache hash as in Themer you can get away with globbing to the minimum.

The lookup object then can be substituted by the user for optimum lookup strategy (you will only need "mailer_path" to complete the picture, I haven't got that yet).

We also know that, uhm, render :inline is not what someome would want when implementing templates in the database. Having a thin layer of indirection instead would allow for more specific lookups anywhere, also in the database, with a file being fed to ActionView when it's loaded.

Latest patch idea: http://pastie.caboo.se/103921.txt

See discussion in rails-core.

This is no longer really relevant to me. I've fixed the issue for Rails 2.0.x, and Rails 2.1 has the TemplateFinder object as suggested by Julik (http://dev.rubyonrails.org/ticket/10800)