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

Ticket #10570 (reopened enhancement)

Opened 5 months ago

Last modified 2 months ago

[PATCH] Change application.rb to application_controller.rb

Reported by: jaycfields Assigned to: core
Priority: normal Milestone: 2.1
Component: ActiveSupport Version: edge
Severity: normal Keywords:
Cc:

Description

Currently the application.rb file does not follow Rails naming convention. This can cause an error if you try to reference a controller before "require 'application'" is executed. For example

FooController.caches_page :bar (in development.rb)

will fail.

Attachments

change_application_to_application_controller.diff (5.7 kB) - added by jaycfields on 12/20/07 19:49:32.
change_application_to_application_controller2.diff (7.7 kB) - added by jaycfields on 12/20/07 20:53:50.
change_application_to_application_controller2.2.diff (7.7 kB) - added by jaycfields on 12/20/07 20:56:07.
application_to_application_controller_forgiving_version.diff (6.5 kB) - added by jaycfields on 01/26/08 19:52:30.
update for rebase

Change History

12/20/07 00:16:13 changed by srrao2k

+1 This is a big annoyance..

12/20/07 00:20:20 changed by sarnacke

+1

12/20/07 00:22:40 changed by josh

+1

12/20/07 00:32:43 changed by clown_puncher

+1

12/20/07 01:10:06 changed by lifofifo

The only side effect I can think of the current patch is, application.rb won't be preloaded now. I'm not quite sure if it'll have any impact or not. Just pointing it out.

Thanks.

12/20/07 02:57:45 changed by wilson

I think this is a good change, but the initializer code should also try to require application.rb if the file exists, at Rails startup.

This will avoid making life irritating for all those existing projects.

12/20/07 05:04:21 changed by alexey

+1

12/20/07 05:24:40 changed by dcmanges

+1

12/20/07 05:29:37 changed by josh

Someone needs to update the Railties generators too.

Not it!

12/20/07 06:43:50 changed by ryankinderman

+1

12/20/07 15:50:20 changed by ph7

+1

12/20/07 16:56:59 changed by zak

+1 any backwards compatible headaches are worth future consistency. imho.

12/20/07 17:08:18 changed by jaycfields

  • keywords set to Verified.

12/20/07 17:43:26 changed by nealford

Big +1

12/20/07 19:49:32 changed by jaycfields

  • attachment change_application_to_application_controller.diff added.

12/20/07 20:19:59 changed by bitsweat

We'd also need to consider folks upgrading their apps without being aware of this change. Perhaps abort 'Please rename <x> to <y>' if File.exist?("#{RAILS_ROOT}/app/controllers/application.rb") in the Initializer?

12/20/07 20:52:37 changed by jaycfields

I changed the generator..

Adding to the initializer sounds like a good idea. I'll update the patch.

12/20/07 20:53:50 changed by jaycfields

  • attachment change_application_to_application_controller2.diff added.

12/20/07 20:56:07 changed by jaycfields

  • attachment change_application_to_application_controller2.2.diff added.

12/20/07 23:38:54 changed by tirsen

+1 (I can't believe you guys didn't do this for Rails 2.0.)

12/21/07 15:39:15 changed by revans

+1 good consistency

12/22/07 16:22:18 changed by pfarley

+1

12/22/07 18:08:08 changed by dcmanges

  • keywords changed from Verified to verified.

12/22/07 18:23:18 changed by bitsweat

  • milestone changed from 2.x to 2.1.

12/22/07 19:54:35 changed by ckponnappa

+1

12/23/07 16:56:52 changed by rickm

+1

01/02/08 11:10:40 changed by xaviershay

+1, not that it needs anymore

01/11/08 14:53:19 changed by lifofifo

-1

application_controller.rb should be *preloaded* to keep the old behavior. There is no sense in breaking existing code for a trivial issue.

01/11/08 19:19:00 changed by jaycfields

what 'old behavior'? I've tested the patch various ways and have yet to find something that breaks or behaves differently in any way. I don't see the point in creating a special case for no reason.

Also, calling it "trivial" is an opinion. I am currently mentoring someone who was stuck for 20 minutes while I was out grabbing lunch because he couldn't find where application_controller was defined, due to the inconsistency.

At another point a developer was blocked from checking in because he was using a controller in development.rb and couldn't figure out why it was failing.

These might not be your problem, but they are real issues for any organization adopting rails.

01/12/08 03:01:48 changed by nzkoz

  • keywords deleted.
  • summary changed from [PATCH] Change application.rb to application_controller.rb to [XPATCH] Change application.rb to application_controller.rb.

