patternphpMinor
PHP MVC - Custom Routing Mechanism
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
```
# -------------------------------------------------------------------------
# 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
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.:
and
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.
The next thing I notice is the use of static methods, e.g.:
\Models\Session::checkSession()and
Config::$defaultLoggedInControllerWhich 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::$defaultLoggedInControllerContext
StackExchange Code Review Q#26345, answer score: 3
Revisions (0)
No revisions yet.