HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

PHP MVC - Custom Routing Mechanism

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mvcphpcustommechanismrouting

Problem

Note #2: This was a post from a while back. Although the top rated answer is useful (generally), I'm looking for something more specific to this issue including routing / architecture. I've even tried a bounty, which failed. As I don't have enough rep to keep adding bounties, I'll leave it open until someone decides to post :)

Note #1: My code currently works well and I've progressed further than this - however I feel that this router is, frankly, crap, and I'm looking for honest opinions and constructive criticism.

I'm in the process of creating my own framework for educational purposes, and for fun! This question pertains to the routing of my application using the front controller pattern.
.htaccess

Every request hits my .htaccess file which, using Apache's mod_rewrite, routes all requests through to my index.php file using ?url=.

```

# -------------------------------------------------------------------------
# This file forwards all requests through to index.php as long as the
# request is not for a file or directory. This uses ?url and index.php
# internally, so any requests including these add an __iserror=1 which the
# framework picks up in the routing and redirects to a 404 error page.
# -------------------------------------------------------------------------

# Turn mod_rewrite on (please make sure it's enabled in Apache)
RewriteEngine On

# Block direct access to "index.php" and query strings with "?url="/"&url="
RewriteCond %{QUERY_STRING} !^__iserror=1$
RewriteCond %{THE_REQUEST} ^(\S+)\ +/(\S+)\/index\.php\b [OR]
RewriteCond %{THE_REQUEST} (?:&|\?)url=
RewriteRule .* /framework/index.php?__iserror=1 [L]

# As long as we're not requesting access to a file...
RewriteCond %{REQUEST_FILENAME} !-f
# And we're also not requesting access to a directory...
RewriteCond %{REQUEST_FILENAME} !-d
# Route everything through /framework/index.php
RewriteRule .* /framework/index.php?url

Solution

The first thing I notice is that you are using the superglobals directly in your class. Normally you want to inject a request object into the class which contains all the info about the request.

The next thing I notice is the use of static methods, e.g.:

\Models\Session::checkSession()


and

Config::$defaultLoggedInController


Which not only makes you method / class hard to test, but also hard to maintain both for somebody else as for the future you. It makes it very hard to see the class / method has those dependencies by looking at the signatures.

I think if you leave the request parsing to your request object you not only clean up this method / class, but you are also following SRP better.

In general it would be nice to GRASP SOLID. To improve both readability and maintainability.

Code Snippets

\Models\Session::checkSession()
Config::$defaultLoggedInController

Context

StackExchange Code Review Q#26345, answer score: 3

Revisions (0)

No revisions yet.