Comments and changes to this ticket

• 

  Matt Jones February 20th, 2010 @ 06:13 PM

  • Title changed from “[PATCH] ” to “[PATCH] Deprecate public_path and create 'public' method that returns a Pathname”

  Fixed the title.

• 

  José Valim February 23rd, 2010 @ 09:29 PM

  • Assigned user set to “José Valim”
  • Milestone cleared.
• 

  José Valim February 26th, 2010 @ 10:58 AM

  Hey Andy, thanks for looking at this. Can I propose a different solution though?

  In Rails 3, all your application paths are configurable using the config.paths method. So, the proper fix to this issue is indeed deprecate Rails.public_path, but however it should point to config.paths.public.

  This is the method which defines the paths:

  http://github.com/rails/rails/blob/master/railties/lib/rails/applic...

  As you can see, it calls super, which gets the paths definitions from the engine:

  http://github.com/rails/rails/blob/master/railties/lib/rails/engine...

  All you need to do is define paths.public, deprecate Rails.public_path as you already did and ensure tests pass. Are you in mood for trying a new patch then?

  Thanks!

• You flagged this item as spam.
  

  andyjeffries February 26th, 2010 @ 11:01 AM

  Sure José, I want to start contributing to Rails so I'll definitely try it the new way. I'll have a go at whipping something up today. Thanks for the hints.

• 

  José Valim February 26th, 2010 @ 11:11 AM

  You're welcome! If you need any help, post here and I will reply as soon as possible.

• You flagged this item as spam.
  

  andyjeffries February 26th, 2010 @ 12:57 PM

  I've tried to rework the patch, but I fail. The tests all pass except for a couple of metal ones (where it seems to return the index.html content - not sure that's me).

  However, when I try to use stylesheet_link_tag (to test that asset_tag_helper is working) it throws a Nil exception (because Rails.public returns nil at that point).

  I'm a bit stuck for why now, as this is my first real look in to Rails and I'm not sure of the startup order.

  If you could have a look at my patch and make any suggestions as to where I'm going wrong, I'd appreciate it.

  • 0001-andy_public_attempt1.diff 6.8 KB
• 

  José Valim February 26th, 2010 @ 01:03 PM

  Andy, you get it right. The problem is with the load order. What you will need to do is to create a new initializer in "actionpack/lib/action_controller/railtie.rb" which sets the @@page_cache_directory. Something like:

  initializer "action_controller.set_page_cache_directory" do |app|
    ActionController::Base.page_cache_directory = app.config.paths.public.to_a.first
  end
  

  Instead "app.config.paths.public.to_a.first", you may also do "Rails.public" as well. Good work so far!

• You flagged this item as spam.
  

  andyjeffries February 26th, 2010 @ 01:05 PM

  It appears the metal failures (which now it becomes important I'll list them - test_multiple_metal_endpoints and test_single_metal_endpoint) are me as a vanilla master checkout passes them.

  However this simple metal works fine:

  Allow the metal piece to run in isolation

  require File.expand_path('../../../config/environment', FILE) unless defined?(Rails)

  class Test
  def self.call(env)

  if env["PATH_INFO"] =~ /^\/test\-metal/
    [200, {"Content-Type" => "text/html"}, ["Hello, World!"]]
  else
    [404, {"Content-Type" => "text/html", "X-Cascade" => "pass"}, ["Not Found"]]
  end
  

  end end

• You flagged this item as spam.
  

  andyjeffries February 26th, 2010 @ 01:15 PM

  José Thanks for that, I guess it's a hint rather than the whole solution, but I can't tell how to apply it to AssetTagHelper?

  The helpers are all autoloaded, so they should only be defined on first access. That shouldn't happen until after the Rails module is defined, right?

• 

  José Valim February 26th, 2010 @ 01:18 PM

  Imagine that I have a gem that extends AssetTagHelper. I will load it before the application is loaded. In other words, AssetTagHelper can be loaded before the application is defined.

  The best solution is to refactor AssertTagHelper to use something like public_path or page_cache_directory as well, that we can also be set in the Railtie.

• You flagged this item as spam.
  

  andyjeffries February 26th, 2010 @ 01:32 PM

  Oh man alive, this is turning out in to quite a job for my first little patch to dip my toes in!

  I'll have a bit of a deeper look at this problem next week then.

• 

  José Valim February 26th, 2010 @ 01:36 PM

  Sure! Fell free to make different patches too. You can have one to make AssetTagHelper more modular and then have a second patch which changes Rails.public_path.

• You flagged this item as spam.
  

  andyjeffries February 26th, 2010 @ 01:37 PM

  If you're willing to offer help to a new developer like this, it might be better to take it to email/MSN rather than fill up lighthouse with it. My email address is andy@andyjeffries.co.uk and MSN is passport@andyjeffries.co.uk

• 

  José Valim February 26th, 2010 @ 01:39 PM

  The pros of having it in Lighthouse is that it may help other people as well to understand how Rails works. Anyway, I send you an e-mail in case you want to continue by e-mail. Both ways work for me.

• You flagged this item as spam.
  

  siong1987 February 28th, 2010 @ 07:51 AM

  I guess it might be a good idea to continue the discussion here. I find that I learn a lot just by following on the discussion.

• 

  José Valim March 5th, 2010 @ 02:46 PM

  It seems that Carl was faster and already refactored the whole configuration stuff. What's left is just to deprecate the public_path method (no need to add a Rails.public method).

• You flagged this item as spam.
  

  andyjeffries March 5th, 2010 @ 03:30 PM

  OK, never mind - he's probably done a better job than me anyway (mine still wasn't working).

  Did you ever speak to the Core guys to see if they wanted public_path deprecating?

  AJ

• 

  José Valim March 5th, 2010 @ 08:56 PM

  • State changed from “new” to “hold”

  I talked with Yehuda and he told me that they already have plans for Rails.public_path. I'm putting this on hold, I hope we will close it in a week or two. Thanks!

• 

  Jeremy Kemper October 15th, 2010 @ 11:01 PM

  • Milestone set to “3.0.2”
  • Importance changed from “” to “Low”

  [bulk edit]