Help with Slim application

[NotionCommotion]

I have the following application where a DELETE HTTP request method to url http://example.com/documents/123 will delete document 123 of the account corresponding to the key sent in $_SERVER['HTTP_X_KEY'].

//index.php

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

require '../vendor/autoload.php';
spl_autoload_register(function ($classname) {
    require ("../classes/" . $classname . ".php");
});

$config=parse_ini_file('../../config.ini',true);
$config=[
    'db_sql'=>$config['mysql']
];

$app = new \Slim\App(["settings" => $config]);

$container = $app->getContainer();

$container['logger'] = function($config) {
    $logger = new \Monolog\Logger('my_logger');
    $file_handler = new \Monolog\Handler\StreamHandler("../logs/app.log");
    $logger->pushHandler($file_handler);
    return $logger;
};
$container['db'] = function ($config) {
    try {
        $db = $config['settings']['db_sql'];
        return new PDO("mysql:host={$db['host']};dbname={$db['dbname']};charset={$db['charset']}",$db['username'],$db['password'],array(PDO::ATTR_EMULATE_PREPARES=>false,PDO::MYSQL_ATTR_USE_BUFFERED_QUERY=>true,PDO::ATTR_ERRMODE=>PDO::ERRMODE_EXCEPTION,PDO::ATTR_DEFAULT_FETCH_MODE=>PDO::FETCH_ASSOC));
    }
    catch(PDOException $e){
        header("HTTP/1.1 503 Service Unavailable. MySQL database is down.");
        exit(json_encode(["status"=>"error","code"=>2,"message"=>"Unable to communicate with database."]));
    }
};

$app->delete('/documents/{documents_id}', function (Request $request, Response $response) {
    $documents=new documents(['logger'=>$container['logger'],'db'=>$container['db']]);
    return $response->withJson($documents->delete($request->getAttribute('documents_id'),$request->getParsedBody()));
});

$app->run();

class main
{
    protected $db, $logger, $account;

    public function __construct($settings)
    {
        foreach($settings as $key=>$value) {
            $this->$key=$value;
        }
        if(empty($_SERVER['HTTP_X_KEY'])){
            http_response_code(422);
            exit(json_encode(["status"=>"error","code"=>3,"message"=>"Missing key."]));
        }
        else {
            $stmt=$this->db->prepare('SELECT id, timestamp FROM accounts WHERE key=?');
            $stmt->execute([$_SERVER['HTTP_X_KEY']]);
            if(!$this->account=$stmt->fetch(PDO::FETCH_OBJ)){
                http_response_code(422);
                exit(json_encode(["status"=>"error","code"=>3,"message"=>"Invalid key."]));
            }
        }
    }
}

class documents extends main
{
    public function delete($documents_id,$params)
    {
        /*
        Delete document based on $documents_id and $this->account->id
        Set headers as appropriate
        return JSON
        */
    }
}

I wish to change this, and instead of having just class "documents", have several classes based on the type of the document (i.e. drawings, specifications ,etc), and have those classes extend the parent documents class.  I have a table "documents" which contains:

• id (PK)
• accounts_id
• documents_id (this is the id sent from the client, and is unique for a given accounts_id)
• type (i.e. drawings, specifications ,etc.  This is what determines the desired class)

 

My first thought was to within index.php query the documents table to determine the type of document based on the received documents_id and the accounts_id, but I don't know the accounts_id until the classes constructor is executed.

 

I am thinking maybe I should be using middleware for authentication and determining the accounts_id, but am not really sure.

 

Any recommendations?

[Jacques1]

So this is the non-fictional version of your previous question?

 

I think this is a deeper problem of your class design, because a lot of this doesn't make much sense:

• What exactly does the “main” class represent? It seems to be a controller, a model and an authentication helper all at the same time, which is probably why it has such a vague name. Use separate classes for separate tasks.
• Why do you need multiple classes to represent the same table? What's the fundamental difference between the document types?
• Your constructors are weird. The sole purpose of a constructor is to initialize the object. It does not send JSON documents around or communicate with the client. If there's an error, you throw an exception. You can then turn that exception into anything you want on a higher level.

