Most modern framework extend beyond the simple MVC pattern to also include object-oriented constructs representing the request and response and oftentimes a separate router class. I think that is something your framework could also benefit from, as right now, you have a mix of request management and routing functions in your App class that really doesn't fully achieve either (for example if you have a proper request object, your controller wouldn't need to bee inspecting $_SERVER['REQUEST_METHOD'] - this info should already be determined before a controller is even invoked.
So perhaps this would give you a program flow like this:
- include bootstrap file.
- instantiate and build request object.
- pass request object to router, which determines controller to be used.
- router injects request object into controller and passes program control to controller.
- controller loads/interacts with model.
- controller build s model and/or other meaningful data into a response object which it passes to view for rendering.
You should strongly consider using a PSR-4 compliant autoloader. This can move you out of the world of having to write a lot of code around locating and loading class dependencies.
You may consider moving towards a dependency injection approach to your framework. Right now, your framework exhibits poor encapsulation and division of responsibility, which is something that proper instantiation and passing of dependencies could help to address. For example, right now, pretty much your whole application logic boils to to the App constructor method. You get to the point of calling this method before:
- you know if you have a properly formatted request
- you know if the request is made against a valid route.
By this point, if you have a problem with the request or route, you would need a lot of code in this constructor to handle these cases, something you do a poor job of in this method. Walk through your code to see what happens if I sent a URL like someBadController/someBadMethod. You would actually try to instantiate new user and call main() on it!
If you went with a program flow like I presented above, you could:
- Determine if request is valid. If not fail out (perhaps to standard error controller) quickly. If valid, instantiate request object and inject into router.
- Determine if the passed request object maps to a valid controller. If not fail out (again perhaps to standard error controller with 400-series error). If route is found, instantiate the appropriate controller and pass it the request object.
- Controller could inspect the injected request object to determine method that is being called, get user input, etc.
Should Controller base class be abstract? You are not ever expecting to instantiate it right?
Why is there model() method on Controller()? I would think any inheriting controller would be built with an explicit understanding of what model it interacts with. So rather than do things in inheriting class like:
$this->model('Users');
Why would you not just have a class constant that stores the model reference? What value is the method adding?
I think it strange to have a controller and model class name that is so similar. That could be really confusing. Consider UserController and User (singular since you are only representing a single user here right?)
I would think your JSON view should be setting appropriate application\json header. I also think it should not have any responsibility for changing the data structure (like you are doing by adding response key. This should be done by the controller/model and passed to the view so you are not splitting your data structure business logic up into different parts of the app.
If you take the approach mentioned above around building a response object into your framework and using dependency injection, you might instead make your views into a proper class that can take response dependency and perform all header setting and rendering actions specific to the passed response content.
For example in controller:
// could in one of your action methods
$response = new Response($model);
$view = $this->view($response);
$view->render();
// code to return a view object
public function view($response) {
// perhaps you have some logic here to determine kind of
// view object to build
return new JsonView($response);
}
In this case, perhaps the response object need to implement JsonSerializable interface to provide instructions on how to serialize data to JSON.
By thew way, the more of your framework object you make into classes, the less require, file_exists(), etc. type of logic you need in your code if you use a proper autoloader.
What values does nesting everything under response key add anyway? You are just adding an unnecessary level of nesting to the response data structure. The caller already knows they are getting a response :)
You should probably have a proper model base class that User and other models in your system can inherit from. I know you said this part wasn't complete, but you are showing nothing here to indicate that you are considering this.
You will find that a well-implemented base class will save a lot of code in the inheriting classes.
You would also move away from class-specific naming for your CRUD methods. These methods should just be add/create, get, delete, update and should be overridden as necessary in inheriting classes. You may also find that you have need for static/factory type methods on you model classes.
abstract class Model
{
const TABLE;
const ID;
protected $fields = [];
private $pdo;
// some methods that could be called in static context
public static getById(int $id, PDO $pdo)
{
$fields = '`' . implode('`, `', $this->fields) . '`';
$sql = "
SELECT {$fields}
FROM `{static::TABLE}`
WHERE `{static::ID}` = :id
";
// prepare and execute query
// assume we read that data into $data
return new static($data, $pdo);
}
public static deleteById(int $id, PDO $pdo)
{
$sql = "
DELETE FROM {static::TABLE}
WHERE {static::ID} = ?
";
// prepare and execute query
}
// perhaps some method to create new object from array of data
// that you want each inheriting class to implement
abstract static function create(array $data, PDO $pdo);
// perhaps some other methods you want classes to implement in operate
// against concrete objects
abstract function update();
// etc.
}
With an inheriting User class perhaps looking like:
class User inherits Model
{
const TABLE = 'users';
const ID = 'id',
protected $fields = [
'name',
'email',
'password',
// etc.
];
// you could make these private with getters/setters if more appropriate
public $name;
public $email;
public $password;
// etc.
public function __construct(array $data, PDO $pdo)
{
// shown without validation for demo purposes
foreach($array as $key => $value) {
$this->{$key} = $value;
}
$this->pdo = $pdo;
}
public static function create(array $data, PDO $pdo)
{
// insert record and upon success
// return new instance
return new User($data, $pdo);
}
// this implementation could possibly live in base class, but showing
// here for demo purposes
public function update()
{
$fields = '';
foreach($this->fields as $field) {
$fields .= "`{$field}` = :{$field}";
}
$sql = "
UPDATE `{self::TABLE}`
SET {$fields}
WHERE {self::ID} = :id
";
// prepare query
// execute prepared statement with bound values from object properties
// return success/failure boolean
}
// etc.
}
This is really just here for informational purposes. I would suggest you look at common frameworks out there to drawn further inspiration around structuring your model classes.
I am concerned that you seem to be looking at a JSON-based REST API, but you have made no provisions at all for reading JSON as input. Your API seems to assume a form-encoded content type being sent to where $_GET, $_POST would be populated as it is in a standard form-based web application. If a REST client is sending your GET/POST with application/json content type, PHP will not auto-populate these superglobal variables. You will need to read input manually from standard input using something like:
$json = file_get_contents('php://input');
I guess this is similar to my earlier thoughts around request/response objects. I think your framework is currently quite naive around aspects of reading and setting HTTP headers, something that can be critical when trying to build a RESTful API framework. What you will likely find is that there might need to be quite a bit of logic around this, such that building these separate classes becomes mandatory in order to encapsulate this logic and not have it scattered throughout the other classes in your framework. Again you may want to look at your favorite framework(s) for inspiration in this area.
public function main($params){
switch($_SERVER['REQUEST_METHOD']){
case 'GET':
$this->get_user();
break;
case 'POST':
$this->add_user();
break;
case 'PUT':
$this->update_user();
break;
case 'DELETE':
$this->delete_user();
break;
}
}
I think this approach of mapping your request to an action in the controller may not scale well over time unless you only have to simplest of database models you are trying to make available via the API.
You will inevitably find that the paradigm of one URI to one set of actions in a model will tend to break down for even the simplest applications. Let's use an example use case of an application where you have a post (like a blog) post and O to n comments that can be made against a post.
When you go to serve up the data from your API, you may find you need convenience methods to allow the caller to get the full post as well as all comments with one API call, such that client doesn't need to get the post data and then make a series of follow-up API calls to get the data for each comment related to the post.
So you might decide to expose an endpoint like:
/posts/{id}/comments
How is that handled with your routing mechanism?
Oftentimes you may need to look at a combination of request method and URI (not just request method) to determine an appropriate controller and/or action on controller to call.
This ties back to my suggestion on having a request object (which would store request method) that can be passed to a dedicated router class. That router should be able to inspect the combination of URI and request method to determine what controller should be invoked.
Some routing strategies may have you just looking at first URI segment to route to a primary controller, which may then invoke subcontrollers to handle more complex routing/method requests, where some routing strategies may have router actually hold all controller/subcontroller mapping logic.
Which approach to take may depend on how you want to use the framework and your personal preferences, but just keep in mind that you will inevitably encounter this kind of use case and likely need to build out more robust routing logic.
