1

Beneath is my PDO wrapper. I want to be able to use the run method however, I want to be able to check if the execution has been successful such as:

if($sth->execute())
{
   ...
}

However, as you can see within the wrapper, the run command only returns the prepare statement, what would be the most efficient way to achieve this?

<?php

class Database {

    const hostname = 'localhost';
    const user = 'root';
    const password = '';
    const charset = 'utf8';
    const database = 'syn_v2';

    protected static $instance;
    protected $pdo;

    protected function __construct()
    {
        $opt = array(
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_OBJ,
            PDO::ATTR_EMULATE_PREPARES => false
        );

        $dsn = sprintf('mysql:host=%s;dbname=%s;charset=%s', self::hostname, self::database, self::charset);

        $this->pdo = new PDO($dsn, self::user, self::password);
    }

    public static function instance()
    {
        if(self::$instance === null)
        {
            self::$instance = new self;
        }

        return self::$instance;
    }

    public function __call($method, $args)
    {
        return call_user_func_array(array($this->pdo, $method), $args);
    }

    public function run($sql, $args = [])
    {
        if(!$args)
        {
            return $this->query($sql);
        }

        $stmt = $this->pdo->prepare($sql);
        $stmt->execute($args);

        return $stmt;
    }

}

?>
Intel
  • 173
  • 1
  • 7
  • @YourCommonSense this is not a duplicate of the referenced question. The author already knows they want the return response of `execute` and why. The issue the OP faced was in determining how to retrieve the results of `execute` after calling it from inside another object. ie. setting a property value, returning it directly, etc. – Will B. Jun 02 '19 at 06:03
  • @fyrye It is not the question but the answer that matters for the closure. There is the answer that handles this very case. In such a *rare* case when you need to check the result, you have to catch the exception – Your Common Sense Jun 02 '19 at 06:08
  • 1
    This is not a duplicate – Intel Jun 02 '19 at 11:30
  • The question here is "I want to be able to check if the execution has been successful", which is even worded almost the same as the duplicated source. I don't see how it is not a duplicate. – Your Common Sense Jun 02 '19 at 16:00
  • @YourCommonSense how specifically would yours or any answer on that page allow the OP to determine `PDOStatement::execute` **explicitly** succeeded or failed when calling `Database::run()`? You have extrapolated the intent to suit what you believe the OP should be looking for. See my updated answer with exception handling for clarification. – Will B. Jun 03 '19 at 00:08
  • @fyrye somehow *you* changed your mind in what you believe the OP should be looking for. In the first version of your answer, which perfectly satisfied the OP, there was no such distinction. – Your Common Sense Jun 03 '19 at 04:58
  • @YourCommonSense I added some clarification to the first revision and an alternative approach based on the discussion of exception handling. The distinction and my comments regarding the OP desired result has remained the same. Which is noted with `//either prepare or execute failed` from my initial answer. If the OP deemed it to be unsatisfactory for their needs, I would have flagged as duplicate just as you had, as that would have been the answer they were looking for. – Will B. Jun 03 '19 at 05:08
  • @fyrye Sorry I don't get the logic here. The OP accepted the answer with "either prepare or execute failed", leaving a comment "works perfectly". From this we can safely assume, such a distinction was not what the OP was looking for. But OK. I am tired to the bone with this discussion caused by a mere typo in the code. Have it the way you wish. – Your Common Sense Jun 03 '19 at 05:17

1 Answers1

0

Since PDOStatement::execute returns true/false and your current run method is returning a PDOStatement on success and false on failure. I suggest checking prepare and execute are not false and return the PDOStatement on success or false otherwise, as is the case with PDO::prepare and PDO::query.

Functional Example https://3v4l.org/OnDdn

/**
 * @return PDOStatement|false
 */
public function run($sql, $args = [])
{
    if (!$args) {
        return $this->pdo->query($sql);
    }
    if ($stmt = $this->pdo->prepare($sql)) {
        if ($stmt->execute($args)) {
            return $stmt;
        }
    }

    return false; //either prepare or execute failed
}
$db = Database::instance();
var_dump($db->run('SELECT ?', ['foo', 'bar'])); //false

An alternative approach would be to store the last execute value in a property, for later retrieval.

Example https://3v4l.org/UbM1N

class Database
{

    protected $lastExecute;

   //...

    /**
     * @return PDOStatement|false
     */
    public function run($sql, $args = [])
    {
        if (!$args) {
            return $this->pdo->query($sql);
        }
        if ($stmt = $this->pdo->prepare($sql)) {
            $this->lastExecute = $stmt->execute($args);
        }

        return $stmt;
    }

    /**
     * @return null|bool
     */
    public function getLastExecute()
    {
       return $this->lastExecute;
    }
}
$db = Database::instance();
$db->run('SELECT ?', ['foo', 'bar']);
var_dump($db->getLastExecute()); //false

To address the best-practices comments below in regard to the issue of determining when PDO::execute method specifically fails from within the Database::run method by using Exception handling.

