2
\$\begingroup\$

I have created this middleware code for slim. I just want to know that is this right way to store site settings globally. so, I can use where ever I want. What do you think about my class structure apart from invoke (which is slim's own feature) what do you think about foreach and setting the data. I'm trying to learn and find good practice. thanks.

this is the middleware

    namespace entity;

    use PDO;
    use Psr\Http\Message\ServerRequestInterface;
    use Psr\Http\Message\ResponseInterface;

    class SiteSettings {

      public function __construct( $app ) {
        $this->pdo =  $app->getContainer()['pdo'];
      }
      public function __invoke( $request, $response, $next ) {
        if( $this->getSettings() !== false ) {
          foreach ( $this->getSettings()  as $row )
          $response = $next($request->withAttribute($row['setting'], $row['value']), $response );
        }
        return $response;
      }
      protected function getSettings() {
        $sql = "SELECT setting, value, description FROM site_settings";
        try {
          $query = $this->pdo->prepare( $sql );
          if ( $query->execute() ) {
            return $query->fetchAll(PDO::FETCH_ASSOC);
          }
          return false;
        } catch ( PDOException $pe ) {
          return false;
        }
      }
    }

added to all routes

$app->add( new entity\SiteSettings( $app ) );

and able to use it like this

    $app->get('/en/admin', function ($request, $response, $args) {
        // Sample log message
        $this->logger->info("Tes '/' route");
        echo $request->getAttribute('site_name');

     });

and this is the table

     CREATE TABLE `site_settings` (
        `setting` VARCHAR(250) NOT NULL DEFAULT '' COLLATE 'utf8_unicode_ci',
        `value` VARCHAR(250) NULL DEFAULT NULL COLLATE 'utf8_unicode_ci',
        `description` TINYTEXT NULL COLLATE 'utf8_unicode_ci',
        PRIMARY KEY (`setting`)
     )
     COLLATE='utf8_unicode_ci'
     ENGINE=InnoDB
     ;
\$\endgroup\$
3
\$\begingroup\$

A couple of general comments:

  • Please follow the PSR coding standards whenever possible
  • Add type-hints for all arguments, especially constructor arguments. You're clearly expecting $app to be an object. Add the type-hint
  • Predeclare properties, because: what PHP calls object overloading (dynamic properties) are all public, and are slower. Relying on them makes it easy to remove/reassign core properties and makes your code buggy the moment you have typos, which you will. Inevitably.
  • Avoid one-line loops without brackets (foreach ( $this->getSettings() as $row )). I know they're technically not needed there, but add them. It's a good habit that avoids awful bugs (think: goto fail;)
  • You're using PDO, which is good. You're using prepared statements, which is excellent, but the way you're using them doesn't really make sense
  • Your use of PDO is made worse by the fact that you're repeatedly calling getSettings. Each call will query the DB for the same data. Don't do that

So Let's start by looking at the constructor, how I'd rewrite it:

//add use Slim\App;
/**
 * @var PDO
 */
protected $pdo = null;

/**
 * You're not using this, but just in case:
 * @var App
 */
 protected $app = null;
public function __construct(App $app)
{//<-- PSR coding style
    $this->app = $app;
    $this->pdo = $app->getContainer()['pdo'];
    if (!$this->pdo instanceof PDO) {
        throw new \RuntimeException(
            sprintf(
                '%s requires a PDO instance, app did not contain "pdo" key',
                __CLASS__
            )
        );
    }
}

Avoid querying too many times

Your __invoke method calls getSettings several times. Each time, you'll be querying for the exact same data as the last time. I'd rewrite that ASAP:

public function __invoke( $request, $response, $next )
{
    $settings = $this->getSettings();//assign!
    if( $settings ) {
        foreach ($settings as $row) {
            //set all attributes first
            $request->withAttribute(
                $row['setting'],
                $row['value']
            );
        }
        $response = $next($request, $response);//only call next once
    }

    return $response;
  }

Because your class is doubling as a callable instance (hence the __invoke), you might want to try adding type-hints to the __invoke method, too.

About your use of prepared statements:

Prepared statements are what you use when you're going to query the DB, and you want to use values that might be used for injection attacks, or values that, because they contain quotes might simply break the query.
You might also choose to use prepared statements for queries that you're running mutliple times, because prepared statements can be reused.

You're doing neither: you're creating a prepared statement resource, execute it and forget about it. The query you want to execute in fact is hard coded, so there's no risk of injection whatsoever. What I'd do is this:

$stmt = $this->pdo->query('SELECT setting, value, description FROM site_settings');
return $stmt->fetchAll(PDO::FETCH_ASSOC);

Nothing more. If an exception is thrown here, that should cause the app to grind to a halt: The query to fetch the site settings failed. There's nothing your app can do that would be even remotely sensible. It's a fatal error, so let the exception propagate until a global handler catches it and renders an error page. Don't hush the exception up.

Right now, you're returning false both if a PDOException was thrown, or if there aren't any settings. That's madness. An EXCEPTION signals something EXCEPTIONAL has happened in a bad way. Don't hush it up, sound the alarms! Your app is on fire!

Final nit-pick

Going by the Slim documentation, and because they say their container component is based on pimple, I'd probably use this to get the PDO instance from the container:

$this->pdo = $app->getContainer()->get('pdo');

You could write $app->getContainer()->pdo, too, but I have a profound aversion for the magic __get and __set methods. So much so, then I often implement them to throw exceptions with the sole purpose to disable overloading (dynamic properties):

public function __get($name)
{
    throw new \RuntimeException(
        sprintf(
            'tried to access property non-%s "%s" as public on %s',
            property_exists($this, $name) ? 'public' : 'existent',
            $name,
            get_class($this)
        )
    );
}

public function __set($name, $value)
{
    throw new \RuntimeException(
        sprintf(
            'tried to set property non-%s "%s" to "%s" on %s',
            property_exists($this, $name) ? 'public' : 'existent',
            $name,
            serialize($value),
            get_class($this)
        )
    );
}
\$\endgroup\$
  • \$\begingroup\$ Your answer is priceless. So much good information for me and I cannot thank you enough for this. thank you so much. Seems like I need to learn much more. thank you so much again! \$\endgroup\$ – Erdem Ece Mar 15 '16 at 18:41
  • \$\begingroup\$ there is a problem. because response = next() is inside foreach loop website renders twice as there is a two record in settings table. how can I fix that? thanks. \$\endgroup\$ – Erdem Ece Mar 15 '16 at 19:38
  • \$\begingroup\$ @ErdemEce: change the foreach to just call $request->withAttribute($row['setting'], $row['value']); and move the next call outside of the loop. Then you'll only call $next once, but you'll have set all attributes on the request object beforehand. I'll update the __invoke method in my answer \$\endgroup\$ – Elias Van Ootegem Mar 16 '16 at 9:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.