PHP MySQL novice to ninja 6th edition - databasetable.php


#43

To me it also seems weird to use a failed insert to decide to do an update. I don’t think the one extra redundant database query for an update matters much except for high traffic systems. However, I wouldn’t choose this method because to me a database error logically is a sign of a problem to be fixed and not a normal application flow - unless I check specifically for the error code returned from the database and act accordingly. Usually, I have my db class set up to log all database errors to a file and in this situation the expected failed inserts (in case of edits) would pollute my logs unless I code an additional flag to prevent that from happening. Therefore, while I wouldn’t call this method wrong it is not what I would call logical or obvious.

I prefer to simply use a hidden field in a form to hold the record ID and have it empty for new records. Then deciding whether to insert or to update is a matter of checking for the existence of the ID. I suppose that’s what @benanamen had in mind, too. And this works very well also for natural primary keys which are user-editable as part of the form.

Also, while I understand there is ultimately a bit less code when it’s done @TomB’s way I find it’s not practical to make the code that universal for both inserts and updates because as the complexity grows there are often requirements for a slightly different logic for each of these actions. For instance, often there is a need to supply a current timestamp value to inserted rows while not to existing ones. When checking for an ID from the form it’s quite simple to do:

if (empty($id)) {
  $record['created_at'] = date('Y-m-d H:i:s');
  $this->insert($record);
} else {
  $this->update($record, $id);
}

How would you do it when you try to always insert first? One idea:

$recordToInsert = $record;  // clone entire record
$recordToInsert['created_at'] = date('Y-m-d H:i:s');

if (!$this->insert($recordToInsert)) {
    $this->update($record);
}

To me it’s a little bit more convoluted (in terms of logic, not amount of code) and as requirements grow the complexities will also grow and I find it cleaner to be able to make the decision upfront whether an insert or update is needed. Sometimes I even keep them as separate methods in order to minimize the amount of if/else statements. It’s easier to follow the code of a method if it has a single purpose. But this is a matter of weighing pros and cons and obviously everyone may make different decisions.


#44

This breaks for keys where the table is not auto-increment and requires adding an additional layer of logic to account for those situations.
And for composite keys requires more logic still.

if (empty($id)) {
  $record['created_at'] = date('Y-m-d H:i:s');
}
$table->save($record);

It’s simpler than your initial solution.


#45

No, why would it break for not auto-increment? It doesn’t matter if the PK is AI or not, the ID of the record is passed to the update function - it doesn’t matter what kind of key it is.

A bit - just one more field.

Maybe simpler but what if the PK is a text field and the user changes the PK? Your save/update method doesn’t event know how to find the record because $record doesn’t contain the PK before the change, unless you add a magic field into $record with the old PK value.


#46

In this special case it needs the same amount of logic your approach needs for every case.

Because you need the logic:

