Klein v2

[Rican7]

Klein - Version 2

Here we go, @chriso...

Here it is, the Version 2 rewrite of Klein.php.

Its been a long time coming: many commits, thousands of lines, and a lot of unit tests. :P

The biggest differences are:

• The modern OOP pattern to the code
• The PSR-2 compliance
• The PSR-0 autoloader compatibility
• The more modular codebase
• The heavily documented code (with PHPdoc style documentation)
• The dependency injection approach to the different Request, Response, and Service handling classes
• The more testable approach to also injecting the request data that is normally contained in the super-globals (your idea!)
• The namespaced, contained code that keeps the global scope clean and relies less on static variables
• The use of custom exceptions that extend the SPL (standard library) for a more consistent exception handling pattern
• The unit tested and "integration tested" code
• The Symfony inspired classes that have no external dependencies

The routing algorithm itself has not been changed. Klein 2's efficiency therefore hasn't been reduced thanks to PHP's handling of objects (they're inexpensive), and the easily APC-cacheable classes (that depend less on the super-globals).

The code was designed to satisfy more of the "Laws of Demeter" and the "Single Responsibility Principle", in that each class handles its own objects and simply interfaces with its related classes without ever directly operating on each other's properties (most of the properties are behind getters/setters).

The use of Klein has also not changed much, other than the lack of the ability to call Klein's methods from anywhere (since the scope isn't global). This is its strength, though, as it allows multiple instances of Klein (great for testing) and a less dirty global scope with less of a chance to accidentally change Klein's intended behavior at runtime.

Anyway

Let me know what you think! You won't hurt my feelings if you don't like something. 👅 Merging this code into master isn't nearly as scary of an idea now that we have well tagged versions. And after a merge, the code can be tagged as a major version (2.0.0), so that dependency managers won't automatically update!

[kafene]

Very nice looking.

Just a few thoughts:

instead of nested try catch blocks you can do:

try {
    // do something
} catch(LockedResponseException $e) {
    // do nothing
} catch(Exception $e) {
    $this->error($e);
}

or possibly

try {
    // do something
} catch(Exception $e) {
    switch(gettype($e)) {
        case 'LockedResponseException': break;
        default: $this->error($e);
    }
}

or even in error() just check if the class is LockedResponseException (or in a new class variable - an array of 'pass exceptions' to be more modular), and return.

Add support for PHP 5.4 function http_response_code and let the engine intelligently handle sending the response code, and then fall back to Response::httpStatusLine().

I had some notion about abstracting the concept of a locked response out of the response class itself, and using some kind of read-only variable class that threw exceptions on __set, but that's probably overkill.

The prepend method can be shortened to:

return $this->body($content.$this->body);

Though that's only really stylistic.

Also not certain about protocolVersion() - this can usually be detected from $_SERVER['SERVER_PROTOCOL']; and I don't know why anybody would want to manually set it to something different. Changing the value, of course, does not actually change the server protocol. I am not sure if it has any effect at all, since you can send a status header by just doing header(':', true, $code) and it will work fine.

And also an early bird feature request, in the validators, add a remote_ip validator that checks for FILTER_VALIDATE_IP with the flags FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE set.

Last thing I can see right now:

Get the php://input once

Since it can only be got once per request, maybe make the variable static, so even if there are multiple instances of Request, they can all access it.

Thanks for your work It is quite well formatted and easy to understand code.

[Rican7]

@kafene Thanks for the suggestions!

• Great call on the exception catch style. I'll definitely do it! I'd much rather prefer your first suggestion over the other styles, personally.
• Good call! I ran into some problems using "http_response_code" earlier, but I could probably replace my header($this->httpStatusLine()); with it. I'll give it another try. :)
• I think the idea of a read-only variable class is abstracting a class' property a bit too much, especially for a routing library (its maybe more appropriate for an all-out framework), IMO.
• The results of the prepend/append can absolutely be achieved through other means. Its more of a nice convenience method than anything else. And yea, that's a stylistic thing more than anything. :P
• There's a reason that Symfony and Slim both use a custom protocol version variable/property. I'm not gonna pretend I know better. :P
• An IP validator is a great idea! I could add it. :)
• Static variables are evil, haha. But yea, you have a point. It just makes testing a pain and adds a global state (code smell). Butttt, sometimes statics are a necessity.

Thanks for your comments and compliments! They're greatly appreciated! Thank you so much!

[Rican7]

So, what do you think @chriso?

[chriso]

Hey Trevor,

All good. I still think that the Request / Response instances should be injected in the constructor but we can talk about that post-merge.

Again, thanks for all the work you've done.

Merging now..

[Rican7]

Woot! Thanks! This has been a long time coming! :)

Yea, we can definitely hash out the details through separate issues/pull-requests, instead of having this giant thread.

Thank you SO much! I'm honored to have been able to contribute to this project!

👍