-3

I'm using this method to connect to my MySQL db to SELECT, INSERT, UPDATE and DELETE data. This is the cnn() function:

function cnn() {
    static $pdo;
    if(!isset($pdo)) {
        $settings = [
            PDO::ATTR_TIMEOUT => 30,
            PDO::ATTR_PERSISTENT => false,
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC
        ];
        try {
            # settings
            $config['db']['host'] = 'example.com';
            $config['db']['name'] = 'db';
            $config['db']['user'] = 'username';
            $config['db']['pass'] = '****************';
            $pdo = new PDO('mysql:host='.$config['db']['host'].';dbname='.$config['db']['name'], $config['db']['user'], $config['db']['pass'], $settings);
            return $pdo;
        } catch(PDOException $e) {
            http_response_code(503);
            echo $e->getCode().': '.$e->getMessage();
        }
    } else {
        return $pdo;
    }
}

And then i can just do this to re-use the same pdo object each time i need it on the same request.

1st query

$sql = 'INSERT INTO user (name, lastname) VALUES (:name, :lastname)';
$stmt = cnn()->prepare($sql);
$stmt->bindValue(':name', "John", PDO::PARAM_STR);
$stmt->bindValue(':name', "Wayne", PDO::PARAM_STR);
$stmt->execute();

2nd query

$sql = 'SELECT * FROM user WHERE id_user = :id_user';
$stmt = cnn()->prepare($sql);
$stmt->bindValue(':id_user', 4641, PDO::PARAM_INT);
$stmt->execute();
$user = $stmt->fetch();

I was wondering if there could be any performance issues by using this approach. Thanks.

Andres SK
  • 10,779
  • 25
  • 90
  • 152
  • That looks fine, since the connection will run only once. But I'm not sure why you are using 2d array for the "config" since it's a local variable – HTMHell Aug 19 '19 at 17:07
  • 1
    *"That looks fine"* @HTMHell no it's not fine notice `static $pdo`....Code after should be `if(!isset(self::$pdo)) {` , `self::$pdo = new PDO('mysql:host='.)` , `return self::$pdo;` Or with the Classname before `::`.. – Raymond Nijland Aug 19 '19 at 17:14
  • 3
    this question rather belongs to https://codereview.stackexchange.com – Your Common Sense Aug 19 '19 at 17:14
  • @RaymondNijland That's clearly a function outside a class, as you can see on his examples. You can take a look at the answers of this [question](https://stackoverflow.com/questions/7508284/static-variables-in-php) to understand the different uses of `static`. – HTMHell Aug 20 '19 at 08:39
  • @HTMHell thanks for the explainment which i don't really need as i know how static or OOP programming in PHP works trust me on that, mine comment about static keyword was meant as general comment... *"That's clearly a function outside a class, as you can see on his examples"* True i agree i should have said in the comment when using classes you also can use Classname instead of `self` before `::` – Raymond Nijland Aug 20 '19 at 14:22

1 Answers1

-1

1)

As referenced by HTMHell, your $config array is local here and I believe this should be deleted after use so password variables are not leakable on a later get_defined_vars() call, or similar cross reference.

2)

It seems far more logical to me to put your function in a class. Run the cnn() function from the __construct. There are a lot of positive aspects to wrapping this function in a full class.

3)

Do NOT set any variable to being static unless you have a specific requirement in the context of putting your cnn() function into a Class.

4)

Correctly use your try{ ... } catch { ... } blocks, do not wrap other code in the try block except code that throws exceptions.

    // settings
    $config['db']['host'] = 'example.com';
    $config['db']['name'] = 'db';
    $config['db']['user'] = 'username';
    $config['db']['pass'] = '****************';
    try {
        $pdo = new PDO('mysql:host='.$config['db']['host'].';dbname='.$config['db']['name'], $config['db']['user'], $config['db']['pass'], $settings);
    } catch(PDOException $e) {
        /***
         * Do you really need this, here?  
         * //http_response_code(503);
         **/
        echo $e->getCode().': '.$e->getMessage();
    }
    return $pdo;

5)