I think it's a little over the top to imply there's some kind of adoption blocker in this particular case. It's like this for historical reasons because abstract_application_controller.rb was a little long.

If you'd like to see this changed, let's discuss it with a thread on the core list rather than lots of back and forth here. We'd need an upgrade strategy, a deprecation strategy and some pretty good justification. I'm as annoyed as most of you by the special case, but that doesn't mean it's an easy thing to change :).

01/12/08 08:17:28 changed by railsjitsu

I totally agree with this, especially since there is a patch for the generator has a patch and a warning. Granted it should have a depreciation notice and perhaps only added as a one way street at 2.1 and for now let it be patched to work either way for 2.0.x

+1

01/12/08 17:39:06 changed by s0nspark

+1

01/12/08 18:11:17 changed by dkubb

+1

01/12/08 18:45:29 changed by indirect

Regardless of whether it's an adoption-blocker or not, it's inconsistent and annoying. Let's change it.

+1

01/12/08 19:34:01 changed by pat_maddox

+1

01/12/08 19:55:53 changed by JasonRoelofs

+1

This makes too much logical sense to leave it out. This breaks the "Principle of Least Surprise".

01/12/08 20:48:41 changed by lazyatom

+1

It should be possible to support loading of app/controllers/application.rb, if it exists, and then at some point deprecate it, surely.

01/12/08 21:50:56 changed by nzkoz

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

There's no point continuing to +1 this ticket until we've had a thread discussing this change on the mailing list. I'm closing this ticket until that's happened, assuming we decide to go that way we can reopen it.

01/12/08 23:36:59 changed by nzkoz

If you're interested in this change and have a strong opinion either way, jump into the mailing list to hash this out.

http://groups.google.com/group/rubyonrails-core/t/468c964babd6d011

01/13/08 03:55:48 changed by kovyrin

+1 - really great idea

01/21/08 23:49:24 changed by mdeiters

+ 1

01/26/08 19:45:24 changed by jaycfields

  • status changed from closed to reopened.
  • resolution deleted.
  • summary changed from [XPATCH] Change application.rb to application_controller.rb to [PATCH] Change application.rb to application_controller.rb.

The new version of the patch warns and autoloads application.rb if it exists.

01/26/08 19:52:30 changed by jaycfields

  • attachment application_to_application_controller_forgiving_version.diff added.

update for rebase

01/26/08 22:11:20 changed by nzkoz

The forgiving version looks good, but perhaps the initializer should use the deprecation methods instead of puts?

01/27/08 02:06:35 changed by nzkoz

The rescue nil thing seems a little broad too, what does it do when there's a syntax error in application.rb?

01/29/08 15:52:02 changed by jaycfields

I'll change it to require the file if it exists. As far as deprecation, I looked at the methods that were in there and only saw warnings that pertained to methods being deprecated. Did I miss a more general deprecation warning?

02/04/08 06:37:58 changed by lehuyvn

+1 I am patching existing Rails Application by opening its classes including controllers and decorating the methods, the patch is loaded at the end of environment.rb, it does not work unless I change application.rb to application_controller.rb

02/04/08 23:43:58 changed by nzkoz

You can just call the warn method on deprecation, that way it'll show up in the right place based on their preferences

Deprecation.warn "Wow, please rename"

02/07/08 07:51:18 changed by Vlad

+1

Annoying

02/13/08 12:35:35 changed by Vlad

Any news on when it's going to be checked in? :-)

02/13/08 12:58:42 changed by lifofifo

I'm sorry to be posting here again. But I *seriously* fail to see any advantages of this patch, compared to the confusion it can bring to people who are way too used to application.rb. May be I'm just too used to application.rb name.

So without planning to start any debates or trying to negative, I'm just gonna say I'm big -1 on this one. However, I do like the idea of renaming ApplicationController to Application. For those who didn't know, application.rb used to be called abstract_controller.rb. So we've kinda been there already.

02/13/08 13:20:01 changed by Vlad

Well, there are naming conventions so why don't we just follow them? Globally, Ruby on Rails is the one big convention :-). That's why I like it. So everything must be on it's place.

P.S. Lifo, thanks for your contribution to Rails, you're definetly one of those who Rails community should proud of, but I can't agree with your -1 ;-).

02/13/08 14:15:25 changed by alek

02/13/08 14:30:32 changed by Vlad

Alek, I don't get it

02/13/08 14:31:52 changed by lifofifo

02/13/08 15:03:06 changed by Vlad

Ahhhr, bastard :-D

03/19/08 21:02:02 changed by pradeep

+1