[NotionCommotion]

• Regarding "main" class, yea, it does seem that way.  Slim acts as the controller, "documents" is a model with a bit of logic, and "main" adds some methods used by "documents" and other similar models.

They are different tables.  I will have a super-type table "documents" with 1-to-1 relationship to sub-type tables "drawings" and "specifications".   Deleting might be the same, but other tasks such as editing are different.

The constructor was being used for authentication.  I've since changed, and will use what I think is the "slim way" as following.

$app->add(function ($request, $response, $next) {

    if(empty($_SERVER['HTTP_X_KEY'])){

        http_response_code(422);

        exit(json_encode(["status"=>"error","code"=>3,"message"=>"Missing key."]));

    }

    else {

        $stmt=$this->db->prepare('SELECT id, timestamp FROM accounts WHERE key=?');

        $stmt->execute([$_SERVER['HTTP_X_KEY']]);

        if(!$this->account=$stmt->fetch(PDO::FETCH_OBJ)){

            http_response_code(422);

            exit(json_encode(["status"=>"error","code"=>3,"message"=>"Invalid key."]));

        }

    }

    $response = $next($request, $response);

    return $response;

});

[NotionCommotion]

I think I just need a factory!

[ignace]

 

$app->add(function ($request, $response, $next) {
    if(empty($_SERVER['HTTP_X_KEY'])){
        http_response_code(422);
        exit(json_encode(["status"=>"error","code"=>3,"message"=>"Missing key."]));
    }
    else {
        $stmt=$this->db->prepare('SELECT id, timestamp FROM accounts WHERE key=?');
        $stmt->execute([$_SERVER['HTTP_X_KEY']]);
        if(!$this->account=$stmt->fetch(PDO::FETCH_OBJ)){
            http_response_code(422);
            exit(json_encode(["status"=>"error","code"=>3,"message"=>"Invalid key."]));
        }
    }
    $response = $next($request, $response);
    return $response;
});

 

I think the main thing you need is starting to learn/read the manual. Why use a framework if you are writing plain PHP anyway?

 

$app->add(function(ServerRequestInterface $request, ResponseInterface $response, $next) use ($container) {
  $key = $request->getHeaderLine('X-Key');

  if (empty($key)) {
    return new MissingKeyResponse(); // <-- correct format across the entire application.
  }

  $account = $container->accountRepository->findOneByKey($key);

  if (!$account) {
    return new InvalidKeyResponse(); // <-- correct format across the entire application.
  }

  return $next($request, $response);
});

Edited August 23, 2016 by ignace

[NotionCommotion]

Thanks Ignace,

 

Why have use ($container)? Isn't the container in $this?

 

I like how you return the error response and just terminate performing the next route (better than my exit).  Note related to Slim, but why do you use individual classes presumably just with a constructor for each error?  Would it be better to have a single class and pass something to the constructor, or maybe use a static class?

[NotionCommotion]