if (empty($record['id']) { 
$table->insert($record);
}
else {
$table->update($record);
}

If the table is not auto-increment then it breaks, it’ll always try to update because id has been supplied by the user on the form.

edit: regardless of the approach you use there will be cases where you need some special logic. However, I’d prefer a default approach that handles most cases with minimum code. In most cases I find that tables are auto-increment and the record needs to be updated if the PK is set.

It’s possible to move this code:

if (empty($record['id']) { 
$table->insert($record);
}
else {
$table->update($record);
}

into a function so it’s reusable, but the primary key is not always ID and it’s not always a single column. In the interest of having some generic code that handles most use-cases, the try/catch approach works well.


#47

Yes, you are right, in this case the if should be like this:

if (empty($old_id) {

where $old_id is passed from the form.

I see your approach looks good but still checking for a failed insert looks weird. If I were to do this at least I’d check for a specific exception, some db drivers like DBAL have exceptions for various database errors:

try {
  $this->insert($record);
} catch (UniqueConstraintViolationException $e) {
  $this->update($record);
}

The downside is this wouldn’t work with pure PDO.


#48

I’m still looking for someone to show a practical downside rather than “eww”. Out of interest how do you feel about REPLACE INTO or ON DUPLICATE KEY UPDATE or even a preliminary SELECT that checked to see if the PK was in use before doing an insert or update in the save function?

The save function’s API would look identical. If there was an issue with the data (e.g. trying to write to a column that doesn’t exist) it would throw a PDOException. Someone using the object and looking at the API alone would not know or care which of those three (or try/catch) was used.

I guess my question is if ON DUPLICATE KEY UPDATE is fine, but you can’t tell if it’s being used without looking at the code what difference does it make? What we’re really interested in is preventing negative side effects by choices the code makes. If, to an object using try/catch and another using REPLACE INTO are indistinguishable, why is one a problem and the other not?


#49

There is none I know of - I said earlier that I don’t consider this incorrect, only to me it looks more logical to user other methods than database errors to determine whether to do an insert or update. In other words - subjective preference, so you won’t get anything more from me than “eww” :smiley:

I think they are perfectly fine. But every time we use methods like this it’s important to think carefully about the execution flow to make sure we don’t run into dangerous race conditions, etc. For example, in your save method’s case it’s important that when an insert fails and you catch the exception and continue to update - that the update also fails but fails with an error, not silently. Otherwise, in some rare cases (like database connection errors, etc.) a user might try inserting a new record - the insert will fail, the update will fail - but if the update does not trigger a visible error the user might think the record was inserted. This might be a problem if your update method issued a well-formed query for an non-existent ID for an insert action like this:

UPDATE mytable SET name='Mark' WHERE id=NULL;

This is the reason why I prefer to use a single UPDATE query for an update - even if it takes a little bit more coding, it’s not that much effort but at least I’m free from thinking about potential problematic edge cases like the one above.


#50

Ok, here it is…drum roll…:open_mouth:

This uses @TomB’s class exactly as is. Only change I made to the class is setting the insert and update methods to public.

This knows if you are doing an update and does an update only. If you are doing an edit, it does an edit only.

Calling Page

<?php
require "DatabaseTable.php";

$db = new DatabaseTable($pdo, 'contacts', 'id');

if ($_SERVER['REQUEST_METHOD'] == 'POST')
{
    isset($_POST['data']['id']) ? $db->update($_POST['data']) : $db->insert($_POST['data']);
}
if (isset($_GET['id']))
{
    $form_data = (array)$db->findById($_GET['id']);
}
include './templates/form_contacts.php';

The Form

<form method="post">
    First <input name="data[first_name]"
                 value="<?= !empty($form_data['first_name']) ? $form_data['first_name'] : ''; ?>"><br>
    Last <input name="data[last_name]"
                value="<?= !empty($form_data['last_name']) ? $form_data['last_name'] : ''; ?>"><br>
    <?= !empty($_GET['id']) ? "<input type='hidden' name='data[id]' value='{$_GET['id']}'>" : ''; ?>
    <button type="submit">Submit</button>
</form>


#51

I’d recommend a few minor changes, although I agree with the general setup :+1:

<?php
require "DatabaseTable.php";

$db = new DatabaseTable($pdo, 'contacts', 'id');

if ($_SERVER['REQUEST_METHOD'] == 'POST')
{
    isset($_POST['data']['id']) ? $db->update($_POST['data']) : $db->insert($_POST['data']);
    header('Location: /path/to/display/contact');
    exit;
}

$form_data = ['first_name' => '', 'last_name' => ''];
$id = null;

if (isset($_GET['id']))
{
    $id = $_GET['id'];
    $form_data = (array)$db->findById($_GET['id']);
}

include './templates/form_contacts.php';
<form method="post">
    First <input name="data[first_name]"
                 value="<?= $form_data['first_name'] ?>"><br>
    Last <input name="data[last_name]"
                value="<?= $form_data['last_name'] ?>"><br>
    <? if (null !== $id): ?>
      <input type="hidden" name="data[id]" value="<?= $id ?>">
    <? endif; ?>
    <button type="submit">Submit</button>
</form>
  • Redirecting after submission a recommended way to prevent duplicate submissions
  • Providing default empty values for first_name and last_name keeps the template a lot simpler and easier to maintain
  • The template shouldn’t care where id comes from, it certainly shouldn’t know or care about HTTP works (i.e. don’t use $_GET in templates)

#52

LOL! Your exactly right and that is exactly what I ripped out to provide the most minimal example I could show to get my point across. Really, go look at my repo, it’s been up exactly as you say for some time now. Excellent observation Mr. @ScallioXTX! :+1:


#53

But that still requires the logic for choosing to update or insert on every single form you create, the whole thing I was avoiding with the save method.

It also mixes a lot of different concerns, most are irrelevant for this thread.


#54

What are you even talking about? That is a very clean, sensible, logical solution to a single add/edit page. It either does the single task of an insert using a single method, or it does the single task of an edit using a single method, in a single simple line with YOUR exact code.


#55

It deals with:

HTTP requests
Loading templates
Fetching data from the database
Saving data to the datavase

Take a look at the example code from the book for how to split these up.

A ternary operator is still if else. It still adds cyclomatic complexity.

Edit: regardless, you still have not done what I asked. I already addressed this approach back in post 11.


#56

Ok, I am already reading the book so I will withhold any further comment until I finish the book.

While I was actually using the class outside your app I did find a small problem in the update routine. Allow me to suggest a minor tweak.

As is, you set a variable for the Primary Key which is great, but the routine requires that the key be named id. Since I use a contact_id type naming convention, I noticed it right off. Also there is a minor issue in the query that is generated. Right now, the query (minus the form fields) says (Example: UPDATE mytable SET id = 5 WHERE id=5. There is no need to update the key value with the exact same key value. The “fix” for both is quite minimal. Take not of the two changes I made.

    public function update($fields) {
        $id = array_pop($fields);// benanamen mod
        $query = ' UPDATE `' . $this->table .'` SET ';
        foreach ($fields as $key => $value) {
            $query .= '`' . $key . '` = :' . $key . ',';
        }
        $query = rtrim($query, ',');
        $query .= ' WHERE `' . $this->primaryKey . '` = :primaryKey';
        //Set the :primaryKey variable
        $fields['primaryKey'] = $id;// benanamen mod
        $fields = $this->processDates($fields);
        $this->query($query, $fields);
    }

#57

In post #35 I did acknowledge I saw some code that appeared to be similar. Since I was not and still not familiar with all the code I didnt know the context in which it was used so I left it at that.

I think it’s best I just finish reading your book first before addressing anything you asked. It will give me a better insight into you and what your are teaching. It’s been a really good read so far. I would definitely recommend people read it no matter what skill level they are at.


#58

That fix assumes the key is the first entry in the array. Try putting your hidden id input at the bottom of the form and send it what happens

As we know the pk you could unset it, but in the interest of avoiding unnecessary complexity, I didn’t cover that in the book.

Please can you show how you would handle two different forms? Doing so will demonstrate to yourself where the repetition is and which concerns you are mixing.

Ultimately, using my approach I don’t even need to repeat the code calling the save function for each additional form (can post code later when I’m not on mobile).


#59

Actually, the key IS at the bottom and it works just fine. Take note of the simple form I posted, the hidden id is the very last item.


#60

Sorry, yes programming for 20 years and I still mix up pop and unshift :stuck_out_tongue: try putting it at the top of the form.


#61

No need, it would obviously not work. As your book says, the Class author gets to tell people how to use it which would include the hidden id at the bottom of the form. But I get a noob could stick it anywhere. In fact, in my noob days, I think I used to stick hidden fields at the top before anything.

So how would you address a DB key not named id then?


#62

It stores the pk column name in the primaryKey property and references that.