This project is archived and is in readonly mode.
template lookup should search controller inheritance chain
Reported by Caio Chassot | September 1st, 2008 @ 07:23 AM | in 3.1
(Josh, I briefly mentioned this to you via email.)
Here's the gist of the feature:
Say you have:
DocumentsController < ApplicationController ArticlesController < DocumentsController
and a view file at:
app/views/documents/list.html.erb
If you request articles/list, and app/views/articles/list.html.erb doesn't exist, rails should render app/views/documents/list.html.erb for you.
Here's my plugin that implements this behavior:
http://github.com/kch/inheritabl...
I'm opening this case to consider making this behavior default in rails.
If you would please experiment with the plugin and let me know if the core is interested, I'd be happy to convert the plugin into a patch.
Comments and changes to this ticket
-
Ahmed Adam September 4th, 2008 @ 05:53 AM
+1 I tried this on 2.1 and it works as expected. It's a very useful feature for people using inherited controllers.
-
josh September 7th, 2008 @ 03:59 PM
- Milestone cleared.
- State changed from “new” to “open”
You can you use git-format-patch to make me a diff file, please.
I'm also curious how this applies to partial rendering. I ran into this problem a few times.
# admin/posts/index.html.erb render :partial => "post" # posts/_post.html.erb Hello -
Caio Chassot September 8th, 2008 @ 03:13 AM
The plugin handles partials as well.
I'll work on a patch now.
-
Caio Chassot September 8th, 2008 @ 04:11 AM
Running into some issues with actionmailer tests. Which reminds me, should I implement inheritable templates for actionmailer too?
In the name of consistency, it seems to make sense, but on the other hand, I feel like it'll never be used. So, for now, I'm skipping it, but if there's interest, post here.
-
Caio Chassot September 8th, 2008 @ 04:20 AM
- Tag changed from “actionpack, edge, enhancement” to “actionpack, edge, enhancement, patch”
Here's a patch.
Only handling actionpack, no actionmailer.
Passes all tests, no new tests added.
I think such a feature would deserve at least a test for the controller action and partial template lookup. Would appreciate some pointers on where it's best to add them.
-
josh September 8th, 2008 @ 03:23 PM
I shouldn't have to touch ActionView, since it only affects the controller's default template. Maybe it's missing a hook?
-
josh September 8th, 2008 @ 03:46 PM
I'm starting to think this would be better off as a plugin. However, it is sucky that you have to override the pick partial stuff. We should fix this so your plugin can hook in easier.
-
Caio Chassot September 8th, 2008 @ 06:24 PM
Template finding is quite spaghettied into the various rails components. Controllers find action templates one way, then views find partials another, and actionmailer works differently too.
Maybe this is something that could be abstracted to a template finder module?
As for the hooks, we used to have them. If you check out the tags in my plugin repository, you can see that for previous versions of rails I had to overwrite less internal stuff. Was never an ideal situation though.
-
Caio Chassot September 8th, 2008 @ 06:26 PM
On having to patch actionview, if you just patch actionview the way I did, and have it ask the controller for the template, then the plugin wouldn't need to orverride _pick_partial_template.
You don't need to call find_inherited_template, you could simply call default_template_name, and the plugin could override it to use find_inherited_template, as it does now.
However, all this feels pretty flaky. I'd prefer to have the whole thing committed to the core. Seems to make sense that templates would be inherited.
I'll try to work on a more robust patch.
-
josh September 8th, 2008 @ 11:25 PM
- Milestone set to “2.x”
:) I totally agree. If you could refactor some of that finder code i'd be more convinced to let it into core. I'm just a bit worried about adding the logic to default_template_name (where it should be) and to pick_template (where it shouldn't). I could see myself auditing pick_template sometime in the future and wondering why this code is there.
-
josh September 8th, 2008 @ 11:26 PM
- State changed from “open” to “wontfix”
Temporary closing the ticket. Can you please email me (directly) when you come up with a kick ass solution. Thanks for looking into this.
-
artemave October 29th, 2009 @ 11:30 PM
Yet another attempt to get this feature through. The attached patch based on current 2-3-stable.
-
josh October 31st, 2009 @ 02:29 AM
- State changed from “wontfix” to “open”
- Milestone cleared.
- Assigned user changed from “josh” to “Yehuda Katz (wycats)”
If this feature is to go in Rails, it certainly needs to be implemented on master too. Reassigning to Yehuda to review this idea. Once we can get it working in 3.0, it seems fine to "backport" it. Even though it was actually done on 2.3 first ;)
-
-
artemave November 6th, 2009 @ 07:04 PM
Guys, quick doublecheck. Am I supposed to make 3.0 version at this point or the whole idea has to be first reviewed by Yehuda Katz?
-
Rizwan Reza January 22nd, 2010 @ 06:05 AM
I just talked to Yehuda and he's asking for a patch on master. He says this should be implemented in the Resolver, which currently doesn't have any way to specify fallbacks. (The "context" slot needs to take an array)
I am sure this will help you, artemave. Please do ask anything here about this. Thanks!
-
artemave January 22nd, 2010 @ 09:01 AM
Rizwan, I'm finishing up the patch at the moment. However the change affects AbstractController::Rendering rather than Resolver. I'll look closely though into this alternative and possibly come up with two different patches.
Thanks for your response, it is good to know there is after all life on Mars.
-
Rizwan Reza January 22nd, 2010 @ 10:56 AM
You might wanna buckle yourself up to write this up since Yehuda is all set to release Rails 3 beta soon. Good Luck!
-
-
Rizwan Reza January 24th, 2010 @ 06:01 AM
The patch applies cleanly. But artemave, you might want to do it in Resolver as Yehuda had said. Anyways, will pass it on to Yehuda and let's see what he says.
-
Rizwan Reza January 24th, 2010 @ 08:11 PM
Yehuda still wants it to be written in Resolver. Can you do that?
-
artemave January 24th, 2010 @ 08:35 PM
I am struggling to figure out how. Resolver has got no awareness of controller at the moment. Unless we start passing it to view_paths elements. Does that sound like a plan? Or is there something I am missing here?
-
Rizwan Reza January 24th, 2010 @ 09:50 PM
This is Yehuda had in mind:
There's a slot for "context" right now, which is the controller directory to look under. I think that would have to take an Array of contexts. The problem with doing it in render is that it'll only work in some cases but not in others. People will be wondering why a layout name from a parent controller doesn't come in.
He also wonders whether it might good to leave this until Rails 3.1.
artemave, I hope this helps. Thanks!
-
artemave January 25th, 2010 @ 09:29 AM
Rizwan,
I've been waiting for 3 months for any response. If it wasn't for that, this feature would have been done long time ago. So I think it would be fair for you guys to wait few days for me to finish.
-
Rizwan Reza January 25th, 2010 @ 09:31 AM
Sorry you had to wait that long. Get your patch ready, I will ask Yehuda to
apply it. Thanks! -
artemave January 25th, 2010 @ 09:33 AM
When you talk about 'array of contexts' do you mean the array in FileSystemResolverWithFallback?
P.S.
Good point about layouts not being inherited (in my patch). Frankly, I didn't know that layouts get picked up much the same way as regular templates. -
Caio Chassot January 25th, 2010 @ 09:43 AM
A quick note on layout inheritance:
Given controller A with layout "a", controller B < A, with an explicit call to layout false, B must not fallback layout "a", it must use no layout.
Just the kind of common bug that tends to crop up.
And thanks for taking this over from me.
-
artemave January 25th, 2010 @ 10:06 AM
No worries. This feature is a bit cursed though. Started here: http://dev.rubyonrails.org/ticket/5027 . Then failed to make it here: http://dev.rubyonrails.org/ticket/7076 . And got shot in this ticket. So, in the end, I just couldn't resist fighting the doom.
P.S.
Nice test case, thanks. -
artemave January 26th, 2010 @ 11:38 AM
Speaking of layout inheritance. If implemented, it may change current behavior of existing applications. Some controllers may start falling back to parent layout instead of application layout (current behavior). Please let me know if this is a problem.
-
artemave January 27th, 2010 @ 04:16 PM
Here is the version WITHOUT layout inheritance.
Few notes:-
Template inheritance ended up in PathSet rather than Resolver. Since instance of Resolver is an element in view_paths, it can only "apply itself" to parent controller. But if parent controller has different elements in its view_paths this will be incorrect.
-
MissingTemplate changed in order to comply with template inheritance. There were also some unused/out of place stuff there. As if it's been copypasted from 2.x code. I believe I cleaned up a bit.
-
PathSet::find_template method seemed to not have been used anywhere. Hence, removed.
Version WITH layout inheritance may be produced quickly once the desired behavior is clarified (see my previous comment)
http://github.com/artemave/rails/tree/view_inheritance_resolver_3-0
-
-
Rizwan Reza January 27th, 2010 @ 04:25 PM
Good job! Looks pretty nice so far. I will show this to Yehuda whenever (or wherever) I find him! :)
-
artemave February 5th, 2010 @ 09:18 PM
in_regards_to 'rails 3.0 beta release' do with :all_the_respect { artemave.says('Thanks for ignoring me').to($rails_team) } end -
artemave February 9th, 2010 @ 04:50 PM
Few changes:
- moved calculating parent controllers to AbstractController::Base
- rebased against latest master
http://github.com/artemave/rails/tree/view_inheritance_resolver_3-0
-
artemave February 25th, 2010 @ 08:33 PM
Can someone please reassign this ticket to someone who actually has time to deal with it? Seriously.
-
Caio Chassot March 3rd, 2010 @ 02:16 AM
Maybe if carl types
mergeand yehuda typespush, they can get it done this year. :P -
Yehuda Katz (wycats) March 5th, 2010 @ 04:01 PM
Hey guys,
Not sure if you've been paying attention to master, but we've been making some changes to ActionController and ActionView, specifically around the Resolver. Currently, there are too many paths that result in template lookup, making patches like this far more complex than they should be.
We've been working on refactoring this part of the code with the express intention of making it trivial to implement this feature (without needing as much code as is in this patch). Part of that work involved reducing global state with the intention of removing AV's direct dependency on AC for much of what it does. Again, you can follow that progress on master.
In short, I haven't pulled this patch because it exposed a problem with our current architecture that Carl and I wanted to fix the right way.
As I've said before, this feature is desired. We're actively working on it (again, see master), and we're going to get it in properly.
-
artemave March 6th, 2010 @ 10:30 AM
Thanks for status update. It certainly cleared up the picture.
I am therefor waiting til you guys finish up fighting global state. And there we'll see what is next.
-
Neil Smith May 19th, 2010 @ 12:15 AM
Please can anyone advise of the status of this issue? Did the relevant refactoring take place? It would be great to use this feature (or something very similar) in Rails 3.
-
-
artemave September 26th, 2010 @ 04:47 PM
Yet another go. http://github.com/artemave/rails/commit/a4de354d32a3bef0a5f14d5a4cc...
All tests are passing against current master.
-
-
artemave October 1st, 2010 @ 06:58 PM
Good point, I believe this fixes it github.com/artemave/rails/commit/f9b87450b2d5bde0177502c5b5bba2761e3b96a4
-
-
-
artemave December 19th, 2010 @ 04:01 PM
Rebased against current master. Again. https://github.com/artemave/rails/tree/view_inheritance
Can someone from rails team respond/review, please?
-
José Valim December 27th, 2010 @ 07:06 PM
- Milestone set to “3.1”
- State changed from “open” to “resolved”
- Importance changed from “” to “Low”
-
TuteC April 2nd, 2011 @ 09:29 PM
I can't make it work on Rails 3.0.5, neither adding 'config.template_inheritance = true' to my application.rb file. Is it published on Rails 3.0, or will it be in 3.1?
-
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
artemave
Caio Chassot
Dmitry Polushkin
Evgeniy Dolzhenko
Jeremy Kemper
José Valim
Manfred Stienstra
Neil Smith
Obie
Piotr Sarnacki
Rizwan Reza
Sam Pohlenz
TuteC
Yehuda Katz (wycats)