I was getting an error, and changed $app->add(function(ServerRequestInterface $request, ResponseInterface $response, $next) use ($container) { to $app->add(function(Request $request, Response $response, $next) use ($container) {.  I also got rid of the  use ($container), however, doing both ways seemed to work find.  Any issues with me doing so?

 

All seems to work out until I have a invalid key, and am getting the below error.  Do you know what I am doing wrong?

 

$container['account'] = function () {
    return new \MyApp\Accounts();
};

$app->add(function(Request $request, Response $response, $next) {
  $key = $request->getHeaderLine('X-MyApp-Key');
  if (empty($key)) {
    return \MyApp\ErrorResponse::missingKey();
  }
  
  $account=$this->account;
  $account = $account->get($this->db,$key);
  
  if (!$account) {
    return \MyApp\ErrorResponse::invalidKey();
  }
  $this->account=$account;

  return $next($request, $response);
});

namespace MyApp;
class ErrorResponse
{
  public static function missingKey()
  {
    return ["status"=>"error","code"=>3,"message"=>"Missing key."];
  }
  public static function invalidKey()
  {
    return ["status"=>"error","code"=>3,"message"=>"Invalid key."];
  }
}

namespace MyApp;
class Accounts
{
  public function get($db,$key)
  {
    $stmt=$db->prepare('SELECT id,name FROM accounts WHERE main_key=?');
    $stmt->execute([$key]);
    return $stmt->fetch(\PDO::FETCH_OBJ);
  }
}

Slim Application Error
The application could not run because of the following error:


Details


Type: UnexpectedValueException
Message: Middleware must return instance of \Psr\Http\Message\ResponseInterface
File: /var/www/src/vendor/slim/slim/Slim/MiddlewareAwareTrait.php
Line: 69
Trace


#0 /var/www/src/vendor/slim/slim/Slim/MiddlewareAwareTrait.php(116): Slim\App->Slim\{closure}(Object(Slim\Http\Request), Object(Slim\Http\Response))
#1 /var/www/src/vendor/slim/slim/Slim/App.php(337): Slim\App->callMiddlewareStack(Object(Slim\Http\Request), Object(Slim\Http\Response))
#2 /var/www/src/vendor/slim/slim/Slim/App.php(298): Slim\App->process(Object(Slim\Http\Request), Object(Slim\Http\Response))
#3 /var/www/src/public/index.php(341): Slim\App->run()
#4 {main}

 

[kicken]

Message: Middleware must return instance of \Psr\Http\Message\ResponseInterface

Slim is expecting your function to return an object implementing \Psr\Http\Message\ResponseInterface. In the case of a missing or invalid key though, you're just returning an array.

 

What you need to do is use that array to create a response then return that response, such as via the $response->withJson method as you've done in other bits of code.

 

$app->add(function(Request $request, Response $response, $next) {
  $key = $request->getHeaderLine('X-MyApp-Key');
  if (empty($key)) {
    return $response->withJson(\MyApp\ErrorResponse::missingKey(), 401);
  }
  
  $account=$this->account;
  $account = $account->get($this->db,$key);
  
  if (!$account) {
    return $response->withJson(\MyApp\ErrorResponse::invalidKey(), 401);
  }
  $this->account=$account;

  return $next($request, $response);
});

[NotionCommotion]

Slim is expecting your function to return an object implementing \Psr\Http\Message\ResponseInterface. In the case of a missing or invalid key though, you're just returning an array.

 

What you need to do is use that array to create a response then return that response, such as via the $response->withJson method as you've done in other bits of code.

 

 

Daaahh...

 

Must I say more?  Thanks!!!

[ignace]

I have never used Slim before, I have however dabbled with StackPHP. But from reading the documentation

I was able to concoct the above example.

 

Regarding ErrorResponse::missingKey() and new MissingKeyErrorResponse() I was of course referring to a parent object containing the

error structure which MissingKeyErrorResponse expands upon.

 

class MissingKeyErrorResponse extends AbstractErrorResponse {
  public function __construct() {
    parent::__construct(3, 'Missing Key.');
  }
}

Then for the rest of your code, which can be drastically improved:

 

$container['accounts'] = function ($c) {
    return new \MyApp\Accounts($c['db']); // <-- instead of passing it to EVERY method
};

namespace MyApp;
class Accounts
{
  private $db;
  public function __construct($db) { $this->db = $db; } // <-- construction injection FTW
  public function findOneByKey($key) // <-- proper naming 
  {
    $stmt=$db->prepare('SELECT id,name FROM accounts WHERE main_key=?');
    $stmt->execute([$key]);
    return $stmt->fetch(\PDO::FETCH_OBJ);
  }
}

$app->add(function(Request $request, Response $response, $next) use ($container) {
  $key = $request->getHeaderLine('X-MyApp-Key');
  if (empty($key)) {
    return new MissingKeyErrorResponse(); // <-- proper response generated
  }
  
  $account = $this->accounts->findOneByKey($key);
  
  if (!$account) {
    return new InvalidKeyErrorResponse();
  }
  
  $container['account'] = function() use ($account) { return $account; }

  return $next($request, $response);
});

If you have a real crush on static methods try this:

 

class ErrorResponseRegistry {
  public static function missingKey() {
    return new MissingKeyErrorResponse();
  }
  public static function invalidKey() {
    return new InvalidKeyErrorResponse();
  }
}

$app->add(function(Request $request, Response $response, $next) use ($container) {
  $key = $request->getHeaderLine('X-MyApp-Key');
  if (empty($key)) {
    return ErrorResponseRegistry::missingKey(); // <-- proper response generated
  }
  
  $account = $this->accounts->findOneByKey($key);
  
  if (!$account) {
    return ErrorResponseRegistry::invalidKey();
  }
  
  $container['account'] = function() use ($account) { return $account; }

  return $next($request, $response);
});

[NotionCommotion]

$container['accounts'] = function ($c) {
    return new \MyApp\Accounts($c['db']); // <-- instead of passing it to EVERY method
};

If you have a real crush on static methods try this:

 

class ErrorResponseRegistry {
  public static function missingKey() {
    return new MissingKeyErrorResponse();
  }
  public static function invalidKey() {
    return new InvalidKeyErrorResponse();
  }
}

 

 

I didn't realize that the slim object was passed to the container, and agree this approach is much better.  Thanks!

 

While I don't have a crush on static methods (I must admit, I once did and since learned my lesson), I still don't understand why you would want to have a bunch of error response classes with just a constructor.

 

[ignace]

Why would you want to have custom exception classes while you can perfectly use Exception everywhere?

 

try {
  somethingHere();
} catch (Exception $e) {
  switch ($e->getCode()) {
    case 400:
      doHandle400();
      break;
    case 401:
      doHandle401();
      break;
    case 405:
      doHandle405();
      break;
  }
}

// versus

try {
  somethingHere();
} catch (GenericErrorException $e) {
  doHandle400();
} catch (UnsupportedMethodErrorException $e) {
  doHandle405();
} // ..

It all boils down to good design.

 

Sure you can do this:

return $response->withJson(\MyApp\ErrorResponse::missingKey(), 401);

But it does not reflect what you want to do, from a design point-of-view. It may also lead to inconsistencies using 401 here and 400 there.

 

If at some point you want to add custom headers to your errors, you will have to hunt down all uses of ErrorResponse and edit the $response

accordingly. Using a class and inheritance avoids duplication and a single point to be edited in case your requirements change.

Edited August 28, 2016 by ignace

[NotionCommotion]

Hi Ignace,

 

Earlier, you said:

if (empty($key)) {
   return ErrorResponseRegistry::missingKey(); // <-- proper response generated
}

On your latest post, are you indicating that I should do something like the following, and have the various error classes (i.e. MissingKeyErrorResponse and InvalidKeyErrorResponse) throw an exception?  Maybe I misunderstood, but I thought that typically one shouldn't throw exceptions (reference http://forums.phpfreaks.com/topic/301769-determining-which-query-caused-exception-in-try-block/)

try {
    $app->add(function(Request $request, Response $response, $next) use ($container) {
        $key = $request->getHeaderLine('X-MyApp-Key');
        if (empty($key)) {
            return new MissingKeyErrorResponse(); // <-- proper response generated
        }

        $account = $this->accounts->findOneByKey($key);

        if (!$account) {
            return new InvalidKeyErrorResponse();
        }

        $container['account'] = function() use ($account) { return $account; }

        return $next($request, $response);
    });
}
catch (Exception $e) {
    switch ($e->getCode()) {
        case 400:
            doHandle400();
            break;
        case 401:
            doHandle401();
            break;
        case 405:
            doHandle405();
            break;
    }
}

[kicken]

It's ok to throw exceptions under unexpected conditions. For example if it's expected that someone trying to access a URL is authenticated and it turns out they are not, then you can throw some sort of access denied / authorization required exception.

 

It's also ok to catch an exception if you have a way to do something about it. For example, you could catch the above authorization required exception and generate an appropriate error response with a 401 status code. Symfony does something like this to handle various HTTP error responses.

[ignace]

No, your response classes should not throw exceptions. My previous post was the answer to why you would want to use multiple response classes

for your error responses, nothing more.

 

The doHandle401() etc.. was an example to show the difference between bad designed code and good code using exceptions as an example.