FIXED: Issue where route table arguments would not be accessible without using non deprecated API

[tractorcow]

This pull request is for a "fix" for a gap in the api.

Previously any arguments specified on the routing table would be saved in Director::urlParams() and could be retrieved by the controller. For example, my routes.yml file looks like the below:

---
Name: translatedroutes
After: framework/routes#rootroutes

---
Director:
  rules:
    'en_NZ/$URLSegment!//$Action/$ID/$OtherID':
      Controller: 'ModelAsController'
      Locale: 'en_NZ'

In 2.4 the "Locale" parameter would be available, but trying to access this in SS 3.0 via Director::urlParams() gives a deprecation error, noting that SS_HTTPRequest->latestParams() should be used instead. Since only arguments in the url pattern (URLSegment, Action, etc) are actually stored in the request object, this actually represents a degradation in functionality.

My fix is to ensure that during routing these additional parameters are correctly passed to the request object and stored, so that they may be retrieved via $this->request->params().

Note: This pull request is based on a previous attempt to solve this problem which had a few issues involved with it. Hopefully this implementation resolves those issue. :)

The original pull request can be found at #711 for reference.

This implementation of the problem differs from the original in that it:

• Does not interfere with the HTTP_Request::match function
• Correctly separates the url parameter arguments (specified directly in the url) from the route table arguments.
• Updateds the behaviour of HTTP_Request::param and ::params fuctions to consider both types of parameters
• Includes (working) test cases ^_^

[travisbot]

This pull request passes (merged c2a8eec into 0a6a3fa).

[travisbot]

This pull request passes (merged 9b6216d into 0a6a3fa).

[sminnee]

This looks good to me but I'd like @hafriedlander to give it a look over.

[hafriedlander]

Hi,

In general this looks good. I don't like the additional argument to Director::test to get the created request out - adding another argument just for that one test seems ugly. Not sure what the alternative is though.

[tractorcow]

I personally feel that any "testing" code should stay in the unit tests themselves, but I was hesitant to change the existing model without a bit of direction in this. My goal was to update the treatment of arguments during routing, not change the testing dynamic.

I'd be happy to refactor Director::test into the unit test themselves, or having a dedicated Director testing subclass, but I feel that this is outside the scope of this pull request personally.

It's another case of "if it's difficult or ugly, it's probably wrong". Suggest that we make it a solution to find later?

[sminnee]

We could probably refactor it like so:

• A function along the lines of SS_HTTPRequest::create() (or the SS_HTTPRequest constructor) would build a "test" request as per the code of Director::test().

• Director::direct() could accept an SS_HTTPRequest as input and return SS_HTTPResponse as output.

• Director::test() would be deprecated and become:

  return self::direct(new SS_HTTPResponse($a, $b, c));
  

The non-deprecated test call-path could then be:

$response = Director::direct(new SS_HTTPRequest($a, $b, $c, $d));

And the main.php call could be this:

$req = SS_HTTPRequest::create_from_server($url);
Director::direct($req)->output();

However, I think we can merge this change and leave my suggested refactoring for another pull?