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

Ticket #10130 (closed defect: fixed)

Opened 1 year ago

Last modified 1 year ago

[PATCH] Subtemplates broken when used with text.plain.erb in ActionMailer

Reported by: java Assigned to: core
Priority: normal Milestone: 2.0
Component: ActionPack Version: edge
Severity: normal Keywords: actionview, actionmailer, template, subtemplate
Cc:

Description

I was writing an ActionMailer using the 2.0 Preview (all this applies to Edge as well). Since my mails were sent as text/plain, I named my template order_confirmation.text.plain.erb like suggested in the documentation.

(I don't know exactly where I read this, the rails documentation is currently lacking a clear and strong opinion on the issue of naming your templates. .rhtml is deprecated but that isn't made clear in the docs and how to properly name the files now is not written down anywhere too, but that's stuff for another ticket)

All went well until I created a second template and a common subtemplate that I rendered with

<%= render "cart" %>

Rails couldn't find the subtemplate since no extension was provided. In such a case ActionView looks into the controller for a Content-Type but since this was ActionMailer, none was found (although ActionMailer does provide that information, just not in #request.parameters[:format], but in #content_type). To fix this I extended ActionView::Base#find_template_extension_for to determine the extension for the subtemplate based on the extension of the @first_render template.

So, if a subtemplate is rendered and no template_format is given, ActionView now assumes the same extension as in the "main" template (@first_render). Doing this revealed an additional bug in ActionView::Base#render_file:

template_extension = template_extension.gsub(/^\w+\./, '')

had to be changed to

template_extension = template_extension.gsub(/^\.+\./, '')

otherwise a text.plain.erb extension would only be stripped to plain.erb and compiling the template wouldn' work.

I have attached two patches, one containing the tests revealing the problem, the other containing the solution.

Attachments

actionview_subtemplate_test.diff (2.1 kB) - added by java on 11/10/07 14:21:29.
A test case revealing the problem
actionview_extension_from_first_render.diff (1.7 kB) - added by java on 11/10/07 14:22:31.
A patch for ActionView::Base that solves the problem
template_extension_fix.patch (432 bytes) - added by tarmo on 11/20/07 02:18:07.
subtemplate_test_missing_fixtures.diff (1.2 kB) - added by java on 11/20/07 08:15:36.
Teh fixture files for the test (there might have been a problem with the original patch, generated by SmartSVN)
template_extension_first_render_fix_v2.patch (0.6 kB) - added by tarmo on 11/21/07 03:40:40.
Better fix for the extension extraction which currently claims '/asd.rb' is an extension for 'asd/asd.rb' and that if there is no extension then the whole path is an extension.
template_extension_first_render_fix_with_tests.patch (1.5 kB) - added by tarmo on 11/22/07 04:26:29.
Now with actual tests

Change History

11/10/07 14:21:29 changed by java

  • attachment actionview_subtemplate_test.diff added.

A test case revealing the problem

11/10/07 14:22:31 changed by java

  • attachment actionview_extension_from_first_render.diff added.

A patch for ActionView::Base that solves the problem

11/18/07 22:01:37 changed by david

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

(In [8166]) Fixed that partial rendering should look at the type of the first render to determine its own type if no other clues are available (like when using text.plain.erb as the extension in AM) (closes #10130) [java] Fixed that partials would be broken when using text.plain.erb as the extension #10130 [java]

11/20/07 01:52:18 changed by tarmo

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

This fix causes problems for ruby-gettext, more specifically it makes ActionView::Base#file_exists?() not return nil for templates that do not exist.

This is because although find_template_extension_from_handler() seems to check if the template file exists find_template_extension_from_first_render() does not, in fact the latter will even return wrong values if the @first_render does not contain an extension. For example with "venue/show" as @first_render it returns "/show" which is not an extension.

The reason ruby-gettext is broken is that ruby-gettext modifies render_file to first try to render a localized version of a template and if one does not exist then fall back to the original and ofcourse the existance check for the localized templated misbehaves because it uses file_exists?().

For reference, here's the ruby-gettext code: http://pastie.caboo.se/120013

11/20/07 02:18:07 changed by tarmo

  • attachment template_extension_fix.patch added.

11/20/07 02:21:46 changed by tarmo

I've attached a patch which I seems to fix the ruby-gettext errors and I believe no longer allows file_exists?() to return false information.

The patch basically just makes the extension extraction more thorough. If the input string has no extension then nil will be returned instead of part of the input string.

11/20/07 08:14:10 changed by java

File.extension does not work correctly for this purpose: 

>> "asdfasdf.text.plain.erb".sub /^\w+\.?/, ''
=> "text.plain.erb"
>> File.extname("asdfasdf.text.plain.erb")
=> ".erb"

The full extension, including the type-identifier is needed. Your change causes the test to fail.

The changeset [8166] includes the testcase from my patch but misses both fixture files, causing the test to fail.

11/20/07 08:15:36 changed by java

  • attachment subtemplate_test_missing_fixtures.diff added.

Teh fixture files for the test (there might have been a problem with the original patch, generated by SmartSVN)

11/20/07 08:16:46 changed by java

Well, somehow trac doesn't properly display the diffs for the new fixture files...

11/20/07 11:59:12 changed by tarmo

Oh, so you need the whole extension, well, then try to write it in a way that no extension is found if there is none. For example the current implementation seems to think "/show" is an extension when @first_render is "venue/show".

11/20/07 11:59:50 changed by tarmo

The fixtures in the patch are fine btw. it's just Trac that fails to show them.

11/21/07 03:40:40 changed by tarmo

  • attachment template_extension_first_render_fix_v2.patch added.

Better fix for the extension extraction which currently claims '/asd.rb' is an extension for 'asd/asd.rb' and that if there is no extension then the whole path is an extension.

11/22/07 04:26:29 changed by tarmo

  • attachment template_extension_first_render_fix_with_tests.patch added.

Now with actual tests

11/22/07 05:01:10 changed by nzkoz

(In [8187]) Tests and fix for extension extraction. References #10130 [tarmo]

11/22/07 05:01:46 changed by nzkoz

(In [8188]) Add missing sub template fixtures. References #10130 [java]

11/26/07 04:08:04 changed by nzkoz

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