DO NOT EVER OUTPUT ERRORS DIRECTLY TO THE SCREEN

Errors should be logged so only server folks and developers rather than the general public can see them. This also means you have a log of errors rather than a single, temporary instance of any that occur.

echo $e->getCode().': '.$e->getMessage(); Should be:

error_log($e->getCode().': '.$e->getMessage());

Or similar. Stay consistent; always return a PDO object and not echo outputs.

6)

Remove your final else wrapper it is merely clutter.

if(!isset($pdo)) {
     ...
} 
return $pdo; 

Also remove the return $pdo; inside your try { ... } block. Don't Repeat Yourself.

7)

Properly document your code:

/***
 * For generating or asserting a PDO Database Object.
 * @return Returns the PDO object 
 ***/
 function cnn() {

To answer the question you actually asked:

I was wondering if there could be any performance issues by using this approach.

This question is far too broad and can be solved by yourself using timer testings or other methods (detailed in the link) to establish on your system if it is particularly efficient or inefficient, or what SQL you choose to give it...

Martin
  • 22,212
  • 11
  • 70
  • 132
  • self::$pdo is outright wrong in a function. During development it's fairly convenient to have errors onscreen. think of it. global static wrappers are frowned upon and for a reason – Your Common Sense Aug 19 '19 at 17:45
  • @YourCommonSense I think that during development ever putting errors on screen is hughly unwise, it sets a bad precendent and means the errors reported are always temporary, and risking being transferred to a live site. – Martin Aug 19 '19 at 17:53
  • 1
    I think letting each petty function to decide where to direct its error messages is most highly unwise. there is nothing wrong with debug mode as long as it turned on and off by a single configuration option. – Your Common Sense Aug 19 '19 at 18:02
  • @YourCommonSense yes, again I agree, -in my own classes I have custom exception handlers to deal with these things, but this level of example is far beyond the depth I'm willing to write here, for this instance! Even so; I would still say that 25 `error_log`s in a script are far better than 25 `echo`s – Martin Aug 19 '19 at 18:04
  • 1
    telling the op to leave their exceptions alone is nowhere "far beyond" any depth you are willing to write. Zero error logs and zero echos what is actually better – Your Common Sense Aug 19 '19 at 18:06
  • @YourCommonSense .. err? I have suggested in my answer that It's thrown in a log file rather than to screen... you can't get more basic than that. I don't see your issue? – Martin Aug 19 '19 at 18:09
  • I, actually, can :) [PHP error reporting basics](https://phpdelusions.net/articles/error_reporting) – Your Common Sense Aug 19 '19 at 18:27
  • @YourCommonSense er, the error handler exampled on that page, actually use `error_log` in the illustrated code. – Martin Aug 19 '19 at 18:30
  • No offense Martin, but I think I can only agree with the point #6. Your answer does not really answer the question. It looks more like a code review. – Dharman Aug 19 '19 at 18:31
  • 1
    In regards to error reporting, either re-throw the exception or just get rid of try-catch altogether. It serves no real purpose here. – Dharman Aug 19 '19 at 18:32
  • If the try/catch is removed then any exception within becomes a Fatal error for having an uncaught exception. Please illustrate your own answer and I can learn from it. `:-)` – Martin Aug 19 '19 at 18:38
  • @Dharman oh yeah it is totally a code review! – Martin Aug 19 '19 at 18:38
  • @YourCommonSense you seem to have gone quiet. Please illustrate your point as I don't see your argument – Martin Aug 19 '19 at 18:39
  • That is the point. Inability to open PDO connection should be a fatal error. Unless you have a very good reason to catch the exception it should trigger fatal error and STOP the code's execution. – Dharman Aug 19 '19 at 18:40
  • @Dharman yes but from an idealist point of view the end user shouldn't see a white screen of death but a template screen with a bland error message. This can be easily done by simply catching the error, because the white screen of death is not caused by the error itself (failed to connect) it's caused by the failure to handle catching that error and recording it. ***In this specific instance*** Exception being caught or not will result in probably-similar behaviour, but as a general guide for general PHP usage **EXCEPTIONS SHOULD BE CAUGHT**. – Martin Aug 19 '19 at 18:47
  • @Dharman one example could be if the PDO connection fails to try a MySQLi connection instead or to cycle through a pre-set list of different SQL ports; these details and choices are down to the OP and their specific situation, but crucially, these secondary steps and mitigators **can not be made** if the PHP code throws a FATAL error due to the Exception not being caught. **THAT** is bad programming. – Martin Aug 19 '19 at 18:50
  • [Reference Q&A](https://stackoverflow.com/questions/6530206/why-should-i-use-exception-handling-in-php). – Martin Aug 19 '19 at 18:52
  • I agree exceptions are useful and catching them sometimes makes a lot of sense, I have done it myself multiple times. The problem here is that in the catch block there is no real error handling happening. `echo` and `error_log` are not going to prevent the rest of the code from throwing fatal error because of missing PDO connection. There should be no blank page either, if there is then it means your server lacks proper error page. But I am just repeating what [@YourCommonSense has already said](https://phpdelusions.net/delusion/try-catch) – Dharman Aug 19 '19 at 19:08
  • @YourCommonSense further to your reference to PHP Delusions, the website, (yours?) [actually states that Exceptions *should* be rased on PDO instance creation](https://phpdelusions.net/pdo#errors). This completely contradicts your statement that the OP should "leave their exceptions alone" at this point in code. . . . – Martin Aug 19 '19 at 19:12
  • @Dharman I would, if I may, like to break things into chuncks: **1)** `error_log` is better than `echo`. That was my original reason for stating it. No it does not fix the exception but it shows how the OP should *record* the error. Output to screen is terrible for the reasons stated in my answer. You seem to disagree with these reasons? **2)** Yes the rest of the code *could* throw various errors and exceptions, and the OP did have a `http_response_code` in place to try to somewhat mitigate end-user issue here. – Martin Aug 19 '19 at 19:23
  • @Dharman (continued) **3)** I think ultimately these secondary effects can be handled in various ways - for example if the function is wrapped in a class or with an appropriate Exception Handling part, or some other code in place. This is beyod my care or the OPs requirements to write up here on SO. **4)** I must fundamentally emphasis that I think that NOT catching Exceptions is absolutely the wrong way to handle them. Yes, the `error_log` output is similar in this specific instance, – Martin Aug 19 '19 at 19:23
  • @Dharman (continued, again) **5)** I am confused how some PHP code on your server does not throw White Screen of Death (WSOD) on an uncaught triggered error? Please can you elaborate? Maybe there are various mitigators in place I've failed to agknowledge? – Martin Aug 19 '19 at 19:24
  • 1) Uncaught exception will be logged anyway. Errors should be displayed only by PHP and only in DEV 2) `http_response_code` will not prevent the rest of the code from executing. It doesn't do anything more than `echo` in terms of error handling. 5) You can configure [custom error pages](https://httpd.apache.org/docs/2.4/custom-error.html) or if you use AJAX check the response code and display a nice error message to the user. [Even SO shows a custom 500 page](https://meta.stackexchange.com/q/300452/515960) – Dharman Aug 19 '19 at 19:37
  • @Dharman re:1 ; whether the error is logged is by-the-by; uncaught Exceptions throw fatal errors and are bad PHP practise. This is even shown on [PHP Delusions for PDO instantiations](https://stackoverflow.com/questions/57561109/how-to-connect-to-mysql-using-pdo-using-php-function/57561575?noredirect=1#comment101586988_57561575). `error_log` > `echo`. Telling the PDO system to throw exceptions and then deliberately not catching them is bad practise, and this is precisely why PHP throws a **FATAL** level error, rather than, say, simply a warning. `:-)` – Martin Aug 19 '19 at 19:46
  • Comments are not for extended discussion; this conversation has been [moved to chat](https://chat.stackoverflow.com/rooms/198154/discussion-on-answer-by-martin-how-to-connect-to-mysql-using-pdo-using-php-funct). – Samuel Liew Aug 20 '19 at 00:45