requestAction considered harmful
Posted on 20/8/08 by Felix Geisendörfer
Hey folks,
this is post #1 of my 30 day challenge. If you have ever written a modestly complex application in CakePHP, you probably noticed that it is very tempting to use requestAction to glue together pieces from different controller actions. While initially that sounds like a good idea, I think requestAction should be avoided wherever possible.
Avoid requestAction in your application wherever you can - it will save you from terrible debugging nightmares.
For one, the speed penalty of using requestAction is quite prohibitive. The main slowdown is that every time you call requestAction, a full dispatch cycle takes place. This is pretty inefficient given that you probably already know what exact Controller/action you want to call up. A much bigger reason is that you will create a debugging nightmare for yourself. As soon has you have requestAction calls that invoke requestAction calls that invoke requestAction calls - you're screwed. If somewhere along the chain odd things happen (like a bug in your Auth system, some controller's / component's beforeFilter) debugging is going to become a major pain.
But worry not. This doesn't mean that you have to overload your dashboard / profile pages with ridiculous amounts of logic. The approach to use is to create named find calls and move all logic there. Once you have done that, getting the data for a complex view is simply a matter of making the right find calls and rendering a bunch of elements. You can even render other controllers views where it makes sense. For example lets say you want to re-use posts/view.ctp inside posts/edit.ctp to show the user a preview of the currently edited post. Instead of using requestAction, try this:
Again, there are some legitimate cases for using requestAction. All I'm saying here is that you should avoid creating an architecture that depends on requestAction at all costs. I've seen (and rewritten) applications that have gone that route and it has never been pretty so far ; ).
HTH,
-- Felix Geisendörfer aka the_undefined
Did you like this blog post? If so, please consider subscribing to the Blog RSS feed.
You can skip to the end and add a comment.
klevo: Glad you like the post : ). About the title: english is my second language so I might be wrong, but I think I've used the term correctly:
klevo: I'd say his usage is spot on, particularly considering the issues he's highlighting. As a general comment, I will say that in all the Cake apps I've built, I've only ever used requestAction once. Over-use (or sometimes just use) of requestAction is a indicative of poor design, and often means you need to push more application logic down into your models, or sometimes into a shared component.
Very good post Felix, and good luck with your 30 days challenge. Only one question for you:
Suppose I wanna show my best posts in a navbar in my website, and it has to repeat over the site. How do you handle this without using requestAction or trying AD7six Mini Components? I would love to have an easier and better solution for this very common scenario.
Cheers
I admittedly use requestAction ALL the time to bind together different pieces of the application; what would your advice be in a situation where you wanted to show 5 recent "posts" on events/index? Would you prefer to include the Posts model in the Events controller and keep the find/set in the Events/index action? Isn't this "breaking" mvc rules?
Not trying to flame, I'm just actually curious as I do this (using requestAction) quite often, and I've never really had any kind of performance/debug issues.
While I'm thinking of it, another case I use requestAction for quite frequently is where I want to perform some AJAX refreshing. Say I have a list of Posts that I want AJAX paging applied to - in order to keep from having the logic/view code in more than 1 place, I'll use a requestAction with a return = true or a requestAction combined with an element to display the initial page and let AJAX take over from there.
Not saying it's right or not - even thinking it's not... just asking:
What do you all think about the spartan's approach of getting data loaded in some sort of a DataHelper - loading models and executing basic data selections...
It's something i've seen but named WidgetHelper, and it worked like this in the view: foreach($widget->retrieve('Comment', 'latests')): // display...
in WidgetHelper::retrieve() it loaded Comment via ClassRegistry and executed find('latests')...
It felt so wrong and nasty, but so simply handy, as all the code needed was straight-from-the-db-data, no complex logic nor any sort of app's business logic.
So, how do you all feel about it?
oh ok, I was a bit lazy writing the post, but you got the picture...
@Martin : why people love to talk about breaking "MVC rules"?
@Ryan : your pagination feature seems a little bit inacurrate, maybe you should try read the manual on paginator or explain yourself better...
@Ryan
One approach would be to render and cache a recent posts element any time a new post is made. Then call the element in your views where ever its appropriate.
@rafaelbandeira3: I really like that approach even though it's supposedly not MVC kosher. It's definitely faster than requestAction, but I'm sure you'll receive complaints about accessing a model in a helper.... even though the Form helper does this.
@Steve Oliveira : Yes, that was the first thing that came in my mind, but there's one thing, it doesn't access db data...
@rafaelbandeira3: Ah? What are u talking about? I didnt say anything about MVC rules...
Anyway, I´m still waiting for Felix answer to my question.
@Martin Bravio : doh... it was Ryan... sorry...
@rafaelbandeira3 - the pagination thing was just an example that popped into mind; doing ajax pagination on a page before the paginator was even invented
@HeathNail & @rafaelbandeira3 - rules is rules, I don't know... I just like to stick as much to the MVC approach as possible (most of the time) and in the examples I tried to give, I was trying to point out times when you would definitely need to access data from other controllers, and I love requestAction for that - for instance a home page with Posts, Events, Galleries, etc etc - I can have a simple page setup in /pages/home.ctp and plop down a bunch of requestAction's to the other various elements I need and either return the view (which is apparently frowned upon) or render an element - that's exactly what I did here: http://reaxmusic.com/ and it's really really handy, and I didn't notice any kind of performance hit (perhaps 5 requestActions isn't too extreme)
just giving my 2 cents, not saying i'm necessarily "right"
@Ryan - Just a suggestion, ref'ing your pagination feature, try to always stick to the core.
@Ryan - actually 5 requestAction() are the almost the same as load 6 pages in one single hit, except that you have just one Http request. Even Felix wich is one of the most not-worried-about-performance issues - tried to find the links to prove, but I suck and don't find them, but you all know - is suggesting not to use it because of performance worries. I think requestAction is a great tool, but completely missused. What is all that logic that you need from controller's action to get the latests events? My EventsController::latests(), in a public - non-auth-needed - fashion wouldn't be bigger than this:
function latests($ammount = 5) {
$data = $this->Event->findLatests($ammount);
$this->set(compact('data'));
return $data;
}
Why would I need all the Dispatcher -> Router -> Controller -> Components -> Models path to get this?
oh.. it's not an object chain... "doh"...
@Ryan : even with the large amount of requestActions: nice website!
@rafaelbandeira3: Agreed. However, one thing, why do you do something like $this->set(compact('data')). Isn't everything data? Shouldn't you be more specific ?
@Tim : actually it's a code standard we use... somewhere at line 240 of our doc book, mainly written by myself, says something like:
"For main data handling on view layer, use var named $data"
So we can share main data handling trough helpers - grabbing View instance on ClassRegistry and then $View->viewVars['data'], easily find data entrys we want, quickly add new data display, and avoid seing redundant stuff like:
echo $post['Post']['author'];
// instead I do
echo $data['Post']['author'];
// or in a multiple row data set
foreach($posts as $post):
echo $post['Post']['title'];
// .. instead I do
foreach($data as $entry):
echo $entry['Post']['title'];
oh... there's also a line wich says: "Main data is the data returned by controller's action". So you can set secondary data to the view, but it isn't the data wich is returned....
that's my design, not saying it's right... but it's very productive. What do you think?
Felix Geisendörfer: I think we feel the same about this subject, I posted similar recommendations a few days ago. I focused more on using elements, but reusing your controller views is a great alternative.
Can you show an example helper and/or view that makes use of this? :)
An excellent post... In my work (with 1.1 only) I too have determined this to be the case. Especially if your application sticks way too much into app_controller and a beforeFilter() -- like all of mine end up doing. Thanks for the post and this blog - getting useful stuff out already.
I used a requestAction call to generate an element on my home page. The home page is driven by the PagesController. I tried to use the method that this post demonstrates but I cannot seem to get the element to work. The view that I am trying to render is from another controller, DomainsController, in this case. So I call echo $this->element('../domains/renewals',array('post' => $this->data)); The view renders just fine when called directly but not on the home page. Any advice? I told PagesController to use the Domain model but that didn't change anything.
@Tim Koschützki : was it for me?
anyway... if you want to know what I mean : http://bin.cakephp.org/view/1546157175
... of course it wont last forever!
I'm a little (a lot) late to the party, but while looking for an alternative to requestAction to access database content from any controller, I came across this post. I was intrigued by Rafael Bandeira's idea of making a Widget helper, so I did my own version of it and came up with a Widget Component: http://jamienay.com/2009/06/a-quick-dirty-and-useful-widget-component-for-cakephp/
@rafaelbandeira3 : That is interesting indeed. Thanks for sharing.
@Jamie : Well I myself like to use custom find methods as well, but I keep calling them from the controller actions again and again. This is not duplicate code in my opinion. Example:
$article = $this->Article->find('by_slug', compact('slug'));
$recentArticles = $this->Article->find('recent', compact('slug'));
$relatedArticles = $this->Article->find('related', compact('slug'));
$this->set(compact('article', 'recentArticles', 'relatedArticles'));
}
This is just for me to know what's going on and what is loaded for each action by simply looking at the controller action code. Scattering code that loads data from models across components, etc. doesn't make so much sense to me.
@Tim Koschützki : That works, but the reason I stay away from that method is that (presumably) in every controller you want to load the most recent articles from, you need to associate the Article model with that controller's model. And if you're putting the latest articles, in, say, a sidebar that displays on every page, that means you need to associate the Article model with every other model. To me that just doesn't make sense semantically: an Article has nothing to do, with, say, a Calendar of Events, but if you want to stick all of the find logic in individual controllers, you'll need to add that association. You can simplify things by adding the association to AppModel, but that's still a lot of redundant code.
@Jamie : You can just use AppController::beforeRender() and then use ClassRegistry::init('ModelName') there.
That's how I do it all the time.
This post is too old. We do not allow comments here anymore in order to fight spam. If you have real feedback or questions for the post, please contact us.
Very good post, especially the solutions to avoid using requestAction. But the title may be a bit misleading. It implies there is something broken or some security risk with requestAction, while that is not true. It's use may be limited, but it's good to have it as an option.