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

I was talking about editing, not inserting. If you are doing an edit there is no reason to care anything whatsoever about an insert.

Please re-read what I posted. The error is a database error and it even has it’s own ERROR code number, 1062. And a successful edit does in fact absolutely depend on it in order to work. The update will not execute without it.

Again I am talking about editing so that is not the logic at all. I already know there is an existing record otherwise I would not be editing it.

Thats easy. Just rename joke.joketext to have an X at the end to simulate a query failure other than a duplicate key constraint error. With production settings turned on you would get a blank page and no indication as to why. Pretty sure that would be unexpected behavior from the users point of view.

A software bug is an error, flaw, failure or fault in a computer program or system that causes it to produce an incorrect or unexpected result, or to behave in unintended ways.

But, but, I am talking about doing an edit. Why do I have to care about inserts???

But, but I just want to do an edit. Why are we still talking about inserts??? Why do I have to care about inserts? I just want to do my edit.

Really? Because the whole implementation depends on it failing for a particular reason in order to do an update. If it fails for any other reason, the whole thing fails. That is a bug.

You are writing code smells for the sake of multi DB “flexibility” when you should be coding to an interface and having the proper DB object available for the particular DB you are using. “Flexibility” sure sounds alot like more than one responsibility.

Not sure where you get two exceptions. All it takes is one failed insert that is not from a duplicate key error and the built in bug shows its face. Are you saying you write your code based on your perceived likelihood of something happening instead of that it CAN happen?

Are you really saying that on a production server there will never be an unintended query failure? I would like to rent that server please. And since when can we ever put malicious users aside?

But, but, what about the bug that I described? It’s really there. I described a very easy way to simulate it.

As far as viable alternatives I will see about posting something. I will consider your selling points in whatever I show. It seems you are stuck in the implementation of the caller which seems to be limiting “viable alternatives”. I haven’t had time to review that code yet so I can’t comment on that.

I keep telling you, I just want to do my update. I don’t care about an insert. Why do you insist on making me care about it?

I am not even close to upset. I love talking code, especially at a higher level as we are doing. Hopefully I will learn some new things. @rpkamp has already learned me up about “Command Query Separation”. Never heard the term before this thread, but the description makes a lot of sense. Trust me, I am all about adopting better practices and refactoring my code as I learn new and better techniques. I am just not seeing it in this one methods implementation, but I am listening. If I had any issue it would just be that this coding method is being taught in a book aimed at beginners (and up).

If you would please, clarify exactly what problem you intended to solve with this particular implementation. It will help coming up with a “viable alternative”.

@TomB, I started reading the book. I am already seeing some impressive coding techniques. Nice job. Looks like I will have to do some refactoring now.

2 Likes

This, I think is the point you are missing. When the user submits the form, I do not know if they are doing an update or an insert, I don’t want to have to write code specifically for edit or for insert. The user submits the form, the record is sent off to the database.

Whatever argument you are making here also applies to ON DUPLICATE KEY UPDATE.

Terminology aside, how do you think ON DUPLICATE KEY UPDATE works? Here’s the code: https://github.com/MariaDB/server/blob/769166e9ddea723143630da16323a583d0f3eca4/storage/innobase/row/row0ins.cc it tries the insert, detects the key constraint and then does an update if the insert failed.

A bug is a situation where the code not work as intended. This is not a bug.

If you think it is, please demonstrate a situation where the expected behaviour does not occur. This is not a bug, it’s a try/catch statement you don’t like for who knows why.

  1. This will happen if you use only the insert or update function alone (not through save)
  2. Only if you have your website’s error handling set up poorly. A try/catch should be set up around the entire site so that errors can be displayed in a more meaningful way. On production, just a generic error page and logged, on dev shown to the developer.
  3. If someone renames a form field in the browser they should expect things to break once it’s submitted

You haven’t made a case here at all. You dislike try/catch for who knows what reason and you’re poorly trying to argue against its use. I’ll ask again, do you have a problem with this code:

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

This is identical: It tries to insert, if the insert fails it does an update. In the same way it is still “relying on an error”. It doesn’t matter if the try to insert otherwise update logic is an if statement, a try/catch or ON DUPLICATE KEY UPDATE

If my code looked like that originally I guarantee you never would have questioned it.

All that is happening here is a database agnostic ON DUPLICATE KEY UPDATE.

See my first post in this thread: Make it so that the logic of if there's an existing record, update, otherwise insert does not need to be repeated. One approach we already discussed (and you don’t seem to mind) is ON DUPLICATE KEY UPDATE. I’ve told you why that’s not a viable solution. The try/catch approach is just a database agnostic version of that. Any argument you make about “not wanting to insert only update” also applies to ON DUPLICATE KEY so you’re at a dead end there because you suggested that earlier.

You entire argument seems to be “I don’t like it” because you have some weird aversion to try/catch that you haven’t been able to explain. I’ve shown you the benefits of this approach and you won’t provide an example of where this approach causes any kind of practical issue (beyond what the insert function would on its own if given invalid data). I’m done with this thread until you can do that.

Here’s what I want you to do:

Provide an implementation for the save function that would pass all unit tests that the original version would and then explain why it is better.

Hi @TomB,
Thanks for the detailed responses. I could show you a simple single add/edit implementation but I think based on your replies you may consider it violating DRY if your definition is not what the founders of it say it is. My problem is not necessarily the try/catch. It is that this implementation requires doing an insert attempt in order to do an edit and that the code assumes the failed insert is from a duplicate key constraint error and doesn’t handle the case when it is some other reason for a failed insert.

Let me ask you this, roles and permissions are a very real part of applications. What happens when my role or permissions only allows me to edit? Now I am screwed because in order for me to do my edit only job I have to also have insert permissions. What now Tom? How would you deal with that with the current implementation?

Anyway, after reviewing a bit more of the code it seems you have something going on close to what I would propose as the solution. In it’s simplest form, if there is a GET id you know you are getting data to doing an edit. The GET id would populate a hidden id form input. When you have a POST with the hidden id value then the code knows to do an update. If there is a POST with no value for the hidden id you know it is an insert. Very simple really. Just a little if/else. It should make sense but I can do a code example if need be.

By the way, the implementation I will post tomorrow does not require any changes to the current Class, it just won’t use the save method.

Aw man, I remember you challenging @John_Betong to put some code up on GitHub (here) and he did. I’ve been following this thread and it would be slightly disappointing if you don’t put your money where your mouth is.

1 Like

OFF TOPIC: Sure, I’ll post it tomorrow. It’s a bit late in the evening right now. You know, I didn’t see that he posted that. He waited some time to do it I will have to check it out. Hardly a challenge though, but I think I get what your getting at. I don’t want to disappoint you. :smile:

2 Likes

This is a red herring. Permissions checks happen before any data is sent to the database for storage so it’s the same whether you are using save or update.

I will remind you once again that it’s a book aimed at teaching novices the concepts and I purposely didn’t want them to get bogged down with unnecessary complexity.

I completely understand Tom. As I posted earlier, my posts are about code, rather one method in particular, not the book itself. After reading several chapters I am quite impressed with the level of expertise it shows and have already learned a couple new techniques. Well done!

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.

1 Like

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.

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.

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.

1 Like

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.

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?

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.

2 Likes

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>

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)

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. @rpkamp! :+1: