#43 ✓resolved
David Krmpotic

Subclassing Rack::Request

Reported by David Krmpotic | March 26th, 2009 @ 03:40 PM | in 1.0

Signature of 'new' class method on Request class should be

def self.new(env, *args)

instead of

def self.new(env)

Because if I want to subclass the Request and redefine a constructor like this:

class MyRequst < Request def initialize(env, param)

super(env)
@param = param

end end

it fails with wrong number of arguments (2 for 1)

simple addition of *args to the signature solves the problem with nothing else to be done.

Comments and changes to this ticket

  • manveru

    manveru March 28th, 2009 @ 03:11 AM

    • State changed from “new” to “invalid”
    • Assigned user set to “manveru”

    -1

    You have to define your own MyRequest::self method, that's the whole point of subclassing.

  • Mislav

    Mislav March 29th, 2009 @ 07:48 PM

    To support David's plea:

    We had a subclass like this:

    
class MyRequest < Rack::Request
  def initialize(env, another_arg)
    super(env)
    @another = another_arg
  end
end

    This works on Rack 0.9.1 (latest stable gem release) and fails on rack edge. Why? Because rack edge overrides Request.new method (a rare practice in Ruby, although perfectly valid) but only forwards one argument through it, instead of all arguments with the "splat" operator.

    In other words, this change in rack edge that was supposed to be completely transparent to the user actually breaks backwards compatibility with rack apps built for 0.9.1.

    Yes, I understand your advice that we should (re)define MyRequest.new (I'm sure that's what you meant when you said "MyRequest::self"), but I don't a way to do that without copy-pasting the original implementation into the subclass and tweaking it. That is what I consider very bad Ruby practice.

    I strongly urge you to reconsider your decision, or at least ask for another pair of eyes on this among your colleagues.

    Thanks

  • manveru

    manveru March 30th, 2009 @ 01:37 AM

    • State changed from “invalid” to “open”

    OK, my bad, shouldn't answer tickets at 3am. I guess I'm for this change after all, let's get a patch going.

  • manveru

    manveru March 31st, 2009 @ 06:34 AM

    • Milestone set to “1.0”
    • State changed from “open” to “hold”

    So the previous patch was wrong, here's a working one. If there are no further complaints or suggestions I'll push that one this week or when i get another +1.

  • Ryan Tomayko

    Ryan Tomayko March 31st, 2009 @ 08:19 AM

    • State changed from “hold” to “open”

    +1

    Also, have I mentioned that I hate Request.new memoization? Just want to get that in there.

  • manveru

    manveru March 31st, 2009 @ 04:43 PM

    • State changed from “open” to “resolved”

    Thank you, pushed.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

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

People watching this ticket

Attachments

Pages