Please keep in mind Best-Practices are not about right or wrong, "they are simply recommended methods of writing code." Referring to an approach of programming that is commonly preferred in professional application development. Always use what works best for you, the environment you are developing for and your application requirements.

Generally speaking StackOverlow is not an appropriate place to discuss or evaluate an author's application of best-practices. Those types of discussions or critiques should be reserved for CodeReview. StackOverflow is intended to answer the author's specific question, or provide a viable alternative method to accomplish what the user is asking for. Not infer the user has asked the wrong question.

To use exceptions you need to enable PDO::ERRMODE_EXCEPTION (see below Database class).

The issue with using try/catch with a PDO wrapper method, is that PDO will only throw a single Exception object PDOException, which does not provide you with the ability to determine which PDO method call specifically failed. Leaving you to read the PDOException::getMessage() or PDOException::getTrace(), to determine the cause.

A simple approach would be to check the PDOException::trace for the function name that caused the exception.

Functional Example (PHP 5.6+): https://3v4l.org/fDnBI

try {
   $db = Database::instance();
   var_dump($db->run('SELECT ?', ['foo', 'bar'])->fetch());
} catch(\PDOException $e) {
   if ('execute' === $e->getTrace()[0]['function']) {
       echo 'PDO::execute() failed';
       //Handle the execute exception
   }
   throw $e;
}

Please see the answer on PDO mysql: How to know if insert was successful by Your Common Sense for a more generalized approach to PDOException handling.

The above approach prevents you from handling only specific exception types within the Database::run method, requiring you to use throw $e; after your conditional, when the exception is unexpected.
To account for this issue, another approach would be to create custom Exception classes. You can do this by extending the base PDOException class to be compliant with other exception handling methods or to catch any of them.

In order to catch any of the run specific exceptions, an empty interface can be used that is then implemented on the extended PDOException classes.

interface DatabaseRunException{}

Then create a new exception class for each of the specific PDO methods you would like to handle, that implements the DatabaseRunException interface.

class PDOPrepareException extends PDOException implements DatabaseRunException{}

class PDOExecuteException extends PDOException implements DatabaseRunException{}

class PDOQueryException extends PDOException implements DatabaseRunException{}

To use the custom exceptions to determine which PDO method failed, you need to handle the PDOException(s) within the Database::run() method and throw one of the custom exceptions.
I have removed certain portions for brevity, commented out things that would alter your current configuration, made some best-practices and optimization changes for PHP 5.6+.

class Database 
{

    private const OPTIONS = [
        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        // PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_OBJ,
        // PDO::ATTR_EMULATE_PREPARES => false
    ];

    //...

    protected function __construct()
    {
        $this->pdo = new PDO($dsn, self::user, self::password, self::OPTIONS);
    }

    public static function instance()
    {
        if (null === self::$instance) {
            self::$instance = new self;
        }

        return self::$instance;
    }

    public function __call($method, $args)
    {
        //always ensure the desired method is callable!
        if (is_callable([$this->pdo, $method])) {
            //php 5.6+ variadic optimization (aka splat operator)
            return $this->pdo->$method(...$args);

            //php <= 5.5
            //return call_user_func_array(array($this->pdo, $method), $args);
        }
        throw new \BadMethodCallException(sprintf('Unknown method PDO::%s called!', $method));
    }

    public function run($sql, $args = [])
    {
        if (!$args) {
            try {
                return $this->query($sql);
            } catch(\PDOException $e) {
                 throw new \PDOQueryException($e->getMessage(), (int) $e->getCode(), $e);
            }
        }
        try {
            $stmt = $this->prepare($sql);
        } catch(\PDOException $e) {
            throw new \PDOPrepareException($e->getMessage(), (int) $e->getCode(), $e);
        }
        try {
            $stmt->execute($args);

            return $stmt;
        } catch(\PDOException $e) {
            throw new \PDOExecuteException($e->getMessage(), (int) $e->getCode(), $e);
        }

        throw new \LogicException('Unknown error occurred');
    }

}

Functional Example (PHP 5.6+): https://3v4l.org/8QoRF

You will now be able to handle each of the exceptions for any of the specific types.

try {
   $db = Database::instance();
   $db->run('SELECT ?', ['foo', 'bar']);
} catch(\PDOExecuteException $e) {
    echo 'PDO::execute() failed';
    //Handle the execute exception
    throw $e;
}

In PHP 7.1+ you can catch multiple exceptions.

try {
   $db = Database::instance();
   $db->run('SELECT ?', ['foo', 'bar']);
} catch(\PDOQueryException $e) {
    //Handle the query exception
    throw $e;
} catch(\PDOPrepareException $e) {
    //Handle the prepare exception
    throw $e;
} catch(\PDOExecuteException $e) {
    echo 'PDO::execute() failed';
    //Handle the execute exception
    throw $e;
}

In PHP <= 7.0 you can use the DatabaseRunException interface to catch and check the specific exceptions caused by Database::run() with instanceof to determine which exception was actually thrown.

