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

Ticket #9525 (closed defect: fixed)

Opened 2 years ago

Last modified 2 years ago

[PATCH][TINY] assert_redirected_to crashes when controller is a symbol

Reported by: shoe Assigned to: core
Priority: normal Milestone: 1.x
Component: ActionPack Version: edge
Severity: normal Keywords: assertions tiny verified
Cc:

Description

I'm writing tests for an application that uses many redirections so:

redirect_to :controller => :elsewhere, :action => "flash_me"

(Note that the controller name is a symbol instead of a string.)

Since the framework seems to work fine with this code, it's a pain that the assertions crash when testing it.

This patch makes the assertion not only not error out, but also pass.

Attachments

assert_redirect_to_with_symbol.diff (3.2 kB) - added by shoe on 09/10/07 21:33:16.
assert_redirect_to_with_symbol_v2.diff (3.7 kB) - added by shoe on 09/29/07 20:02:09.

Change History

09/10/07 21:33:16 changed by shoe

  • attachment assert_redirect_to_with_symbol.diff added.

09/10/07 21:35:14 changed by shoe

Just a note: the patch also contains a test case for the assertion.

09/10/07 21:43:18 changed by shoe

  • keywords changed from assert_redirect_to to assert_redirect_to, tiny.
  • summary changed from assert_redirected_to crashes when controller is a symbol. to [PATCH][TINY] assert_redirected_to crashes when controller is a symbol..

09/11/07 13:48:57 changed by norbert

  • keywords changed from assert_redirect_to, tiny to assertions tiny docs.
  • summary changed from [PATCH][TINY] assert_redirected_to crashes when controller is a symbol. to [PATCH][TINY] assert_redirected_to crashes when controller is a symbol.

Though the patch seems to work, I'm not sure using symbols is officially supported behaviour.

09/11/07 13:51:02 changed by norbert

Also see #7535 (which was reported earlier but didn't have a patch) for more information on this issue.

09/11/07 14:28:42 changed by shoe

Well, in the ActiveController::Base docs I see only examples of strings for controllers, but here's my reasoning:

1) Apparently, symbols actually work fine. 2) Assertions should only pass or fail, never crash.

Therefore, which would be more welcome, a patch to make the assertion fail or a patch to make it pass?

09/11/07 14:59:50 changed by norbert

  • keywords changed from assertions tiny docs to assertions tiny.

I think the patch is fine as is, but it should be noted that this is a bit of an edge case. Maybe others will have some more constructive input.

09/14/07 01:05:17 changed by shoe

What's the next step with this ticket?

09/16/07 23:49:20 changed by tarmo

I don't know if symbols are officially supported, but I've been using them without issues for a long time (also on edge with the patch from #8562) so it would be nice to have symbols work.

09/17/07 02:02:44 changed by boone

Aren't symbols supposed to be faster and use less memory than strings? I seem to recall reading that somewhere, and when I found they worked with redirects, I started using symbols for all of them. Which was great until I got around to writing my tests, and came across this bug.

I wonder if using symbols with redirect_to has any measurable performance benefit. If so, I'd say that's a good argument for officially accepting their use.

09/17/07 08:51:52 changed by tarmo

Symbols use less memory than strings, if there is more than one instance of them all the instances point to the same textual representation instead of having to copy it (like strings). The downside is that once created a memory taken by a symbol can not be recovered.

In the case of url_for the memory usage and speed improvement is probably insignificant compared to the slight speed improvement in writing the code, a symbol is a bit easier to write than a string.

09/25/07 18:33:51 changed by shoe

Can anyone suggest how to move this bug along? What needs to happen next? Is there a subsystem maintainer for ActionPack? A module-owner? Is there a review/super-review process?

09/25/07 18:43:57 changed by tarmo

shoe, read http://dev.rubyonrails.org/wiki

+1, already using this to make tests pass on my project

09/28/07 14:36:39 changed by tomafro

+1, I've been bitten by this too many times

09/29/07 20:02:09 changed by shoe

  • attachment assert_redirect_to_with_symbol_v2.diff added.

09/29/07 20:03:18 changed by shoe

The v2 version of the patch just changes the test to exercise the :action => :symbol case, too.

10/04/07 20:42:03 changed by joe

+1 on assert_redirect_to_with_symbol_v2.diff -- just got bit by this trying a 1.2.3 app on the 2.0 preview release.

10/04/07 20:46:18 changed by tarmo

  • keywords changed from assertions tiny to assertions tiny verified.

10/07/07 19:12:06 changed by bitsweat

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

(In [7776]) Fix url_for, redirect_to, etc. with :controller => :symbol instead of 'string'. Closes #8562, #9525.