Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #10800 (closed defect: fixed)

Opened 2 years ago

Last modified 2 years ago

[PATCH] Refactor caching mechanism of ActionView and introduce ActionView::TemplateFinder

Reported by: lifofifo Assigned to: core
Priority: normal Milestone: 2.1
Component: ActionPack Version: edge
Severity: normal Keywords: threadsafe
Cc:

Description

This patch refactors ActionView caching mechanism using a new proposed class - ActionView::TemplateFinder. TemplateFinder reads view paths directory when rails gets loaded and caches them for further use.

Attachments

refactor_action_view_io_caching.patch (20.9 kB) - added by lifofifo on 01/14/08 08:37:32.
refactor_action_view_io_caching.2.patch (27.5 kB) - added by lifofifo on 01/16/08 04:08:32.
safe_av.patch (27.5 kB) - added by lifofifo on 01/18/08 00:19:37.
Updated against latest edge
safe_av.2.patch (29.1 kB) - added by lifofifo on 01/19/08 02:32:10.
add_deprecation_2_0_stable_relative_render_file.patch (2.1 kB) - added by lifofifo on 01/19/08 03:08:38.
Add deprecation notice to 2-0 stable branch about not using render :file with relative path
safe_av.2.2.patch (29.1 kB) - added by lifofifo on 01/19/08 03:12:29.
safe_av.2.3.patch (28.6 kB) - added by lifofifo on 01/19/08 03:15:32.
fix_all_issues_with_template_finder.patch (29.5 kB) - added by lifofifo on 01/19/08 15:53:23.
fix_all_issues_with_template_finder.2.patch (29.7 kB) - added by lifofifo on 01/19/08 17:10:51.
fix_all_issues_with_template_finder.2.2.patch (29.2 kB) - added by lifofifo on 01/21/08 12:06:36.

Change History

01/14/08 08:37:32 changed by lifofifo

  • attachment refactor_action_view_io_caching.patch added.

01/14/08 08:38:13 changed by lifofifo

Please note that the patch introduces one failing test case, which needs to be discussed.

01/14/08 09:33:34 changed by chuyeow

I like where this is going. I had lots of pain trying to fix the currently broken test in caching_test.rb (the one where a partial uses cache within an RJS template).

Newbie question, but how does one apply a git patch to a SVN checkout?

(follow-up: ↓ 6 ) 01/14/08 09:56:17 changed by lifofifo

chuyeow : This patch introduces another failing test, apart from currently broken caching_test.rb. You can just do "patch -p1 < this_patch.patch" to apply it.

The current patch is a work in progress. It still needs some discussions and testing. I don't really like instance level prepend/append_view_path methods. Evan Weaver thinks the current code might leak if someone is doing some crazy stuff with per request view_paths. I'm not really sure if request level view_paths deserves to be in core or not.

Also, the patch could use some benchmarking on some view heavy application, if someone wants to help.

01/16/08 04:08:32 changed by lifofifo

  • attachment refactor_action_view_io_caching.2.patch added.

01/18/08 00:19:37 changed by lifofifo

  • attachment safe_av.patch added.

Updated against latest edge

01/19/08 02:32:10 changed by lifofifo

  • attachment safe_av.2.patch added.

01/19/08 03:08:38 changed by lifofifo

  • attachment add_deprecation_2_0_stable_relative_render_file.patch added.

Add deprecation notice to 2-0 stable branch about not using render :file with relative path

01/19/08 03:12:29 changed by lifofifo

  • attachment safe_av.2.2.patch added.

01/19/08 03:15:32 changed by lifofifo

  • attachment safe_av.2.3.patch added.

01/19/08 03:19:54 changed by bitsweat

  • milestone changed from 2.x to 2.1.

deprecation patch doesn't apply cleanly

01/19/08 03:20:42 changed by bitsweat

  • status changed from new to closed.
  • resolution set to fixed.

(In [8669]) Introduce TemplateFinder to handle view paths and lookups. Closes #10800.

(in reply to: ↑ 3 ) 01/19/08 06:38:59 changed by josh

Replying to lifofifo:

The current patch is a work in progress. It still needs some discussions and testing. I don't really like instance level prepend/append_view_path methods. Evan Weaver thinks the current code might leak if someone is doing some crazy stuff with per request view_paths. I'm not really sure if request level view_paths deserves to be in core or not.

I feel the same way.

I think the view extension name caching foo should be its own class (locked with a mutex). Almost like a ActiveSupport::CacheStore. eg ActionView::TemplateCache. Hey, you could even try to stash the compiled erb templates in there. Maybe I'm breaking off into other patch however just ideas.

I say try that first, then stab at refactoring the general view looker-upers.

01/19/08 06:47:45 changed by josh

Okay, created a little todo note for myself #10857

01/19/08 15:53:23 changed by lifofifo

  • attachment fix_all_issues_with_template_finder.patch added.

01/19/08 17:10:51 changed by lifofifo

  • attachment fix_all_issues_with_template_finder.2.patch added.

01/20/08 03:42:24 changed by chuyeow

  • status changed from closed to reopened.
  • resolution deleted.

r8676 reopens this.

01/21/08 12:06:36 changed by lifofifo

  • attachment fix_all_issues_with_template_finder.2.2.patch added.

01/21/08 20:45:08 changed by nzkoz

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [8683]) Reapply the TemplateFinder first applied in [8669] then reverted in [8676]. Closes #10800 [lifofifo]

01/31/08 23:23:26 changed by cainlevy

So if I'm understanding this correctly, view_paths is now not just a list of paths that ActionView will use to auto-detect views, but an exclusive whitelist of paths that ActionView will render from. This breaks every plugin I'm using that provides its own views: ActiveScaffold, SeleniumOnRails, and even ExceptionNotification.

-1

01/31/08 23:45:51 changed by lifofifo

Here is the fix for ExceptionNotification.

As long as all the plugins play well with view paths ( and change them using rails methods for doing so - e.g. append/prepend_view_paths view_paths= etc. ), it should be fine. If not, you might wanna call process_view_paths directly.

02/01/08 02:48:36 changed by cainlevy

I still think that this is a paradigm shift, because now templates *must* be in view_paths, whereas before they *could* be in view_paths. The problem with SeleniumOnRails, for example, is that it does something like:

render :file => "../../vendor/plugins/selenium_on_rails/lib/views/setup.rhtml"

I figured out to fix this by hacking it to use absolute paths and adding :use_full_path => false' to all the render :file' calls.

So is the paradigm shift good or bad? Depends on your perspective, I guess. My impression was that Rails' overall philosophy was to encourage/discourage, not permit/deny, and that API breaks were bundled up in major updates.

Anyhow, I've hacked all my plugins into obedience by now, so it worksforme.