try {
   $db = Database::instance();
   $db->run('SELECT ?', ['foo', 'bar']);
} catch(\DatabaseRunException $e) {
    if ($e instanceof \PDOQueryException) {
       //Handle the query exception
    } elseif ($e instanceof \PDOPrepareException) {
       //Handle the prepare exception
    } elseif ($e instanceof \PDOExecuteException) {
       echo 'PDO::execute() failed';
       //Handle the execute exception
    }
    throw $e;
}

As you can see, this increases the code complexity and will be up to you on determining what best suits your application needs.

It is important to note, that variables declared in the try section will not be declared if an exception occurs prior to the declaration.

try {
  throw new \Exception('FooBar');
  $foo = 'foo';
} catch(\Exception $e) {
   var_dump(isset($foo)); //false
}
var_dump(isset($foo)); //false
Will B.
  • 17,883
  • 4
  • 67
  • 69
  • 1
    Oh wow, that works perfectly. I never thought of doing that for some odd reason. Thank you. – Intel Jun 02 '19 at 00:36
  • @Intel this is exactly the same as you have at the moment. It means your initial code "works perfectly" already. – Your Common Sense Jun 02 '19 at 05:39
  • Besides, it's a bad architecture in general, to make your function return results of different types. Your function shouldn't return false on error - this is what *exceptions* are for – Your Common Sense Jun 02 '19 at 05:43
  • @YourCommonSense While I agree with your assessment on exceptions, especially with database queries and almost wrote the same but saw that `PDO` is not set with `PDO::ERRMODE_EXCEPTION`, You are incorrect about the author's original code, which returns the `$stmt` from `prepare` and NOT `execute`, Meaning `prepare` could succeed but `execute` could fail without notice. Additionally both `prepare` and `query` return `PDOStatement` or `false` on failure, so the `run` method functions similarly to how the individual PHP functions do by default. – Will B. Jun 02 '19 at 05:56
  • This is basically my [PDO wrapper](https://phpdelusions.net/pdo/pdo_wrapper) so I know on sight that PDO *is* set with PDO::ERRMODE_EXCEPTION. Check the code in the OP again. – Your Common Sense Jun 02 '19 at 06:04
  • @YourCommonSense The `$opt` variable is not used, despite it containing `PDO::ERRMODE_EXCEPTION`, please check the OP code again – Will B. Jun 02 '19 at 06:06
  • 1
    WOW. indeed. You can tell though it's more likely a typo. So, @Intel looks like you want to fix it instead of going at lengths down the road of bad practices. – Your Common Sense Jun 02 '19 at 06:10
  • Honestly, I don't understand what is the proposed use case for your solution. Given the default errmode is SILENT, this code of yours will fail silently as well, and in the condition proposed in the OP there are no means to get the error information. A big rad flag if you ask me. What I am missing here? – Your Common Sense Jun 02 '19 at 06:22
  • @YourCommonSense It will ERROR silently but still return `true/false`. This allows the OP to handle it as they desire as specifically requested in their question with `if ($sth->execute())` As such `if (Database::run($params)){ /* do something */ }` As opposed to `try{ Database::run($params); /*do something */ } catch(PDOException){ /*handle error*/ }` Same result different approaches. Originally `execute` could fail silently and the OP would not tknow. – Will B. Jun 02 '19 at 06:29
  • It is not the same. The second part is actually `catch(PDOException $e)` where $e contains **the error information** to handle. It makes all the difference. – Your Common Sense Jun 02 '19 at 06:31
  • @YourCommonSense I am aware, it is why I left out the variable and `$e->getMessage();` The OP never asked for *What is the error?*, they **ONLY** wanted to know how to get the `false` result of `execute` from the `run` method to determine if `execute` was unsuccessful. Your responses and duplicate flag with "You're doing it wrong", is just your personal opinion, despite my agreeing with you on using exceptions. It is not what the OP asked for and is leading down a rabbit hole of refactoring an application in order to handle exceptions, that is unrelated.. – Will B. Jun 02 '19 at 06:41
  • See, the OP made a silly mistake when porting the existing code for their needs. It led them to asking the wrong question. Technically you have the right to answer it directly, but I were you I wouldn't defend the apparently bad practice so fiercely. Yes, it is my personal *opinion* that answers on Stack Overflow, being publicly accessible, must promote best practices and show the right way, as opposed to cater to anything the OP asks out of a some mistake or wrong premises. Have a nice day. – Your Common Sense Jun 02 '19 at 06:51
  • @YourCommonSense I have had my comments stating the same thing removed by moderators, and SO staff informing me that my comments were argumentative and noisy and that I should either answer the question or move on. – Will B. Jun 02 '19 at 07:04
  • I'll be honest I don't mind learning what I'm doing wrong, I'm not sure how to properly handle errors. I wouldn't mind if you let me know – Intel Jun 02 '19 at 10:51
  • @Intel updated with an example with exception handling for your specific use-case of determine when `execute` explicitly causes an exception to occur. – Will B. Jun 02 '19 at 14:45