3
\$\begingroup\$

I have put together the back-end (API) with the Slim framework (v3) and MySQL.

enter image description here

In index.php I have:

use \Psr\Http\Message\ServerRequestInterface as Request;
use \Psr\Http\Message\ResponseInterface as Response;

require '../vendor/autoload.php';
require '../src/config/db.php';

$app = new \Slim\App;

// Todos Routes
require '../src/routes/todos.php';

$app->run();

In db.php I have:

class db{
    // Properties
    private $dbhost = 'localhost';
    private $dbuser = 'root';
    private $dbpass = '';
    private $dbname = 'todoapp';

    // Connect
    public function connect(){
            $mysql_connect_str = "mysql:host=$this->dbhost;dbname=$this->dbname";
            $dbConnection = new PDO($mysql_connect_str, $this->dbuser, $this->dbpass);
            $dbConnection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            return $dbConnection;
    }
 }

In todos.php I have:

use \Psr\Http\Message\ServerRequestInterface as Request;
use \Psr\Http\Message\ResponseInterface as Response;

$app = new \Slim\App;

$app->options('/{routes:.+}', function ($request, $response, $args) {
    return $response;
});

$app->add(function ($req, $res, $next) {
    $response = $next($req, $res);
    return $response
            ->withHeader('Access-Control-Allow-Origin', '*')
            ->withHeader('Access-Control-Allow-Headers', 'X-Requested-With, Content-Type, Accept, Origin, Authorization')
            ->withHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, OPTIONS');
});

// Get Todos
$app->get('/api/todos', function(Request $request, Response $response){
    $sql = "SELECT * FROM todos";

    try{
        // Get DB Object
        $db = new db();
        // Connect
        $db = $db->connect();

        $stmt = $db->query($sql);
        $todos = $stmt->fetchAll(PDO::FETCH_OBJ);
        $db = null;
        echo json_encode($todos);
    } catch(PDOException $e){
        echo '{"error": {"text": '.$e->getMessage().'}';
    }
});

// Add Todo
$app->post('/api/todo/add', function(Request $request, Response $response){
    $title = $request->getParam('title');
    $completed = $request->getParam('completed');

    $sql = "INSERT INTO todos (title, completed) VALUES (:title,:completed)";

    try {
        // Get DB Object
        $db = new db();
        // Connect
        $db = $db->connect();

        $stmt = $db->prepare($sql);

        $stmt->bindParam(':title', $title);
        $stmt->bindParam(':completed',  $completed);

        $stmt->execute();
        echo '{"notice": {"text": "Todo Added"}';

    } catch(PDOException $e){
        echo '{"error": {"text": '.$e->getMessage().'}';
    }
});

// Update Todo
$app->put('/api/todo/update/{id}', function(Request $request, Response $response){
    $id = $request->getAttribute('id');
    $title = $request->getParam('title');
    $completed = $request->getParam('completed');

    $sql = "UPDATE todos SET
                title   = :title,
                completed   = :completed WHERE id = $id";

    try{
        // Get DB Object
        $db = new db();
        // Connect
        $db = $db->connect();

        $stmt = $db->prepare($sql);

        $stmt->bindParam(':title', $title);
        $stmt->bindParam(':completed',  $completed);

        $stmt->execute();

        echo '{"notice": {"text": "Todo Updated"}';

    } catch(PDOException $e){
        echo '{"error": {"text": '.$e->getMessage().'}';
    }
});

// Delete Todo
$app->delete('/api/todo/delete/{id}', function(Request $request, Response $response){
    $id = $request->getAttribute('id');

    $sql = "DELETE FROM todos WHERE id = $id";

    try{
        // Get DB Object
        $db = new db();
        // Connect
        $db = $db->connect();

        $stmt = $db->prepare($sql);
        $stmt->execute();
        $db = null;
        echo '{"notice": {"text": "Todo Deleted"}';
    } catch(PDOException $e){
        echo '{"error": {"text": '.$e->getMessage().'}';
    }
});

Questions/concerns:

  1. Is the application well-structured or should I move the logic into controllers?

  2. If I should move the logic into controllers, what would be the best approach to doing that?

Post scriptum

I have added the front-end of the application here for those that might be interested.

\$\endgroup\$
2
  • \$\begingroup\$ I have added the front-end of the application here if you're interested :) \$\endgroup\$ Commented May 29, 2021 at 7:54
  • \$\begingroup\$ Why do you catch all PDO exceptions? Why don't you implement an error handler for all exceptions or just use the built-in error handler? \$\endgroup\$ Commented Jun 8, 2021 at 14:02

2 Answers 2

Reset to default
3
\$\begingroup\$

Is the application well-structured or should I move the logic into controllers?

I agree with NemoXP that the current format is not well-structured. A file with 112 lines of code isn't horrible in terms of file length, but it handles multiple things - e.g. routing, updating models, etc. Separating the code out to controller methods would be wise - especially if multiple developers end up modifying the files.

If I should move the logic into controllers, what would be the best approach to doing that?

