This project is archived and is in readonly mode.

#3988 ✓hold
andyjeffries

[PATCH] Deprecate public_path and create 'public' method that returns a Pathname

Reported by andyjeffries | February 18th, 2010 @ 08:26 AM | in 3.0.2

Having got quite used to Rails.root returning a Pathname object to allow easy concatenation of paths, I noticed that Rails.public_path doesn't. I checked in 2.3.5 and master and it's not in either case.

Am I missing something? Is there a reason for the inconsistency that I'm unaware of?

I've attached a patch (at Ryan Bigg's suggestion on rails-core) that deprecates public_path and makes a new public method that returns a Pathname (at Nick Quaranto's suggestion on rails-core).

Comments and changes to this ticket

  • Matt Jones

    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

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

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

    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!

  • andyjeffries

    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

    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.

  • andyjeffries

    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.

  • José Valim

    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!

  • andyjeffries

    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

  • andyjeffries

    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

    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.

  • andyjeffries

    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

    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.

  • andyjeffries

    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

    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.

  • siong1987

    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

    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).

  • andyjeffries

    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

    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

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

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

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Tags