1
\$\begingroup\$

I'm new to PDO, and I want to make sure I made this in the correct way. This way is working, but I have a feeling it's unclean.

I have this input:

$query = $handler->query('SELECT * FROM content WHERE id=1');
while($r = $query->fetch()) {
?>
<div id="content">
    <div class="container">
    <form action="<?php echo BASE_URL ?>admin/edit_process.php?edit_about" method="post" autocomplete="off" id="contact_form" name="contact_form" role="form">
      <div class="form-group">
          <label for="message">Izmeni sadržaj sa početne strane:</label>
          <textarea class="form-control" id="message" name="message" required="" rows="6"><?php echo $r['edit_about']; ?></textarea>
      </div>
      <div class="form-group">
          <button class="btn btn-success float-right" type="submit" name="submit">Sačuvaj</button>
      </div>
    </form>
  </div>
</div>
<?php
}

Then it goes to this:

if(isset($_REQUEST['edit_about'])){
    if(!$user->isLoggedIn()){ Redirect::to(''.BASE_URL.''); }
  if(!$user->hasPermission('admin')) { Redirect::to(''.BASE_URL.''); }
    if(Input::exists()){
        try{
      $message = $_POST['message'];
      $sql = "UPDATE `content` SET `edit_about` = :message WHERE `id` = 1";
      $query = $handler->prepare($sql);
      $query->execute(array(
          ':message' => $message
      ));
            Redirect::to(''.BASE_URL.'index');
        }catch(Exception $e){
            die($e->getMessage());
        }
    }
}

And this is the output:

<?php
$query = $handler->query('SELECT * FROM content WHERE id=1');
while($r = $query->fetch()) {
    echo $r['edit_about'];
}
?>
\$\endgroup\$
0
\$\begingroup\$

The biggest problem with your PDO code is error reporting. die($e->getMessage()); is the worst method possible. Citing from my article on error reporting (which I highly recommend to read):

  • for the casual user the error message is pretty cryptic and confusing
  • it it essentially useless for the site programmer/admin as they likely won't be browsing the site at the moment and will have no idea that error has been occurred
  • the information provided in the error message is extremely useful for a potential hacker, as it provides them with the feedback for their actions and may leak some sensitive information about your site

So never write a try..catch operator unless you have a certain scenario to be executed in the catch block as it will be useless and harmful.

Another suggestion is just a matter of taste. Given you have only one variable in the query, you can use a positional placeholder instead of a named one, sparing yourself writing some extra code:

if(Input::exists()){
  $sql = "UPDATE `content` SET `edit_about` = ? WHERE `id` = 1";
  $query = $handler->prepare($sql);
  $query->execute([$_POST['message']]);
  Redirect::to(BASE_URL.'index');
}

As you may notice, the PDO code looks a bit different from the other code you are using. With a simple PDO wrapper could make this code even simpler and uniform:

if(Input::exists()) {
    $sql = "UPDATE `content` SET `edit_about` = ? WHERE `id` = 1";
    DB::run($sql, [$_POST['message']]);
    Redirect::to(BASE_URL.'index');
}

The example is taken from my PDO wrapper usage examples but you can use any wrapper you like.

Also, I don't know which framework you are using, but $_POST['message'] seems a bit alien to me. I would expect something like Input::get('message') instead.

\$\endgroup\$
  • \$\begingroup\$ So i should use trigger_error($e->getMessage()); is that correct? \$\endgroup\$ – Monk Nov 21 '18 at 20:22
  • \$\begingroup\$ can i remove try...catch and die(), do i need them? Can i just make redirection to 500 page? \$\endgroup\$ – Monk Nov 21 '18 at 20:36
  • \$\begingroup\$ a redirection is a bad idea, as it makes no sense. a page that has an error returns a 300 code, and then a page it redirected to returns 500. A total mess. You should not redirect, you must return 500 right away. \$\endgroup\$ – Your Common Sense Nov 22 '18 at 6:38
  • \$\begingroup\$ I'm little bit confused. Is it good to show error or not? Because in my code i gave you i had that error, and you suggest me the code where i don't have try...catch and error report. Is it safe to have it like that, without error report? If not, should i use then try...catch and put trigger_error($e->getMessage()); inside of it? Thank you. \$\endgroup\$ – Monk Nov 22 '18 at 7:46
  • \$\begingroup\$ trigger_error($e->getMessage()); inside of it makes absolutely no sense. the outcome would be exactly the same as without try catch. Your other questions are already answered in the article I linked at the top of my answer. \$\endgroup\$ – Your Common Sense Nov 22 '18 at 7:51

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.