As NemoXP stated: a controller would be a good solution for moving the logic out of the router - e.g. TodoController, with a method for each route. That could have methods to abstract common tasks like returning the JSON, exception handling, etc.

Suggestions

Avoid re-assignment

    // Get DB Object
    $db = new db();
    // Connect
    $db = $db->connect();

With the first assignment $db is assigned an instance of class db. Then in the next assignment that same variable is assigned the return value from the method connect, which is an instance of PDO.

I likely mentioned in a review of your JavaScript code that it is wise to use const instead of let to avoid accidental re-assignment. The concept applies here as well - over-writing a variable can make it difficult to "reason" about code if a variable changes type.

A better name for the variable in the second assignment would be something that illustrates that it is a connection, not a database - something like $connection, $conn, etc.

Limit data returned

$sql = "SELECT * FROM todos";

For a small application this likely isn't an issue, especially if there are typically a small number of records and only a few columns. Problems can occur when:

  • the table grows to have many columns - of course it isn't ideal but in real life it does happen. It is best to specify only the columns needed instead of *
  • the number of rows grows to more than is necessary. This can lead to memory issues. It is best to limit the number of results instead of selecting all records
  • the data isn't filtered- perhaps for this application there isn't a need to filter the data but in a larger application, it would be wise to add some conditions to limit the data returned.

Storing credentials and other details

private $dbhost = 'localhost';
private $dbuser = 'root';
private $dbpass = '';
private $dbname = 'todoapp';

These are things that should not be stored in a repository. It is wise to store them in a file that is ignored by the VCS - e.g. .env files, which can be included with packages like phpdotenv.

Response Type

It appears that all routes return strings that are to be interpreted as JSON. In such cases it is appropriate to add a header to describe the Content-Type. While it likely isn’t a security hole anymore, at one point ~15 years ago an XSS attack may have been possible if content-type headers weren’t set.

$response->withHeader('Content-type', 'application/json');

Presuming all routes have that same type, the header could be added with the other headers (e.g. Access-Control-Allow-*).

JSON String construction

In the route to get all TODO items json_encode() is used to convert the list to JSON format. Yet in other cases, included the catch blocks, a JSON object is created manually - e.g.

  echo '{"error": {"text": '.$e->getMessage().'}';

This could be simplified using json_encode()

   echo json_encode('["error" => ["text” => $e->getMessage()]]];

The benefit here is no risk of the exception message breaking the string literal - e.g. if it contained a delimter character like ” or }. Actually, now that I think of it, the original line could lead to a JavaScript error because the message is not surrounded by double quotes!

\$\endgroup\$
1
  • \$\begingroup\$ I have added the front-end of the application here if you're interested :) \$\endgroup\$ Commented May 29, 2021 at 7:54
4
\$\begingroup\$
  1. In short, no. Actually doesn't seem to be structured in any way :D
  2. If your API will be just that, you can even give up on slim and have a plain index.php with url rewrite so you can achieve the best performance.

Few things that I recommend:

  • first, I would try to use a DI (dependency injection). I like php-di ( https://php-di.org/ )
  • next would be to try to go with an mvc structure where the view is actually a JSON output: have some classes, TodoController, and multiple public methods (one for each action/route). Another approach would be to use Actions, as in symfony (basically would mean to have separate class files for each route: AddTodoAction.php,DeleteTodoAction.php).
  • the, you should group your files and here we can debate a lot. I like to have a "Library" folder and inside that have one folder per each module and each module has it's own grouping (Controller / Model / Dao / Processor / Validator / etc. *). It's some sort of mix between having all controllers inside a single folder and grouping by business logic. If you only have Todo's without any users or any other relations, you can have a generic Controller/Action folder and keep all controllers/actions inside that folder.
  • ideally to also use a orm for communicating with the database, or at least use bindings for all inputs ($id should also be considered an input). I use eloquent with slim ( https://laravel.com/docs/8.x/eloquent ).
  • have any sort of validation before actually using the input
  • try and reuse the $response object that slim is already providing and avoid direct echo-ing the output as so.

You can also try to have a look over this: https://github.com/gurkanbicer/slimmvc

Extra: have you tried slim 4.8? Why start a new project on such an old version?

*: folder structure example:

config/
    bootstrap.php
    routes.php
    settings.php
public/
    index.php
src/
    Core/ (here I keep common middlewares for authentication, health checks or other things, maybe a way to format arrays / objects to json to output them properly)
    Library/
        Todo/
            Action/
                AddTodoAction.php (called from routes.php and should only validate input, call a method from service and the output the result)
                DeleteTodoAction.php
                ListTodoAction.php
            Model/
                Todo.php
                TodoDao.php (can keep it here or in a separate namespace: Src/Library/Todo/Dao/TodoDao.php )
            Validator/ (can go with same approach as for controller vs action: a single class for all routes or multiple classes separated per action / business logic)
                AddTodoValidator.php
                TodoValidator.php
            TodoService.php (the service is the only class that I call from other services. It has methods for controller, example: getTodo(), and it calls a DAO class for communication with database / execute queries)
tests/
    ... (if any)
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.